Message-ID: <CABCJKucj7zjc4=EiFdSnzNDBvQmaWBBt_KJsTq1ybp=Vegp5eQ@mail.gmail.com>
Date: Fri, 26 Jul 2024 21:05:22 +0000
From: Sami Tolvanen <samitolvanen@...gle.com>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Masahiro Yamada <masahiroy@...nel.org>, 
	Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Matthew Maurer <mmaurer@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 00/15] Implement MODVERSIONS for Rust

Hi Petr,

On Mon, Jul 22, 2024 at 8:20 AM Petr Pavlu <petr.pavlu@...e.com> wrote:
>
> From my perspective, I'm okay if gendwarfksyms doesn't provide
> functionality to compare a new object file with its reference symtypes
> file.
>
> As mentioned, genksyms has this functionality but I actually think the
> way it works is not ideal. Its design is to operate on one compilation
> unit at the time. This has the advantage that a comparison of each file
> is performed in parallel during the build, simply because of the make
> job system. On the other hand, it has two problems.
>
> The first one is that genksyms doesn't provide a comparison of the
> kernel as a whole. This means that the tool gives rather scattered and
> duplicated output about changed structs in the build log. Ideally, one
> would like to see a single compact report about what changed at the end
> of the build.

Sure, that makes sense. Android uses STG for this, which might be
useful to other folks too:

https://android.googlesource.com/platform/external/stg/
https://android.googlesource.com/platform/external/stg/+/refs/heads/main/doc/stgdiff.md#output-formats

> A few months ago, I also started working on a tool inspired by this
> script. The goal is to have similar functionality but hopefully with
> a much faster implementation. Hence, this tool is written in a compiled
> language (Rust at the moment) and should also become multi-threaded. I'm
> hoping to find some time to make progress on it and make the code
> public. It could later be added to the upstream kernel to replace the
> comparison functionality implemented by genksyms, if there is interest.
>
> So as mentioned, I'm fine if gendwarfksyms doesn't have this
> functionality. However, for distributions that rely on the symtypes
> format, I'd be interested in having gendwarfksyms output its dump data
> in this format as well.

We can definitely tweak the output format, but I'm not sure if making
it fully compatible with the genksyms symtypes format is feasible,
especially for Rust code. I also intentionally decided to use DWARF
tag names in the output instead of shorthands like s# etc. to make it
a bit more readable.

> For example, instead of producing:
>
> gendwarfksyms: process_exported_symbols: _some_mangled_func_name (@ XYZ)
> subprogram(
>    [formal parameters...]
> )
> -> structure_type core::result::Result<(), core::fmt::Error> {
>    [a description of the structure...]
> };
>
> .. the output could be something like this:
>
> S#'core::result::Result<(), core::fmt::Error>' structure_type core::result::Result<(), core::fmt::Error> { [a description of the structure...] }
> _some_mangled_func_name subprogram _some_mangled_func_name ( [formal parameters...] ) -> S#'core::result::Result<(), core::fmt::Error>'

This wouldn't be enough to make the output format compatible with
symtypes though. genksyms basically produces a simple key-value pair
database while gendwarfksyms currently outputs the fully expanded type
string for each symbol. If you need the tool to produce a type
database, it might also be worth discussing if we should use a bit
less ad hoc format in that case.

One more thing to note about the current --debug output is that it
directly correlates with the debugging information and thus may not
contain all aliases. For example, the Rust compiler deduplicates
identical function implementations (e.g. Deref::deref and
DerefMut::deref_mut etc.), but only one of the symbol names appears in
DWARF. We use symbol addresses to print out #SYMVERs also for the
aliases, but they don't show up in the debugging output right now.

> > If using unions here is acceptable to everyone, a simple solution
> > would be to use a known name prefix for the reserved members and teach
> > gendwarfksyms to only print out the original type for the replaced
> > ones. For example:
> >
> > The initial placeholder:
> >
> >     u8 __kabi_reserved_1[8];
> >
> > After replacement:
> >
> >     union {
> >             u64 new_member;
> >             struct {
> >                     u8 __kabi_reserved_1[8];
> >             };
> >     }
> >
> > Here gendwarfksyms would see the __kabi_reserved prefix and only use
> > u8 [8] for the CRC calculation. Does this sound reasonable?
>
> I like this idea. I think it's good that the necessary kABI information
> about an updated member can be expressed at the source code level in
> place of the actual change, and it isn't needed to feed additional input
> to the tool.

OK, cool. I agree that being able to specify these details in source
code is much cleaner. I'll add an implementation for this, and for the
definition visibility issue Greg mentioned in v2.

Sami

Powered by blists - more mailing lists