From: Masahiro Yamada <masahiroy@kernel.org>
To: Giuliano Procida <gprocida@google.com>
Cc: Sami Tolvanen <samitolvanen@google.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-modules@vger.kernel.org, linux-kbuild@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] gendwarfksyms: order -T symtypes output by name
Date: Tue, 1 Jul 2025 00:40:57 +0900 [thread overview]
Message-ID: <CAK7LNAQo0CyMdsVSg4dfWjVU+uYUSMdmwgLEdpRfTVcgOTuwzg@mail.gmail.com> (raw)
In-Reply-To: <CAGvU0HkKacQKB1q9NWcqChLGoMB+1vu9UdqYc+tBRbTTc3++GQ@mail.gmail.com>
On Mon, Jun 30, 2025 at 10:46 PM Giuliano Procida <gprocida@google.com> wrote:
>
> On Mon, 30 Jun 2025 at 14:24, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Mon, Jun 30, 2025 at 7:05 PM Giuliano Procida <gprocida@google.com> wrote:
> > >
> > > Hi.
> > >
> > > On Sun, 29 Jun 2025 at 18:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@google.com> wrote:
> > > > >
> > > > > When writing symtypes information, we iterate through the entire hash
> > > > > table containing type expansions. The key order varies unpredictably
> > > > > as new entries are added, making it harder to compare symtypes between
> > > > > builds.
> > > > >
> > > > > Resolve this by sorting the type expansions by name before output.
> > > > >
> > > > > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > > > ---
> > > > > scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> > > > > 1 file changed, 26 insertions(+), 3 deletions(-)
> > > > >
> > > > > [Adjusted the first line of the description. Added reviewer tags.
> > > > > Added missing CC to linux-modules.]
> > > > >
> > > > > diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> > > > > index 7bd459ea6c59..51c1471e8684 100644
> > > > > --- a/scripts/gendwarfksyms/types.c
> > > > > +++ b/scripts/gendwarfksyms/types.c
> > > > > @@ -6,6 +6,8 @@
> > > > > #define _GNU_SOURCE
> > > > > #include <inttypes.h>
> > > > > #include <stdio.h>
> > > > > +#include <stdlib.h>
> > > > > +#include <string.h>
> > > > > #include <zlib.h>
> > > > >
> > > > > #include "gendwarfksyms.h"
> > > > > @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> > > > > return -1;
> > > > > }
> > > > >
> > > > > +static int cmp_expansion_name(const void *p1, const void *p2)
> > > > > +{
> > > > > + struct type_expansion *const *e1 = p1;
> > > > > + struct type_expansion *const *e2 = p2;
> > > > > +
> > > > > + return strcmp((*e1)->name, (*e2)->name);
> > > > > +}
> > > > > +
> > > > > static void type_map_write(FILE *file)
> > > > > {
> > > > > struct type_expansion *e;
> > > > > struct hlist_node *tmp;
> > > > > + struct type_expansion **es;
> > > > > + size_t count = 0;
> > > > > + size_t i = 0;
> > > > >
> > > > > if (!file)
> > > > > return;
> > > > >
> > > > > - hash_for_each_safe(type_map, e, tmp, hash) {
> > > > > - checkp(fputs(e->name, file));
> > > > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > > > + ++count;
> > > > > + es = xmalloc(count * sizeof(struct type_expansion *));
> > > >
> > > > Just a nit:
> > > >
> > > > es = xmalloc(count * sizeof(*es));
> > > >
> > > > is better?
> > > >
> > > > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > > > + es[i++] = e;
> > > > > +
> > > > > + qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
> > > >
> > > > qsort(es, count, sizeof(*es), cmp_expansion_name);
> > > >
> > >
> > > That's a fair point.
> > >
> > > However, in the gendwarfksyms code, all but one of the sizeofs uses an
> > > explicit type name. The exception is sizeof(stats) where stats is an array.
> > >
> > > I'll leave Sami's code as it is.
> >
> >
> > This rule is clearly documented with rationale.
> >
> > See this:
> > https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
> >
> >
>
> I can follow up with a change that adjusts all occurrences. That
> shouldn't take long at all.
I expected a new patch version (I do not know whether it is v2 or v3 since
you do not add such a prefix),
instead of breaking the style, and fixing it in a follow-up patch.
--
Best Regards
Masahiro Yamada
prev parent reply other threads:[~2025-06-30 15:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-23 9:21 [PATCH] gendwarfksyms: make -T symtypes output be in name order Giuliano Procida
2025-06-23 9:32 ` Greg Kroah-Hartman
2025-06-23 15:22 ` Sami Tolvanen
2025-06-25 9:52 ` [PATCH] gendwarfksyms: order -T symtypes output by name Giuliano Procida
2025-06-29 17:51 ` Masahiro Yamada
2025-06-30 10:05 ` Giuliano Procida
2025-06-30 13:24 ` Masahiro Yamada
2025-06-30 13:45 ` Giuliano Procida
2025-06-30 14:27 ` [PATCH] gendwarfksyms: use preferred form of sizeof for allocation Giuliano Procida
2025-06-30 15:40 ` Masahiro Yamada [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAK7LNAQo0CyMdsVSg4dfWjVU+uYUSMdmwgLEdpRfTVcgOTuwzg@mail.gmail.com \
--to=masahiroy@kernel.org \
--cc=gprocida@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-modules@vger.kernel.org \
--cc=samitolvanen@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).