linux-kernel - Re: [PATCH v2 06/19] gendwarfksyms: Add a cache for processed DIEs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABCJKucgYt09P=cjD8J6Epi67XXvJ7hBdQksLBt9-UtFSu8Pug@mail.gmail.com>
Date: Thu, 5 Sep 2024 10:19:10 -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>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Neal Gompa <neal@...pa.dev>, 
	Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 06/19] gendwarfksyms: Add a cache for processed DIEs

Hi Petr,

On Mon, Sep 2, 2024 at 3:05 AM Petr Pavlu <petr.pavlu@...e.com> wrote:
>
> On 8/15/24 19:39, Sami Tolvanen wrote:
> > +void die_map_free(void)
> > +{
> > +     struct hlist_node *tmp;
> > +     unsigned int stats[LAST + 1];
> > +     struct die *cd;
> > +     int i;
> > +
> > +     memset(stats, 0, sizeof(stats));
> > +
> > +     hash_for_each_safe(die_map, i, tmp, cd, hash) {
> > +             stats[cd->state]++;
> > +             reset_die(cd);
> > +             free(cd);
> > +     }
> > +     hash_init(die_map);
> > +
> > +     if ((map_hits + map_misses > 0))
>
> Nit: Extra parentheses can be dropped.

Oops, I'll fix that.

> > +             /*
> > +              * If any of the DIEs in the scope is missing a name, consider
> > +              * the DIE to be unnamed.
> > +              */
> > +             list[count] = get_name(&scopes[i]);
> > +
> > +             if (!list[count]) {
> > +                     free(scopes);
> > +                     return 0;
> > +             }
>
> This slightly changes how scopes with no name are processed which is
> unrelated to the added caching. The previous logic used "<unnamed>" for
> individual unnamed scopes. The new code in such a case returns an empty
> FQN which is turned in process_fqn() into "<unnamed>".
>
> This is likely ok in practice for this particular tool. In general,
> I think "<unnamed>" should be returned when the initial DIE is missing
> a name and something like "<anonymous>::foo" when an outer scope has no
> name.

I did consider that, but didn't find instances of anonymous scopes in
the output, so I simplified this a bit. I'll dig around a bit more and
change this if I find a use case. Note that going through the scopes
is mostly just needed for Rust code.

> More importantly, using "<unnamed>" when a type has no name looks to me
> overly verbose, in particular, when it comes to the symtypes output. For
> instance, the current output for a 'const char *' parameter is:
> formal_parameter pointer_type <unnamed> { const_type <unnamed> { base_type char byte_size(1) encoding(8) } } byte_size(8)
>
> .. while the following should be sufficient and easier to grasp:
> formal_parameter pointer_type { const_type { base_type char byte_size(1) encoding(8) } } byte_size(8)

Agreed, that's way more readable. I'll drop the "<unnamed>" from the output.

> > +     for (i = 0; i < count; i++)
> > +             strcat(*fqn, list[i]);
>
> Small optimization: This loop could be written as follows to avoid
> repeatedly searching the end of fqn:
>
> char *p = *fqn;
> for (i = 0; i < count; i++)
>         p = stpcpy(p, list[i]);

True, I'll change this. Thanks!

> > +static int process_fqn(struct state *state, struct die *cache, Dwarf_Die *die)
> > +{
> > +     const char *fqn;
> > +
> > +     if (!cache->fqn)
> > +             check(get_fqn(state, die, &cache->fqn));
> > +
> > +     fqn = cache->fqn;
> > +     fqn = fqn ?: "<unnamed>";
>
> As a small optimization and for consistency, I would recommended to also
> cache the "<unnamed>" name to avoid repeatedly calling get_fqn() for
> such DIEs.

Ack.

> > +enum die_state { INCOMPLETE, COMPLETE, LAST = COMPLETE };
> > +enum die_fragment_type { EMPTY, STRING, DIE };
>
> Nit: I would suggest to prefix the enum values, for example,
> STATE_INCOMPLETE, ... and FRAGMENT_EMPTY, ...

Sure, I'll add prefixes.

Sami