| Message ID | 20241121204220.2378181-20-samitolvanen@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Implement DWARF modversions | expand |
On Thu, Nov 21, 2024 at 9:42 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi, > > Here's v6 of the DWARF modversions series. The main motivation is > modversions support for Rust, which is important for distributions > like Android that are about to ship Rust kernel modules. Per Luis' > request [1], v2 dropped the Rust specific bits from the series and > instead added the feature as an option for the entire kernel to > make it easier to evaluate the benefits of this approach, and to > get better test coverage. Matt is addressing Rust modversion_info > compatibility issues in a separate patch set [2] that depends on > this series, and actually allows modversions to be enabled with > Rust. > > Short background: Unlike C, Rust source code doesn't have sufficient > information about the final ABI, as the compiler has considerable > freedom in adjusting structure layout, for example, which makes > using a source code parser like genksyms a non-starter. Based on > earlier feedback, this series uses DWARF debugging information for > computing versions. DWARF is an established and a relatively stable > format, which includes all the necessary ABI details, and adding a > CONFIG_DEBUG_INFO dependency for Rust symbol versioning seems like a > reasonable trade-off as most distributions already enable it. > > The first 15 patches add gendwarfksyms, a tool for computing symbol > versions from DWARF. When passed a list of exported symbols and > object files, the tool generates an expanded type string for each > symbol and computes symbol versions. gendwarfksyms is written in C, > uses libdw to process DWARF, and zlib for CRC32. Patch 16 ensures > that debugging information is present where we need it, patch 17 > adds gendwarfksyms as an alternative to genksyms, and the last patch > adds documentation. > > v6 has changes to structure expansion and the kABI stability > features based on our backtesting results with previous Android > release kernels (see the change log below). It's also based on > linux-kbuild/for-next to include symtypes build rule clean-ups from > Masahiro [3]. For your convenience, the series is also available > here: > > https://github.com/samitolvanen/linux/commits/gendwarfksyms-v6 > Thanks for the update, Sami. What are your plans to get this upstream? Is Linux 6.13 the new development base? Personally, I would like to see a Linux v6.12 LTS version offered. LTO is not supported - might be worth mentioning this in the documentation patch with some explanations? BTW, I am testing with the latest kmod-git and pahole-git. I will give this a try when Linux v6.12.1 is released. Best regards, -Sedat- > If you also want to test the series with actual Rust modules, this > branch adds Matt's latest modversion_info series: > > https://github.com/samitolvanen/linux/commits/rustmodversions-v6 > > Sami > > > [1] https://lore.kernel.org/lkml/ZnIZEtkkQWEIGf9n@bombadil.infradead.org/ > [2] https://lore.kernel.org/linux-modules/20241030-extended-modversions-v8-0-93acdef62ce8@google.com/ > [3] https://lore.kernel.org/linux-modules/CAK7LNAR9c+EEsOvPPn4qSq3gAFskYOXVd=dg8O+bKeeC-HMifw@mail.gmail.com/ > > --- > > v6: > - Dropped pointer expansion limits as this affects version > stability when exported symbols are removed. (Patch 9) > > - Changed local type definitions (in .c files) that are opaque > to external users to be treated as declarations even if a > definition is available. (Patch 9) > > - Switched to zlib's CRC32 implementation per Masahiro's > suggestion. (Patch 12) > > - Renamed struct_declonly kABI rule to simply declonly, as it > applies also to unions and enums, added a new rule for > overriding enumerator values, and refactored the examples. > (Patch 13) > > - Added --stable support for renamed structure members and > also added examples. (Patch 14) > > - Rebased on linux-kbuild/for-next for Masahiro's symtypes > build rule clean-ups. (Patch 17) > > - Updated the documentation reflect --stable changes. (Patch 18) > > v5: https://lore.kernel.org/lkml/20241030170106.1501763-21-samitolvanen@google.com/ > - Rebased on v6.12-rc5. > > - Fixed an issue with limiting structure expansion, and applied > Petr's clean-up. (Patch 10) > > - Dropped an unnecessary return statement in error path. (Patch > 12) > > - Addressed several other smaller issues Petr brought up. (Patches > 13, 14, and 15) > > - Added a KBUILD_GENDWARFKSYMS_STABLE flag to enable --stable for > the entire kernel build. (Patch 18) > > - Updated documentation to include KBUILD flags. (Patch 19) > > - Picked up Reviewed-by tags from v4. > > v4: https://lore.kernel.org/lkml/20241008183823.36676-21-samitolvanen@google.com/ > - Rebased on v6.12-rc2, which now includes all the prerequisites. > > - Dropped unnecessary name_only parameter for symbols.c::for_each > and cleaned up error handling. (Patch 3) > > - Fixed anonymous scope handling to ensure unnamed DIEs don't get > names. (Patch 4) > > - Added non-variant children to variant_type output, and included > DW_AT_discr_value attributes for variants. (Patch 9) > > - Added another symbol pointer test case. (Patch 16) > > - Picked up (Acked|Reviewed)-by tags from v3. > > v3: https://lore.kernel.org/lkml/20240923181846.549877-22-samitolvanen@google.com/ > - Updated SPX license headers. > > - Squashed the first two patches in v2 and tried to reduce churn as > much as reasonable. > > - Dropped patch 18 from v2 ("x86/asm-prototypes: Include > <asm/ptrace.h>") as it's addressed by a separate patch. > > - Changed the error handling code to immediately terminate instead > of propagating the errors back to main, which cleaned up the code > quite a bit. > > - Switched to the list and hashtable implementations in scripts and > dropped the remaining tools/include dependencies. Added a couple > missing list macros. (patch 1) > > - Moved the genksyms CRC32 implementation to scripts/include and > dropped the duplicate code. (patches 2 and 14) > > - Switched from ad-hoc command line parsing to getopt_long (patch 3). > > - Added structure member and function parameter names to the DIE > output to match genksyms behavior, and tweaked the symtypes format > to be more parser-friendly in general based on Petr's suggestions. > > - Replaced the declaration-only struct annotations with more generic > kABI stability rules that allow source code annotations to be used > where #ifndef __GENKSYMS__ was previously used. Added support for > rules that can be used to exclude enumerators from versioning. > (patch 16) > > - Per Miroslav's suggestion, added an option to hide structure > members from versioning when they're added to existing alignment > holes, for example. (patch 16) > > - Per Greg's request, added documentation and example macros for the > --stable features, and a couple of test cases. (patches 15, 16, and > 20) > > - Fixed making symtypes files, which need to depend on .o files with > gendwarfksyms. (patch 19) > > - Addressed several other smaller issues that Petr and Masahiro > kindly pointed out during the v2 review. > > v2: https://lore.kernel.org/lkml/20240815173903.4172139-21-samitolvanen@google.com/ > - Per Luis' request, dropped Rust-specific patches and added > gendwarfksyms as an alternative to genksyms for the entire > kernel. > > - Added support for missing DWARF features needed to handle > also non-Rust code. > > - Changed symbol address matching to use the symbol table > information instead of relying on addresses in DWARF. > > - Added __gendwarfksyms_ptr patches to ensure the compiler emits > the necessary type information in DWARF even for symbols that > are defined in other TUs. > > - Refactored debugging output and moved the more verbose output > behind --dump* flags. > > - Added a --symtypes flag for generating a genksyms-style > symtypes output based on Petr's feedback, and refactored > symbol version calculations to be based on symtypes instead > of raw --dump-dies output. > > - Based on feedback from Greg and Petr, added --stable flag and > support for reserved data structure fields and declaration-onl > structures. Also added examples for using these features. > > - Added a GENDWARFKSYMS option and hooked up kbuild support > for both C and assembly code. Note that with gendwarfksyms, > we have to actually build a temporary .o file for calculating > assembly modversions. > > v1: https://lore.kernel.org/lkml/20240617175818.58219-17-samitolvanen@google.com/ > > --- > > Sami Tolvanen (18): > tools: Add gendwarfksyms > gendwarfksyms: Add address matching > gendwarfksyms: Expand base_type > gendwarfksyms: Add a cache for processed DIEs > gendwarfksyms: Expand type modifiers and typedefs > gendwarfksyms: Expand subroutine_type > gendwarfksyms: Expand array_type > gendwarfksyms: Expand structure types > gendwarfksyms: Limit structure expansion > gendwarfksyms: Add die_map debugging > gendwarfksyms: Add symtypes output > gendwarfksyms: Add symbol versioning > gendwarfksyms: Add support for kABI rules > gendwarfksyms: Add support for reserved and ignored fields > gendwarfksyms: Add support for symbol type pointers > export: Add __gendwarfksyms_ptr_ references to exported symbols > kbuild: Add gendwarfksyms as an alternative to genksyms > Documentation/kbuild: Add DWARF module versioning > > Documentation/kbuild/gendwarfksyms.rst | 308 ++++++ > Documentation/kbuild/index.rst | 1 + > include/linux/export.h | 15 + > kernel/module/Kconfig | 31 + > scripts/Makefile | 3 +- > scripts/Makefile.build | 35 +- > scripts/gendwarfksyms/.gitignore | 2 + > scripts/gendwarfksyms/Makefile | 12 + > scripts/gendwarfksyms/cache.c | 51 + > scripts/gendwarfksyms/die.c | 166 +++ > scripts/gendwarfksyms/dwarf.c | 1158 ++++++++++++++++++++ > scripts/gendwarfksyms/examples/kabi.h | 157 +++ > scripts/gendwarfksyms/examples/kabi_ex.c | 30 + > scripts/gendwarfksyms/examples/kabi_ex.h | 263 +++++ > scripts/gendwarfksyms/examples/symbolptr.c | 33 + > scripts/gendwarfksyms/gendwarfksyms.c | 185 ++++ > scripts/gendwarfksyms/gendwarfksyms.h | 301 +++++ > scripts/gendwarfksyms/kabi.c | 333 ++++++ > scripts/gendwarfksyms/symbols.c | 339 ++++++ > scripts/gendwarfksyms/types.c | 477 ++++++++ > 20 files changed, 3893 insertions(+), 7 deletions(-) > create mode 100644 Documentation/kbuild/gendwarfksyms.rst > create mode 100644 scripts/gendwarfksyms/.gitignore > create mode 100644 scripts/gendwarfksyms/Makefile > create mode 100644 scripts/gendwarfksyms/cache.c > create mode 100644 scripts/gendwarfksyms/die.c > create mode 100644 scripts/gendwarfksyms/dwarf.c > create mode 100644 scripts/gendwarfksyms/examples/kabi.h > create mode 100644 scripts/gendwarfksyms/examples/kabi_ex.c > create mode 100644 scripts/gendwarfksyms/examples/kabi_ex.h > create mode 100644 scripts/gendwarfksyms/examples/symbolptr.c > create mode 100644 scripts/gendwarfksyms/gendwarfksyms.c > create mode 100644 scripts/gendwarfksyms/gendwarfksyms.h > create mode 100644 scripts/gendwarfksyms/kabi.c > create mode 100644 scripts/gendwarfksyms/symbols.c > create mode 100644 scripts/gendwarfksyms/types.c > > > base-commit: 3596c721c4348b2a964e43f9296a0c01509ba927 > -- > 2.47.0.371.ga323438b13-goog >
> BTW, I am testing with the latest kmod-git and pahole-git. > > I will give this a try when Linux v6.12.1 is released. > I have a prolonged build-time of +22,75 per cent. Compared gendwarfksyms-v5 + Linux-v6.12.0 VS. gendwarfksyms-v6 + Linux-v6.12.1 Is this a known issue? Best regards, -Sedat-
Hi Sedat, On Fri, Nov 22, 2024 at 3:51 AM Sedat Dilek <sedat.dilek@gmail.com> wrote: > > Thanks for the update, Sami. > > What are your plans to get this upstream? Once everything has been reviewed, I'm suspect it would be up to Masahiro to decide if he wants to pick this up. > Is Linux 6.13 the new development base? > > Personally, I would like to see a Linux v6.12 LTS version offered. This version is based on linux-kbuild/for-next, which at the time was based on 6.12-rc6, I think. It should apply cleanly to 6.12. > LTO is not supported - might be worth mentioning this in the > documentation patch with some explanations? Sure, it's probably worthwhile to add a note. Sami
Hi, On Sat, Nov 23, 2024 at 1:23 PM Sedat Dilek <sedat.dilek@gmail.com> wrote: > > > BTW, I am testing with the latest kmod-git and pahole-git. > > > > I will give this a try when Linux v6.12.1 is released. > > > > I have a prolonged build-time of +22,75 per cent. > Compared gendwarfksyms-v5 + Linux-v6.12.0 VS. gendwarfksyms-v6 + Linux-v6.12.1 > > Is this a known issue? In my tests, compared to a genksyms defconfig build with debugging information, v5 was slightly faster, and v6 was 5.6% slower. This is because the expansion limits were dropped in this version to ensure version stability when exports are removed. See the explanation here: https://lore.kernel.org/linux-modules/20241120215454.GA3512979@google.com/ I did profile this quickly and most of the time seems to be spent in libdw looking up attributes. We might be able to speed this up by limiting the number of attributes we look up depending on the DIE type, but I haven't had time to look into it yet. I'll take a closer look when I'm back from my vacation in about three weeks. Sami
On Thu, Nov 21, 2024 at 3:42 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi, > > Here's v6 of the DWARF modversions series. The main motivation is > modversions support for Rust, which is important for distributions > like Android that are about to ship Rust kernel modules. Per Luis' > request [1], v2 dropped the Rust specific bits from the series and > instead added the feature as an option for the entire kernel to > make it easier to evaluate the benefits of this approach, and to > get better test coverage. Matt is addressing Rust modversion_info > compatibility issues in a separate patch set [2] that depends on > this series, and actually allows modversions to be enabled with > Rust. > > Short background: Unlike C, Rust source code doesn't have sufficient > information about the final ABI, as the compiler has considerable > freedom in adjusting structure layout, for example, which makes > using a source code parser like genksyms a non-starter. Based on > earlier feedback, this series uses DWARF debugging information for > computing versions. DWARF is an established and a relatively stable > format, which includes all the necessary ABI details, and adding a > CONFIG_DEBUG_INFO dependency for Rust symbol versioning seems like a > reasonable trade-off as most distributions already enable it. > > The first 15 patches add gendwarfksyms, a tool for computing symbol > versions from DWARF. When passed a list of exported symbols and > object files, the tool generates an expanded type string for each > symbol and computes symbol versions. gendwarfksyms is written in C, > uses libdw to process DWARF, and zlib for CRC32. Patch 16 ensures > that debugging information is present where we need it, patch 17 > adds gendwarfksyms as an alternative to genksyms, and the last patch > adds documentation. > > v6 has changes to structure expansion and the kABI stability > features based on our backtesting results with previous Android > release kernels (see the change log below). It's also based on > linux-kbuild/for-next to include symtypes build rule clean-ups from > Masahiro [3]. For your convenience, the series is also available > here: > > https://github.com/samitolvanen/linux/commits/gendwarfksyms-v6 > > If you also want to test the series with actual Rust modules, this > branch adds Matt's latest modversion_info series: > > https://github.com/samitolvanen/linux/commits/rustmodversions-v6 > > Sami > > > [1] https://lore.kernel.org/lkml/ZnIZEtkkQWEIGf9n@bombadil.infradead.org/ > [2] https://lore.kernel.org/linux-modules/20241030-extended-modversions-v8-0-93acdef62ce8@google.com/ > [3] https://lore.kernel.org/linux-modules/CAK7LNAR9c+EEsOvPPn4qSq3gAFskYOXVd=dg8O+bKeeC-HMifw@mail.gmail.com/ > > --- > > v6: > - Dropped pointer expansion limits as this affects version > stability when exported symbols are removed. (Patch 9) > > - Changed local type definitions (in .c files) that are opaque > to external users to be treated as declarations even if a > definition is available. (Patch 9) > > - Switched to zlib's CRC32 implementation per Masahiro's > suggestion. (Patch 12) > > - Renamed struct_declonly kABI rule to simply declonly, as it > applies also to unions and enums, added a new rule for > overriding enumerator values, and refactored the examples. > (Patch 13) > > - Added --stable support for renamed structure members and > also added examples. (Patch 14) > > - Rebased on linux-kbuild/for-next for Masahiro's symtypes > build rule clean-ups. (Patch 17) > > - Updated the documentation reflect --stable changes. (Patch 18) > > v5: https://lore.kernel.org/lkml/20241030170106.1501763-21-samitolvanen@google.com/ > - Rebased on v6.12-rc5. > > - Fixed an issue with limiting structure expansion, and applied > Petr's clean-up. (Patch 10) > > - Dropped an unnecessary return statement in error path. (Patch > 12) > > - Addressed several other smaller issues Petr brought up. (Patches > 13, 14, and 15) > > - Added a KBUILD_GENDWARFKSYMS_STABLE flag to enable --stable for > the entire kernel build. (Patch 18) > > - Updated documentation to include KBUILD flags. (Patch 19) > > - Picked up Reviewed-by tags from v4. > > v4: https://lore.kernel.org/lkml/20241008183823.36676-21-samitolvanen@google.com/ > - Rebased on v6.12-rc2, which now includes all the prerequisites. > > - Dropped unnecessary name_only parameter for symbols.c::for_each > and cleaned up error handling. (Patch 3) > > - Fixed anonymous scope handling to ensure unnamed DIEs don't get > names. (Patch 4) > > - Added non-variant children to variant_type output, and included > DW_AT_discr_value attributes for variants. (Patch 9) > > - Added another symbol pointer test case. (Patch 16) > > - Picked up (Acked|Reviewed)-by tags from v3. > > v3: https://lore.kernel.org/lkml/20240923181846.549877-22-samitolvanen@google.com/ > - Updated SPX license headers. > > - Squashed the first two patches in v2 and tried to reduce churn as > much as reasonable. > > - Dropped patch 18 from v2 ("x86/asm-prototypes: Include > <asm/ptrace.h>") as it's addressed by a separate patch. > > - Changed the error handling code to immediately terminate instead > of propagating the errors back to main, which cleaned up the code > quite a bit. > > - Switched to the list and hashtable implementations in scripts and > dropped the remaining tools/include dependencies. Added a couple > missing list macros. (patch 1) > > - Moved the genksyms CRC32 implementation to scripts/include and > dropped the duplicate code. (patches 2 and 14) > > - Switched from ad-hoc command line parsing to getopt_long (patch 3). > > - Added structure member and function parameter names to the DIE > output to match genksyms behavior, and tweaked the symtypes format > to be more parser-friendly in general based on Petr's suggestions. > > - Replaced the declaration-only struct annotations with more generic > kABI stability rules that allow source code annotations to be used > where #ifndef __GENKSYMS__ was previously used. Added support for > rules that can be used to exclude enumerators from versioning. > (patch 16) > > - Per Miroslav's suggestion, added an option to hide structure > members from versioning when they're added to existing alignment > holes, for example. (patch 16) > > - Per Greg's request, added documentation and example macros for the > --stable features, and a couple of test cases. (patches 15, 16, and > 20) > > - Fixed making symtypes files, which need to depend on .o files with > gendwarfksyms. (patch 19) > > - Addressed several other smaller issues that Petr and Masahiro > kindly pointed out during the v2 review. > > v2: https://lore.kernel.org/lkml/20240815173903.4172139-21-samitolvanen@google.com/ > - Per Luis' request, dropped Rust-specific patches and added > gendwarfksyms as an alternative to genksyms for the entire > kernel. > > - Added support for missing DWARF features needed to handle > also non-Rust code. > > - Changed symbol address matching to use the symbol table > information instead of relying on addresses in DWARF. > > - Added __gendwarfksyms_ptr patches to ensure the compiler emits > the necessary type information in DWARF even for symbols that > are defined in other TUs. > > - Refactored debugging output and moved the more verbose output > behind --dump* flags. > > - Added a --symtypes flag for generating a genksyms-style > symtypes output based on Petr's feedback, and refactored > symbol version calculations to be based on symtypes instead > of raw --dump-dies output. > > - Based on feedback from Greg and Petr, added --stable flag and > support for reserved data structure fields and declaration-onl > structures. Also added examples for using these features. > > - Added a GENDWARFKSYMS option and hooked up kbuild support > for both C and assembly code. Note that with gendwarfksyms, > we have to actually build a temporary .o file for calculating > assembly modversions. > > v1: https://lore.kernel.org/lkml/20240617175818.58219-17-samitolvanen@google.com/ > > --- > > Sami Tolvanen (18): > tools: Add gendwarfksyms > gendwarfksyms: Add address matching > gendwarfksyms: Expand base_type > gendwarfksyms: Add a cache for processed DIEs > gendwarfksyms: Expand type modifiers and typedefs > gendwarfksyms: Expand subroutine_type > gendwarfksyms: Expand array_type > gendwarfksyms: Expand structure types > gendwarfksyms: Limit structure expansion > gendwarfksyms: Add die_map debugging > gendwarfksyms: Add symtypes output > gendwarfksyms: Add symbol versioning > gendwarfksyms: Add support for kABI rules > gendwarfksyms: Add support for reserved and ignored fields > gendwarfksyms: Add support for symbol type pointers > export: Add __gendwarfksyms_ptr_ references to exported symbols > kbuild: Add gendwarfksyms as an alternative to genksyms > Documentation/kbuild: Add DWARF module versioning > > Documentation/kbuild/gendwarfksyms.rst | 308 ++++++ > Documentation/kbuild/index.rst | 1 + > include/linux/export.h | 15 + > kernel/module/Kconfig | 31 + > scripts/Makefile | 3 +- > scripts/Makefile.build | 35 +- > scripts/gendwarfksyms/.gitignore | 2 + > scripts/gendwarfksyms/Makefile | 12 + > scripts/gendwarfksyms/cache.c | 51 + > scripts/gendwarfksyms/die.c | 166 +++ > scripts/gendwarfksyms/dwarf.c | 1158 ++++++++++++++++++++ > scripts/gendwarfksyms/examples/kabi.h | 157 +++ > scripts/gendwarfksyms/examples/kabi_ex.c | 30 + > scripts/gendwarfksyms/examples/kabi_ex.h | 263 +++++ > scripts/gendwarfksyms/examples/symbolptr.c | 33 + > scripts/gendwarfksyms/gendwarfksyms.c | 185 ++++ > scripts/gendwarfksyms/gendwarfksyms.h | 301 +++++ > scripts/gendwarfksyms/kabi.c | 333 ++++++ > scripts/gendwarfksyms/symbols.c | 339 ++++++ > scripts/gendwarfksyms/types.c | 477 ++++++++ > 20 files changed, 3893 insertions(+), 7 deletions(-) > create mode 100644 Documentation/kbuild/gendwarfksyms.rst > create mode 100644 scripts/gendwarfksyms/.gitignore > create mode 100644 scripts/gendwarfksyms/Makefile > create mode 100644 scripts/gendwarfksyms/cache.c > create mode 100644 scripts/gendwarfksyms/die.c > create mode 100644 scripts/gendwarfksyms/dwarf.c > create mode 100644 scripts/gendwarfksyms/examples/kabi.h > create mode 100644 scripts/gendwarfksyms/examples/kabi_ex.c > create mode 100644 scripts/gendwarfksyms/examples/kabi_ex.h > create mode 100644 scripts/gendwarfksyms/examples/symbolptr.c > create mode 100644 scripts/gendwarfksyms/gendwarfksyms.c > create mode 100644 scripts/gendwarfksyms/gendwarfksyms.h > create mode 100644 scripts/gendwarfksyms/kabi.c > create mode 100644 scripts/gendwarfksyms/symbols.c > create mode 100644 scripts/gendwarfksyms/types.c > > > base-commit: 3596c721c4348b2a964e43f9296a0c01509ba927 > -- > 2.47.0.371.ga323438b13-goog > As my Acked-by was removed, I'm sorry to say that there is no point for me to provide feedback since it is unwanted. I hope it lands soon, but I also hope the people here who decided that a person's efforts aren't worth recording because they don't personally know them should reflect on this too. It's a good way to keep people from coming into the community for the long term. Regardless, I've copied one of the RHEL kernel folks (Don Zickus) onto this thread to have his team be aware and give feedback. You'll probably want their feedback instead of mine.
On Mon, Nov 25, 2024 at 2:29 PM Neal Gompa <neal@gompa.dev> wrote: > > As my Acked-by was removed, I'm sorry to say that there is no point > for me to provide feedback since it is unwanted. > > I hope it lands soon, but I also hope the people here who decided that > a person's efforts aren't worth recording because they don't > personally know them should reflect on this too. It's a good way to > keep people from coming into the community for the long term. Hopefully this reply helps -- apologies to anyone if I am overstepping. On one side, it is true that Acked-by is typically used by people that is responsible for the code one way or another, because the tag is meant for them to acknowledge they are OK with the change going in, and so I can see the argument that restricting it for that purpose only may help avoid confusion later on reading the log. On the other hand, someone being willing to put their name on a patch is very valuable, whoever they are, and whatever the tag name is. Moreover, it is also true that, Acked-by may be used here in a "as a key user downstream, this looks reasonable and satisfies our needs" sense. Finally, sometimes new tags are invented on the fly because there is no good fit, too. Either way, I don't think anyone wanted to disregard your efforts or to be rude to you in particular, but rather wanted to keep tags usage aligned to how they view them or how they use them in their subsystem. The Tested-by was still wanted, so I doubt their goal was to remove you from the log or to make you feel unwelcomed. Two solutions here that could be OK for both sides: - Would you be OK with another tag name? For instance, "Acked-by-User:" or similar? That may help the maintainer keep the Acked-bys the way they prefer, yet record your own Acked-by in a separate category. - Another idea that the maintainer may accept is an Acked-by with a "# suffix" comment that clarifies the meaning in this particular case, e.g.: Acked-by: Neal Gompa <neal@gompa.dev> # As primary consumer (Fedora Asahi kernel maintainer). Cheers, Miguel
On Mon, Nov 25, 2024 at 4:41 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, Nov 25, 2024 at 2:29 PM Neal Gompa <neal@gompa.dev> wrote: > > > > As my Acked-by was removed, I'm sorry to say that there is no point > > for me to provide feedback since it is unwanted. > > > > I hope it lands soon, but I also hope the people here who decided that > > a person's efforts aren't worth recording because they don't > > personally know them should reflect on this too. It's a good way to > > keep people from coming into the community for the long term. > > Hopefully this reply helps -- apologies to anyone if I am overstepping. > > On one side, it is true that Acked-by is typically used by people that > is responsible for the code one way or another, because the tag is > meant for them to acknowledge they are OK with the change going in, > and so I can see the argument that restricting it for that purpose > only may help avoid confusion later on reading the log. > > On the other hand, someone being willing to put their name on a patch > is very valuable, whoever they are, and whatever the tag name is. > Moreover, it is also true that, Acked-by may be used here in a "as a > key user downstream, this looks reasonable and satisfies our needs" > sense. > > Finally, sometimes new tags are invented on the fly because there is > no good fit, too. > > Either way, I don't think anyone wanted to disregard your efforts or > to be rude to you in particular, but rather wanted to keep tags usage > aligned to how they view them or how they use them in their subsystem. > The Tested-by was still wanted, so I doubt their goal was to remove > you from the log or to make you feel unwelcomed. Thank you for putting this more eloquently than I could, Miguel. Neal, I do appreciate your feedback, and I'm sorry if I didn't make it clear enough in my previous emails. I would very much welcome your Tested-by, or another suitable tag that's acceptable to both you and Masahiro. Sami
On Thu, Nov 21, 2024 at 08:42:21PM +0000, Sami Tolvanen wrote: > If you also want to test the series with actual Rust modules, this > branch adds Matt's latest modversion_info series: The merge window for v6.13 is now open so this is too late for that, so this is all work to be queued up after, so in about 2 weeks or so. Given that, considering this and the extended modversions patches what I don't see is actual selftests to easily test this and extended modversions. Could you guys add tests for this? Since we have automated tests for modules and we now extended it with another new test for kallsyms under the modules directly it should be fairly easy I think to add tests for this. Think about how we can easily grow these tests to ensure we don't break things with future kernel regressions. Luis
On Mon, Nov 25, 2024 at 10:34 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Mon, Nov 25, 2024 at 4:41 PM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > On Mon, Nov 25, 2024 at 2:29 PM Neal Gompa <neal@gompa.dev> wrote: > > > > > > As my Acked-by was removed, I'm sorry to say that there is no point > > > for me to provide feedback since it is unwanted. > > > > > > I hope it lands soon, but I also hope the people here who decided that > > > a person's efforts aren't worth recording because they don't > > > personally know them should reflect on this too. It's a good way to > > > keep people from coming into the community for the long term. > > > > Hopefully this reply helps -- apologies to anyone if I am overstepping. > > > > On one side, it is true that Acked-by is typically used by people that > > is responsible for the code one way or another, because the tag is > > meant for them to acknowledge they are OK with the change going in, > > and so I can see the argument that restricting it for that purpose > > only may help avoid confusion later on reading the log. > > > > On the other hand, someone being willing to put their name on a patch > > is very valuable, whoever they are, and whatever the tag name is. > > Moreover, it is also true that, Acked-by may be used here in a "as a > > key user downstream, this looks reasonable and satisfies our needs" > > sense. > > > > Finally, sometimes new tags are invented on the fly because there is > > no good fit, too. > > > > Either way, I don't think anyone wanted to disregard your efforts or > > to be rude to you in particular, but rather wanted to keep tags usage > > aligned to how they view them or how they use them in their subsystem. > > The Tested-by was still wanted, so I doubt their goal was to remove > > you from the log or to make you feel unwelcomed. > > Thank you for putting this more eloquently than I could, Miguel. Neal, > I do appreciate your feedback, and I'm sorry if I didn't make it clear > enough in my previous emails. I would very much welcome your > Tested-by, or another suitable tag that's acceptable to both you and > Masahiro. > Honestly, I don't think it's worth it if my tag is going to be stripped simply because someone thinks I'm "unqualified". If you want more people participating in these things, doing stuff like that is definitely not the way to do it. It's not like people haven't had a chance to know me or even just look me up to know I'm not just blowing smoke. I definitely feel like I'm being disregarded. :( The sole reason I didn't give a Reviewed-by or Tested-by is that I didn't want to do any integration work to validate it beyond the basics, which would have meant dipping into the Red Hat kernel symbol tracking infrastructure. I don't have time for that right now. If someone else does, they can be my guest. I just don't feel comfortable giving either without *actually* going that far. -- 真実はいつも一つ！/ Always, there's only one truth!
On Tue, Dec 10, 2024 at 9:42 PM Neal Gompa <neal@gompa.dev> wrote: > > On Mon, Nov 25, 2024 at 10:34 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Mon, Nov 25, 2024 at 4:41 PM Miguel Ojeda > > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > > > On Mon, Nov 25, 2024 at 2:29 PM Neal Gompa <neal@gompa.dev> wrote: > > > > > > > > As my Acked-by was removed, I'm sorry to say that there is no point > > > > for me to provide feedback since it is unwanted. > > > > > > > > I hope it lands soon, but I also hope the people here who decided that > > > > a person's efforts aren't worth recording because they don't > > > > personally know them should reflect on this too. It's a good way to > > > > keep people from coming into the community for the long term. > > > > > > Hopefully this reply helps -- apologies to anyone if I am overstepping. > > > > > > On one side, it is true that Acked-by is typically used by people that > > > is responsible for the code one way or another, because the tag is > > > meant for them to acknowledge they are OK with the change going in, > > > and so I can see the argument that restricting it for that purpose > > > only may help avoid confusion later on reading the log. > > > > > > On the other hand, someone being willing to put their name on a patch > > > is very valuable, whoever they are, and whatever the tag name is. > > > Moreover, it is also true that, Acked-by may be used here in a "as a > > > key user downstream, this looks reasonable and satisfies our needs" > > > sense. > > > > > > Finally, sometimes new tags are invented on the fly because there is > > > no good fit, too. > > > > > > Either way, I don't think anyone wanted to disregard your efforts or > > > to be rude to you in particular, but rather wanted to keep tags usage > > > aligned to how they view them or how they use them in their subsystem. > > > The Tested-by was still wanted, so I doubt their goal was to remove > > > you from the log or to make you feel unwelcomed. > > > > Thank you for putting this more eloquently than I could, Miguel. Neal, > > I do appreciate your feedback, and I'm sorry if I didn't make it clear > > enough in my previous emails. I would very much welcome your > > Tested-by, or another suitable tag that's acceptable to both you and > > Masahiro. > > > > Honestly, I don't think it's worth it if my tag is going to be > stripped simply because someone thinks I'm "unqualified". If you want > more people participating in these things, doing stuff like that is > definitely not the way to do it. It's not like people haven't had a > chance to know me or even just look me up to know I'm not just blowing > smoke. I definitely feel like I'm being disregarded. :( > > The sole reason I didn't give a Reviewed-by or Tested-by is that I > didn't want to do any integration work to validate it beyond the > basics, which would have meant dipping into the Red Hat kernel symbol > tracking infrastructure. I don't have time for that right now. If > someone else does, they can be my guest. I just don't feel comfortable > giving either without *actually* going that far. If you provided a Reviewed-by and/or Tested-by tag, they would not be stripped. I would not say you are unqualified in terms of skills or abilities. However, this is not how the Acked-by tag is typically used. As Miguel mentioned, "Acked-by-User" or "Acked-by: # As primary user" could be an option, but I am not sure if they would gain sufficient consensus. Code reviews and tests are always appreciated. The Reviewed-by and Tested-by tags are open to everyone. If you are uncomfortable with a Reviewed-by or Tested-by tag, I cannot think of any other alternatives.
On Tue, Dec 10, 2024 at 11:27:06PM +0900, Masahiro Yamada wrote: > On Tue, Dec 10, 2024 at 9:42 PM Neal Gompa <neal@gompa.dev> wrote: > > > > On Mon, Nov 25, 2024 at 10:34 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > On Mon, Nov 25, 2024 at 4:41 PM Miguel Ojeda > > > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > > > > > On Mon, Nov 25, 2024 at 2:29 PM Neal Gompa <neal@gompa.dev> wrote: > > > > > > > > > > As my Acked-by was removed, I'm sorry to say that there is no point > > > > > for me to provide feedback since it is unwanted. > > > > > > > > > > I hope it lands soon, but I also hope the people here who decided that > > > > > a person's efforts aren't worth recording because they don't > > > > > personally know them should reflect on this too. It's a good way to > > > > > keep people from coming into the community for the long term. > > > > > > > > Hopefully this reply helps -- apologies to anyone if I am overstepping. > > > > > > > > On one side, it is true that Acked-by is typically used by people that > > > > is responsible for the code one way or another, because the tag is > > > > meant for them to acknowledge they are OK with the change going in, > > > > and so I can see the argument that restricting it for that purpose > > > > only may help avoid confusion later on reading the log. > > > > > > > > On the other hand, someone being willing to put their name on a patch > > > > is very valuable, whoever they are, and whatever the tag name is. > > > > Moreover, it is also true that, Acked-by may be used here in a "as a > > > > key user downstream, this looks reasonable and satisfies our needs" > > > > sense. > > > > > > > > Finally, sometimes new tags are invented on the fly because there is > > > > no good fit, too. > > > > > > > > Either way, I don't think anyone wanted to disregard your efforts or > > > > to be rude to you in particular, but rather wanted to keep tags usage > > > > aligned to how they view them or how they use them in their subsystem. > > > > The Tested-by was still wanted, so I doubt their goal was to remove > > > > you from the log or to make you feel unwelcomed. > > > > > > Thank you for putting this more eloquently than I could, Miguel. Neal, > > > I do appreciate your feedback, and I'm sorry if I didn't make it clear > > > enough in my previous emails. I would very much welcome your > > > Tested-by, or another suitable tag that's acceptable to both you and > > > Masahiro. > > > > > > > Honestly, I don't think it's worth it if my tag is going to be > > stripped simply because someone thinks I'm "unqualified". If you want > > more people participating in these things, doing stuff like that is > > definitely not the way to do it. It's not like people haven't had a > > chance to know me or even just look me up to know I'm not just blowing > > smoke. I definitely feel like I'm being disregarded. :( > > > > The sole reason I didn't give a Reviewed-by or Tested-by is that I > > didn't want to do any integration work to validate it beyond the > > basics, which would have meant dipping into the Red Hat kernel symbol > > tracking infrastructure. I don't have time for that right now. If > > someone else does, they can be my guest. I just don't feel comfortable > > giving either without *actually* going that far. > > > If you provided a Reviewed-by and/or Tested-by tag, they would not be stripped. > > I would not say you are unqualified in terms of skills or abilities. > However, this is not how the Acked-by tag is typically used. > As Miguel mentioned, "Acked-by-User" or "Acked-by: # As primary user" > could be an option, but I am not sure if they would gain > sufficient consensus. > > Code reviews and tests are always appreciated. > The Reviewed-by and Tested-by tags are open to everyone. > > If you are uncomfortable with a Reviewed-by or Tested-by tag, > I cannot think of any other alternatives. I think we just need to update docs that Acked-by is a *tool* mechanism to help maintainers in their quest to find sanity in determining if a patch is ready. Clearly, if I don't know a contributor and they send an Acked-by, it does not matter if was Donald Knuth (CS algorithmic lord), or John Grisham (author of intense legal drama books), what really is of value to the maintainer is the the qualitative appreciation over the maintainer's own perception of the value of that Acked-by is going to help the maintainer own's dreadful quest to find sanity in a review process in answering if a patch series is ready. An unknown developer to a maintainer, as qualified as they may be, could provide an Acked-by, but the utlitiarian value is completely subjective to the domain of review and so up to the maintainer's own critique. The value in Acked-by is in helping the maintainer. There is a hidden context of trust of behind the appreciation of maintainer's value of a simple Acked-by. A series of Reviewed-by tags over time on a subsystem can help a maintainer appreciate the value of a future Acked-by by the same developer. In fact the *more utilitrarian appreciation* for a maintainer will be how often said reviewer is helping find *bugs* in previous patch series, not how many Reviewed-by tags or Acked-by tags they use. The act of finding real issues and thinking critically about the big picture over and over of on different patchsets can help a maintainer garner confidence in future Reviewed-by tags from the developer. Helping a maintainer review patchsets and get authors to fix issues until it is mind numbingly clear the patchset is ready is a way to help a maintainer appreciate future Reviewed-by or Acked-by tags from that developer. Since anyone can also provide a Tested-by, it does not mean that there cannot also be subjective association to it as well. A Tested-by a reporter of a complex regression is of most value. A Tested-by a random new developer for the same complex regression may be meaningless to that same maintainer unless the same original reporter also conirms the issues of a regression are fixed. Keeping a tag is not too important as well, we sometimes loose them even for developers engaging daily, what is more important is *helping the maintainer* on their quest to ensure proper state of affairs of the patchset and to remain sane. Luis
Hi, Here's v6 of the DWARF modversions series. The main motivation is modversions support for Rust, which is important for distributions like Android that are about to ship Rust kernel modules. Per Luis' request [1], v2 dropped the Rust specific bits from the series and instead added the feature as an option for the entire kernel to make it easier to evaluate the benefits of this approach, and to get better test coverage. Matt is addressing Rust modversion_info compatibility issues in a separate patch set [2] that depends on this series, and actually allows modversions to be enabled with Rust. Short background: Unlike C, Rust source code doesn't have sufficient information about the final ABI, as the compiler has considerable freedom in adjusting structure layout, for example, which makes using a source code parser like genksyms a non-starter. Based on earlier feedback, this series uses DWARF debugging information for computing versions. DWARF is an established and a relatively stable format, which includes all the necessary ABI details, and adding a CONFIG_DEBUG_INFO dependency for Rust symbol versioning seems like a reasonable trade-off as most distributions already enable it. The first 15 patches add gendwarfksyms, a tool for computing symbol versions from DWARF. When passed a list of exported symbols and object files, the tool generates an expanded type string for each symbol and computes symbol versions. gendwarfksyms is written in C, uses libdw to process DWARF, and zlib for CRC32. Patch 16 ensures that debugging information is present where we need it, patch 17 adds gendwarfksyms as an alternative to genksyms, and the last patch adds documentation. v6 has changes to structure expansion and the kABI stability features based on our backtesting results with previous Android release kernels (see the change log below). It's also based on linux-kbuild/for-next to include symtypes build rule clean-ups from Masahiro [3]. For your convenience, the series is also available here: https://github.com/samitolvanen/linux/commits/gendwarfksyms-v6 If you also want to test the series with actual Rust modules, this branch adds Matt's latest modversion_info series: https://github.com/samitolvanen/linux/commits/rustmodversions-v6 Sami [1] https://lore.kernel.org/lkml/ZnIZEtkkQWEIGf9n@bombadil.infradead.org/ [2] https://lore.kernel.org/linux-modules/20241030-extended-modversions-v8-0-93acdef62ce8@google.com/ [3] https://lore.kernel.org/linux-modules/CAK7LNAR9c+EEsOvPPn4qSq3gAFskYOXVd=dg8O+bKeeC-HMifw@mail.gmail.com/ --- v6: - Dropped pointer expansion limits as this affects version stability when exported symbols are removed. (Patch 9) - Changed local type definitions (in .c files) that are opaque to external users to be treated as declarations even if a definition is available. (Patch 9) - Switched to zlib's CRC32 implementation per Masahiro's suggestion. (Patch 12) - Renamed struct_declonly kABI rule to simply declonly, as it applies also to unions and enums, added a new rule for overriding enumerator values, and refactored the examples. (Patch 13) - Added --stable support for renamed structure members and also added examples. (Patch 14) - Rebased on linux-kbuild/for-next for Masahiro's symtypes build rule clean-ups. (Patch 17) - Updated the documentation reflect --stable changes. (Patch 18) v5: https://lore.kernel.org/lkml/20241030170106.1501763-21-samitolvanen@google.com/ - Rebased on v6.12-rc5. - Fixed an issue with limiting structure expansion, and applied Petr's clean-up. (Patch 10) - Dropped an unnecessary return statement in error path. (Patch 12) - Addressed several other smaller issues Petr brought up. (Patches 13, 14, and 15) - Added a KBUILD_GENDWARFKSYMS_STABLE flag to enable --stable for the entire kernel build. (Patch 18) - Updated documentation to include KBUILD flags. (Patch 19) - Picked up Reviewed-by tags from v4. v4: https://lore.kernel.org/lkml/20241008183823.36676-21-samitolvanen@google.com/ - Rebased on v6.12-rc2, which now includes all the prerequisites. - Dropped unnecessary name_only parameter for symbols.c::for_each and cleaned up error handling. (Patch 3) - Fixed anonymous scope handling to ensure unnamed DIEs don't get names. (Patch 4) - Added non-variant children to variant_type output, and included DW_AT_discr_value attributes for variants. (Patch 9) - Added another symbol pointer test case. (Patch 16) - Picked up (Acked|Reviewed)-by tags from v3. v3: https://lore.kernel.org/lkml/20240923181846.549877-22-samitolvanen@google.com/ - Updated SPX license headers. - Squashed the first two patches in v2 and tried to reduce churn as much as reasonable. - Dropped patch 18 from v2 ("x86/asm-prototypes: Include <asm/ptrace.h>") as it's addressed by a separate patch. - Changed the error handling code to immediately terminate instead of propagating the errors back to main, which cleaned up the code quite a bit. - Switched to the list and hashtable implementations in scripts and dropped the remaining tools/include dependencies. Added a couple missing list macros. (patch 1) - Moved the genksyms CRC32 implementation to scripts/include and dropped the duplicate code. (patches 2 and 14) - Switched from ad-hoc command line parsing to getopt_long (patch 3). - Added structure member and function parameter names to the DIE output to match genksyms behavior, and tweaked the symtypes format to be more parser-friendly in general based on Petr's suggestions. - Replaced the declaration-only struct annotations with more generic kABI stability rules that allow source code annotations to be used where #ifndef __GENKSYMS__ was previously used. Added support for rules that can be used to exclude enumerators from versioning. (patch 16) - Per Miroslav's suggestion, added an option to hide structure members from versioning when they're added to existing alignment holes, for example. (patch 16) - Per Greg's request, added documentation and example macros for the --stable features, and a couple of test cases. (patches 15, 16, and 20) - Fixed making symtypes files, which need to depend on .o files with gendwarfksyms. (patch 19) - Addressed several other smaller issues that Petr and Masahiro kindly pointed out during the v2 review. v2: https://lore.kernel.org/lkml/20240815173903.4172139-21-samitolvanen@google.com/ - Per Luis' request, dropped Rust-specific patches and added gendwarfksyms as an alternative to genksyms for the entire kernel. - Added support for missing DWARF features needed to handle also non-Rust code. - Changed symbol address matching to use the symbol table information instead of relying on addresses in DWARF. - Added __gendwarfksyms_ptr patches to ensure the compiler emits the necessary type information in DWARF even for symbols that are defined in other TUs. - Refactored debugging output and moved the more verbose output behind --dump* flags. - Added a --symtypes flag for generating a genksyms-style symtypes output based on Petr's feedback, and refactored symbol version calculations to be based on symtypes instead of raw --dump-dies output. - Based on feedback from Greg and Petr, added --stable flag and support for reserved data structure fields and declaration-onl structures. Also added examples for using these features. - Added a GENDWARFKSYMS option and hooked up kbuild support for both C and assembly code. Note that with gendwarfksyms, we have to actually build a temporary .o file for calculating assembly modversions. v1: https://lore.kernel.org/lkml/20240617175818.58219-17-samitolvanen@google.com/ --- Sami Tolvanen (18): tools: Add gendwarfksyms gendwarfksyms: Add address matching gendwarfksyms: Expand base_type gendwarfksyms: Add a cache for processed DIEs gendwarfksyms: Expand type modifiers and typedefs gendwarfksyms: Expand subroutine_type gendwarfksyms: Expand array_type gendwarfksyms: Expand structure types gendwarfksyms: Limit structure expansion gendwarfksyms: Add die_map debugging gendwarfksyms: Add symtypes output gendwarfksyms: Add symbol versioning gendwarfksyms: Add support for kABI rules gendwarfksyms: Add support for reserved and ignored fields gendwarfksyms: Add support for symbol type pointers export: Add __gendwarfksyms_ptr_ references to exported symbols kbuild: Add gendwarfksyms as an alternative to genksyms Documentation/kbuild: Add DWARF module versioning Documentation/kbuild/gendwarfksyms.rst | 308 ++++++ Documentation/kbuild/index.rst | 1 + include/linux/export.h | 15 + kernel/module/Kconfig | 31 + scripts/Makefile | 3 +- scripts/Makefile.build | 35 +- scripts/gendwarfksyms/.gitignore | 2 + scripts/gendwarfksyms/Makefile | 12 + scripts/gendwarfksyms/cache.c | 51 + scripts/gendwarfksyms/die.c | 166 +++ scripts/gendwarfksyms/dwarf.c | 1158 ++++++++++++++++++++ scripts/gendwarfksyms/examples/kabi.h | 157 +++ scripts/gendwarfksyms/examples/kabi_ex.c | 30 + scripts/gendwarfksyms/examples/kabi_ex.h | 263 +++++ scripts/gendwarfksyms/examples/symbolptr.c | 33 + scripts/gendwarfksyms/gendwarfksyms.c | 185 ++++ scripts/gendwarfksyms/gendwarfksyms.h | 301 +++++ scripts/gendwarfksyms/kabi.c | 333 ++++++ scripts/gendwarfksyms/symbols.c | 339 ++++++ scripts/gendwarfksyms/types.c | 477 ++++++++ 20 files changed, 3893 insertions(+), 7 deletions(-) create mode 100644 Documentation/kbuild/gendwarfksyms.rst create mode 100644 scripts/gendwarfksyms/.gitignore create mode 100644 scripts/gendwarfksyms/Makefile create mode 100644 scripts/gendwarfksyms/cache.c create mode 100644 scripts/gendwarfksyms/die.c create mode 100644 scripts/gendwarfksyms/dwarf.c create mode 100644 scripts/gendwarfksyms/examples/kabi.h create mode 100644 scripts/gendwarfksyms/examples/kabi_ex.c create mode 100644 scripts/gendwarfksyms/examples/kabi_ex.h create mode 100644 scripts/gendwarfksyms/examples/symbolptr.c create mode 100644 scripts/gendwarfksyms/gendwarfksyms.c create mode 100644 scripts/gendwarfksyms/gendwarfksyms.h create mode 100644 scripts/gendwarfksyms/kabi.c create mode 100644 scripts/gendwarfksyms/symbols.c create mode 100644 scripts/gendwarfksyms/types.c base-commit: 3596c721c4348b2a964e43f9296a0c01509ba927