Message-ID: <CABCJKuejz2snUN12d7rA7-nNMXjUx9BKYmp86c43E4gJCjZFbA@mail.gmail.com>
Date: Wed, 23 Oct 2024 10:47:54 -0700
From: Sami Tolvanen <samitolvanen@...gle.com>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Matthew Maurer <mmaurer@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Daniel Gomez <da.gomez@...sung.com>, Neal Gompa <neal@...pa.dev>, Hector Martin <marcan@...can.st>, 
	Janne Grunau <j@...nau.net>, Miroslav Benes <mbenes@...e.cz>, Asahi Linux <asahi@...ts.linux.dev>, 
	Sedat Dilek <sedat.dilek@...il.com>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v4 14/19] gendwarfksyms: Add support for kABI rules

Hi,

On Tue, Oct 22, 2024 at 7:39 AM Petr Pavlu <petr.pavlu@...e.com> wrote:
>
> On 10/8/24 20:38, Sami Tolvanen wrote:
> > +/*
> > + * KABI_USE_ARRAY(fqn)
> > + *   Treat the struct fqn as a declaration, i.e. even if a definition
> > + *   is available, don't expand the contents.
> > + */
> > +#define KABI_STRUCT_DECLONLY(fqn) __KABI_RULE(struct_declonly, fqn, ;)
>
> Nit: s/KABI_USE_ARRAY/KABI_STRUCT_DECLONLY/ in the preceding comment.

Thanks, I'll fix this in the next version.

> > + * Verify --stable output:
> > + *
> > + * RUN: echo -e "ex0\nex1" | \
> > + * RUN:   ./gendwarfksyms --stable --dump-dies \
> > + * RUN:      examples/kabi_rules.o 2>&1 >/dev/null | \
> > + * RUN:   FileCheck examples/kabi_rules.c --check-prefix=STABLE
> > + */
>
> It would be useful to make this test automated. Overall, I believe
> gendwarfksyms should have a set of automated tests to verify its
> functionality. At a minimum, I think we would want to work out some
> blueprint how to write them. Should they be added to kselftests, or
> would something like kconfig/tests be more appropriate? How to write
> tests with stable DWARF data that ideally work across all platforms?
> More tests can be then added incrementally.

Different compilers produce slightly different DWARF data, so we can't
necessarily guarantee that the output is the same even between
different compilers, let alone architectures, which makes automated
testing a bit more challenging. If we want tests for simple cases like
in these example files, it should be possible to work something out.
Otherwise, I think the best way to test the tool is to do build tests
and ensure that there are no warnings or errors, e.g. for missing
versions. Did you have any thoughts about the kinds of tests you'd
like to see?

> > +#define KABI_RULE_EMPTY_VALUE ";"
>
> Hmm, is there a reason why an empty value is ";" instead of just ""?

Not really, I can change this to an empty string in v5.

> > +
> > +/*
> > + * Rule: struct_declonly
> > + * - For the struct in the target field, treat it as a declaration
> > + *   only even if a definition is available.
> > + */
> > +#define KABI_RULE_TAG_STRUCT_DECLONLY "struct_declonly"
> > +
> > +/*
> > + * Rule: enumerator_ignore
> > + * - For the enum in the target field, ignore the named enumerator
> > + *   in the value field.
> > + */
> > +#define KABI_RULE_TAG_ENUMERATOR_IGNORE "enumerator_ignore"
> > +
> > +enum kabi_rule_type {
> > +     KABI_RULE_TYPE_UNKNOWN,
> > +     KABI_RULE_TYPE_STRUCT_DECLONLY,
> > +     KABI_RULE_TYPE_ENUMERATOR_IGNORE,
> > +};
>
> Nit: All new KABI_* defines and the enum kabi_rule_type added in
> gendwarfksyms.h are used only locally from kabi.c, so they could be
> moved in that file.

True, I'll move these.

> > +struct rule {
> > +     enum kabi_rule_type type;
> > +     const char *target;
> > +     const char *value;
> > +     struct hlist_node hash;
> > +};
>
> What is the idea behind using 'const char *' instead of 'char *' for
> owned strings in structures?

I mentioned this in the previous response, but it's to make it more
obvious that the contents of these strings shouldn't be modified by
the users of this struct.

> > +static inline unsigned int rule_hash(enum kabi_rule_type type,
> > +                                  const char *target, const char *value)
> > +{
> > +     return hash_32(type) ^ hash_str(target) ^ hash_str(value);
> > +}
> > +
> > +static inline unsigned int __rule_hash(const struct rule *rule)
> > +{
> > +     return rule_hash(rule->type, rule->target, rule->value);
> > +}
>
> Nit: Perhaps call the two hash functions rule_values_hash() and
> rule_hash() to avoid the "__" prefix?

Sure, I'll rename the functions.

> As a general comment, I believe the gendwarfksyms code overuses the "__"
> prefix. Similarly, I find harder to navigate its code when, in a few
> instances, there is a function named <verb>_<object>() and another as
> <object>_<verb>(). An example of both would be the functions
> expand_type(), type_expand() and __type_expand().

I suspect this is a matter of personal preference. I don't have strong
feelings about the naming, but I'm happy to accept suggestions!

> > +     scn = elf_nextscn(elf, NULL);
> > +
> > +     while (scn) {
> > +             shdr = gelf_getshdr(scn, &shdr_mem);
> > +             if (shdr) {
>
> Isn't it an error when gelf_getshdr() returns NULL and as such it should
> be reported with error()? If this makes sense then the same handling
> should be implemented in symbols.c:elf_for_each_global().

Good point, I'll change this, also in symbols.c.

>
> > +                     const char *sname =
> > +                             elf_strptr(elf, shstrndx, shdr->sh_name);
> > +
> > +                     if (sname && !strcmp(sname, KABI_RULE_SECTION)) {
> > +                             rule_data = elf_getdata(scn, NULL);
>
> Similarly here for elf_strptr() and elf_getdata().

Ack.

Sami

Powered by blists - more mailing lists