| Message ID | 20240617175818.58219-17-samitolvanen@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Implement MODVERSIONS for Rust | expand |
On Tue, Jun 18, 2024 at 2:58 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi folks, > > This series implements CONFIG_MODVERSIONS for Rust, an important > feature for distributions like Android that want to ship Rust > kernel modules, and depend on modversions to help ensure module ABI > compatibility. > > There have been earlier proposals [1][2] that would allow Rust > modules to coexist with modversions, but none that actually implement > symbol versioning. 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 improved performance [3], > for example, which makes using a source code parser like genksyms > a non-starter. Based on Matt's suggestion and previous feedback > from maintainers, this series uses DWARF debugging information for > computing versions. DWARF is an established and 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. > > The first 12 patches of this series add a small tool for computing > symbol versions from DWARF, called gendwarfksyms. When passed a list > of exported symbols, the tool generates an expanded type string > for each symbol, and computes symbol CRCs similarly to genksyms. > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > because of the existing support for C host tools that use elfutils > (e.g., objtool). > > Another compatibility issue is fitting the extremely long mangled > Rust symbol names into struct modversion_info, which can only hold > 55-character names (on 64-bit systems). Previous proposals suggested > changing the structure to support longer names, but the conclusion was > that we cannot break userspace tools that parse the version table. > > The next two patches of the series implement support for hashed > symbol names in struct modversion_info, where names longer than 55 > characters are hashed, and the hash is stored in the name field. To > avoid breaking userspace tools, the binary hash is prefixed with a > null-terminated string containing the name of the hash function. While > userspace tools can later be updated to potentially produce more > useful information about the long symbols, this allows them to > continue working in the meantime. > > The final patch allows CONFIG_MODVERSIONS to be selected with Rust, > provided that debugging information is also available. I am surprised at someone who attempts to add another variant of genksyms. I am also surprised at the tool being added under the tools/ directory. At least, we can avoid the latter misfortune, though. A patch attached (on top of your patch set). Such a tool can be compiled in scripts/, or even better in rust/Makefile if it is only used in rust/Makefile. -- Best Regards Masahiro Yamada
On Mon, Jun 17, 2024 at 05:58:19PM +0000, Sami Tolvanen wrote: > Hi folks, > > This series implements CONFIG_MODVERSIONS for Rust, an important > feature for distributions like Android that want to ship Rust > kernel modules, and depend on modversions to help ensure module ABI > compatibility. > > There have been earlier proposals [1][2] that would allow Rust > modules to coexist with modversions, but none that actually implement > symbol versioning. 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 improved performance [3], > for example, which makes using a source code parser like genksyms > a non-starter. Based on Matt's suggestion and previous feedback > from maintainers, this series uses DWARF debugging information for > computing versions. DWARF is an established and 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. > > The first 12 patches of this series add a small tool for computing > symbol versions from DWARF, called gendwarfksyms. When passed a list > of exported symbols, the tool generates an expanded type string > for each symbol, and computes symbol CRCs similarly to genksyms. > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > because of the existing support for C host tools that use elfutils > (e.g., objtool). That's cool, can the C code be switched to also use this? That way we only have one path/code for all of this? thanks, greg k-h
On Wed, Jun 19, 2024 at 1:44 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jun 17, 2024 at 05:58:19PM +0000, Sami Tolvanen wrote: > > Hi folks, > > > > This series implements CONFIG_MODVERSIONS for Rust, an important > > feature for distributions like Android that want to ship Rust > > kernel modules, and depend on modversions to help ensure module ABI > > compatibility. > > > > There have been earlier proposals [1][2] that would allow Rust > > modules to coexist with modversions, but none that actually implement > > symbol versioning. 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 improved performance [3], > > for example, which makes using a source code parser like genksyms > > a non-starter. Based on Matt's suggestion and previous feedback > > from maintainers, this series uses DWARF debugging information for > > computing versions. DWARF is an established and 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. > > > > The first 12 patches of this series add a small tool for computing > > symbol versions from DWARF, called gendwarfksyms. When passed a list > > of exported symbols, the tool generates an expanded type string > > for each symbol, and computes symbol CRCs similarly to genksyms. > > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > > because of the existing support for C host tools that use elfutils > > (e.g., objtool). > > That's cool, can the C code be switched to also use this? That way we > only have one path/code for all of this? As the description says, it requires CONFIG_DEBUG_INFO. We can strip the debug info from the final vmlinux, but I guess the build speed will be even slower than the current genksyms.
On Wed, Jun 19, 2024 at 01:50:36AM +0900, Masahiro Yamada wrote: > On Wed, Jun 19, 2024 at 1:44 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Jun 17, 2024 at 05:58:19PM +0000, Sami Tolvanen wrote: > > > Hi folks, > > > > > > This series implements CONFIG_MODVERSIONS for Rust, an important > > > feature for distributions like Android that want to ship Rust > > > kernel modules, and depend on modversions to help ensure module ABI > > > compatibility. > > > > > > There have been earlier proposals [1][2] that would allow Rust > > > modules to coexist with modversions, but none that actually implement > > > symbol versioning. 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 improved performance [3], > > > for example, which makes using a source code parser like genksyms > > > a non-starter. Based on Matt's suggestion and previous feedback > > > from maintainers, this series uses DWARF debugging information for > > > computing versions. DWARF is an established and 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. > > > > > > The first 12 patches of this series add a small tool for computing > > > symbol versions from DWARF, called gendwarfksyms. When passed a list > > > of exported symbols, the tool generates an expanded type string > > > for each symbol, and computes symbol CRCs similarly to genksyms. > > > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > > > because of the existing support for C host tools that use elfutils > > > (e.g., objtool). > > > > That's cool, can the C code be switched to also use this? That way we > > only have one path/code for all of this? > > > As the description says, it requires CONFIG_DEBUG_INFO. > We can strip the debug info from the final vmlinux, but > I guess the build speed will be even slower than the current genksyms. For people who want genksyms (i.e. distros), don't they normally already enable DEBUG_INFO as well? The problems of genksyms are well known and a pain (I speak from experience), so replacing it with info based on DWARF would be great, I'll gladly trade off the DEBUG_INFO issue for stablilty! thanks, greg k-h
On Wed, Jun 19, 2024 at 2:18 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jun 19, 2024 at 01:50:36AM +0900, Masahiro Yamada wrote: > > On Wed, Jun 19, 2024 at 1:44 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Jun 17, 2024 at 05:58:19PM +0000, Sami Tolvanen wrote: > > > > Hi folks, > > > > > > > > This series implements CONFIG_MODVERSIONS for Rust, an important > > > > feature for distributions like Android that want to ship Rust > > > > kernel modules, and depend on modversions to help ensure module ABI > > > > compatibility. > > > > > > > > There have been earlier proposals [1][2] that would allow Rust > > > > modules to coexist with modversions, but none that actually implement > > > > symbol versioning. 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 improved performance [3], > > > > for example, which makes using a source code parser like genksyms > > > > a non-starter. Based on Matt's suggestion and previous feedback > > > > from maintainers, this series uses DWARF debugging information for > > > > computing versions. DWARF is an established and 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. > > > > > > > > The first 12 patches of this series add a small tool for computing > > > > symbol versions from DWARF, called gendwarfksyms. When passed a list > > > > of exported symbols, the tool generates an expanded type string > > > > for each symbol, and computes symbol CRCs similarly to genksyms. > > > > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > > > > because of the existing support for C host tools that use elfutils > > > > (e.g., objtool). > > > > > > That's cool, can the C code be switched to also use this? That way we > > > only have one path/code for all of this? > > > > > > As the description says, it requires CONFIG_DEBUG_INFO. > > We can strip the debug info from the final vmlinux, but > > I guess the build speed will be even slower than the current genksyms. > > For people who want genksyms (i.e. distros), don't they normally already > enable DEBUG_INFO as well? The problems of genksyms are well known and > a pain (I speak from experience), so replacing it with info based on > DWARF would be great, I'll gladly trade off the DEBUG_INFO issue for > stablilty! > > thanks, > > greg k-h > I do not think gendwarfksyms is a drop-in replacement, because it relies on libelf and libdw, which will not work with LLVM bitcode when CONFIG_LTO_CLANG=y. His "Let's postpone this until final linking" stuff will come back? Then, vmlinux.o is processed to extract the CRC of all symbols? In my benchmark, this tool took 3.84 sec just for processing a single rust/core.o object. I'd love to see how long it will take to process vmlinux.o And this occurs even when a single source file is changed and vmlinux.o is re-linked. -- Best Regards Masahiro Yamada
On Mon, Jun 17, 2024 at 05:58:19PM +0000, Sami Tolvanen wrote: > Hi folks, > > This series implements CONFIG_MODVERSIONS for Rust, an important > feature for distributions like Android that want to ship Rust > kernel modules, and depend on modversions to help ensure module ABI > compatibility. > > There have been earlier proposals [1][2] that would allow Rust > modules to coexist with modversions, but none that actually implement > symbol versioning. 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 improved performance [3], > for example, which makes using a source code parser like genksyms > a non-starter. Based on Matt's suggestion and previous feedback > from maintainers, this series uses DWARF debugging information for > computing versions. DWARF is an established and 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. OK sure. > The first 12 patches of this series add a small tool for computing > symbol versions from DWARF, called gendwarfksyms. When passed a list > of exported symbols, the tool generates an expanded type string > for each symbol, and computes symbol CRCs similarly to genksyms. So this is too word centric Rust, let's think about this generically. We still ahve a symbol limitation even in the C world then, and this solution can solve that problem also for other reasons for *whatever* reason we devise to-come-up-with-in-the-future for augmenting symbols. Today Rust, tomorrow, who knows. > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > because of the existing support for C host tools that use elfutils > (e.g., objtool). I agree with Masahiro, that testing this with vmlinux would be eye opening to what challenges we really have ahead. So, to help with this let's try to re-think about this from another perspective. Yes, old userspace should not break, but you can add yet another option to let you opt-in to a new world order of how these crc are mapped to hashed repersentations of the symbols. This would allow us to: a) Ensure correctness for all users / tools, so that proper plumbing is really done. By considering all symbols you increase your scope of awareness of anything that could really break. b) Remove the "Rust" nature about this c) Rust modules just becomes a *user* of this approach It gets me wondering, given Kris is also working on allowing traces to map symbols to module names, how does this fit into that world [0]? As for a) the reason I'm thinking about having the ability to test a full real kernel and moules with this is, without that, how are you sure you have the full scope of the changes needed? [0] https://lkml.kernel.org/r/20240614171428.968174-3-kris.van.hees@oracle.com Luis
Hi Masahiro, On Wed, Jun 19, 2024 at 01:28:21AM +0900, Masahiro Yamada wrote: > I am surprised at someone who attempts to add another variant of genksyms. The options are rather limited if we want Rust modules that are compatible with modversions. We either come up with a way to version Rust symbols or we bypass version checks for them, which is basically what Matt's earlier patch set did: https://lore.kernel.org/rust-for-linux/20231118025748.2778044-1-mmaurer@google.com/ If there are better solutions, I would be happy to hear them. > I am also surprised at the tool being added under the tools/ directory. I picked this location because that's where objtool lives, and wasn't aware that this was frowned upon. I'm fine with moving the tool elsewhere too. Sami
On Wed, Jun 19, 2024 at 04:03:45AM +0900, Masahiro Yamada wrote: > On Wed, Jun 19, 2024 at 2:18 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Jun 19, 2024 at 01:50:36AM +0900, Masahiro Yamada wrote: > > > On Wed, Jun 19, 2024 at 1:44 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > > > > > That's cool, can the C code be switched to also use this? That way we > > > > only have one path/code for all of this? > > > > > > > > > As the description says, it requires CONFIG_DEBUG_INFO. > > > We can strip the debug info from the final vmlinux, but > > > I guess the build speed will be even slower than the current genksyms. > > > > For people who want genksyms (i.e. distros), don't they normally already > > enable DEBUG_INFO as well? The problems of genksyms are well known and > > a pain (I speak from experience), so replacing it with info based on > > DWARF would be great, I'll gladly trade off the DEBUG_INFO issue for > > stablilty! > > > > thanks, > > > > greg k-h > > > > > > I do not think gendwarfksyms is a drop-in replacement, > because it relies on libelf and libdw, which will not > work with LLVM bitcode when CONFIG_LTO_CLANG=y. > > His "Let's postpone this until final linking" stuff will > come back? > Then, vmlinux.o is processed to extract the CRC > of all symbols? I agree, this won't work with LTO unless we process vmlinux.o. > In my benchmark, this tool took 3.84 sec just for processing > a single rust/core.o object. To be fair, Rust currently exports all globals and core.o has 400 exported symbols as a result. During my brief testing, this tool is faster than genksyms for normal C code. > I'd love to see how long it will take to process vmlinux.o It's obviously going to be quite slow, my defconfig vmlinux.o has 14k exported symbols: Performance counter stats for './tools/gendwarfksyms/gendwarfksyms vmlinux.o': 371,527.67 msec task-clock:u # 1.000 CPUs utilized 0 context-switches:u # 0.000 /sec 0 cpu-migrations:u # 0.000 /sec 231,554 page-faults:u # 623.248 /sec <not supported> cycles:u <not supported> instructions:u <not supported> branches:u <not supported> branch-misses:u 371.686151684 seconds time elapsed 370.534637000 seconds user 0.987825000 seconds sys The tool is currently single-threaded, so if we really want to go this route, it could probably be made a bit faster. > And this occurs even when a single source file is changed > and vmlinux.o is re-linked. I suppose anyone using LTO already knows it won't be a quick rebuild though. Sami
Hi Luis, On Tue, Jun 18, 2024 at 12:42:51PM -0700, Luis Chamberlain wrote: > On Mon, Jun 17, 2024 at 05:58:19PM +0000, Sami Tolvanen wrote: > > The first 12 patches of this series add a small tool for computing > > symbol versions from DWARF, called gendwarfksyms. When passed a list > > of exported symbols, the tool generates an expanded type string > > for each symbol, and computes symbol CRCs similarly to genksyms. > > So this is too word centric Rust, let's think about this generically. > We still ahve a symbol limitation even in the C world then, and this > solution can solve that problem also for other reasons for *whatever* > reason we devise to-come-up-with-in-the-future for augmenting symbols. > Today Rust, tomorrow, who knows. If you're referring to the symbol hashing in the __versions table, that's not specific to Rust. Rust just happens to be the only source of long symbol names right now. > > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > > because of the existing support for C host tools that use elfutils > > (e.g., objtool). > > I agree with Masahiro, that testing this with vmlinux would be eye > opening to what challenges we really have ahead. So, to help with this > let's try to re-think about this from another perspective. > > Yes, old userspace should not break, but you can add yet another option > to let you opt-in to a new world order of how these crc are mapped to > hashed repersentations of the symbols. This would allow us to: We did run into an issue with depmod in our testing, where it needs to be taught about hashed names to avoid 'unknown symbol' warnings. I'm not sure if this is acceptable, so I would like to hear feedback about the viability of the hashing scheme in general. If old userspace can't have any regressions because of long symbol names, I suspect we'll have to go back to omitting long symbols from struct modversion_info and adding a v2 of the __versions table with no symbol name length limitations. Happy to hear your thoughts. > a) Ensure correctness for all users / tools, so that proper plumbing is > really done. By considering all symbols you increase your scope of > awareness of anything that could really break. > > b) Remove the "Rust" nature about this > > c) Rust modules just becomes a *user* of this approach I believe the only Rust nature here is the last patch that enables gendwarfksyms only for Rust. Otherwise, there shouldn't be anything strictly Rust specific. > It gets me wondering, given Kris is also working on allowing traces to > map symbols to module names, how does this fit into that world [0]? AFAIK long symbol names are only a problem for struct modversion_info, which uses a relatively short name buffer, so I'm hoping other efforts won't be affected. > As for a) the reason I'm thinking about having the ability to test a > full real kernel and moules with this is, without that, how are you sure > you have the full scope of the changes needed? Sure, I can look into hooking this up for the entire kernel and seeing what breaks, in addition the issues Masahiro pointed out, of course. Sami
On Tue, Jun 18, 2024 at 02:19:47PM -0700, Sami Tolvanen wrote: > Hi Luis, > > On Tue, Jun 18, 2024 at 12:42:51PM -0700, Luis Chamberlain wrote: > > a) Ensure correctness for all users / tools, so that proper plumbing is > > really done. By considering all symbols you increase your scope of > > awareness of anything that could really break. > > > > b) Remove the "Rust" nature about this > > > > c) Rust modules just becomes a *user* of this approach > > I believe the only Rust nature here is the last patch that enables > gendwarfksyms only for Rust. Otherwise, there shouldn't be anything > strictly Rust specific. Right now the check for length is generic, and assumes a hash may exist without considering that check is moot for non-rust modules. So the inverse is true, but doesn't provide a solution or proper architecture for it. > > It gets me wondering, given Kris is also working on allowing traces to > > map symbols to module names, how does this fit into that world [0]? > > AFAIK long symbol names are only a problem for struct modversion_info, > which uses a relatively short name buffer, so I'm hoping other efforts > won't be affected. We'll see! > > As for a) the reason I'm thinking about having the ability to test a > > full real kernel and moules with this is, without that, how are you sure > > you have the full scope of the changes needed? > > Sure, I can look into hooking this up for the entire kernel and seeing > what breaks, in addition the issues Masahiro pointed out, of course. :) should be fun! I think once its revised as a "generic" strategy and not a Rust one, the proper architecture will be considered. Right now, it just looks like a cheap band aid for Rust. Luis
On 6/17/24 19:58, Sami Tolvanen wrote: > Hi folks, > > This series implements CONFIG_MODVERSIONS for Rust, an important > feature for distributions like Android that want to ship Rust > kernel modules, and depend on modversions to help ensure module ABI > compatibility. Thanks for working on this. Below is some feedback with my (open)SUSE hat on, although it should be quite general. > There have been earlier proposals [1][2] that would allow Rust > modules to coexist with modversions, but none that actually implement > symbol versioning. 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 improved performance [3], > for example, which makes using a source code parser like genksyms > a non-starter. Based on Matt's suggestion and previous feedback > from maintainers, this series uses DWARF debugging information for > computing versions. DWARF is an established and 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. Using the DWARF data makes sense to me. Distribution kernels are normally built with debuginfo because one has to be able to debug them later, unsurprisingly. Besides that, one now typically wants to use BPF together with built-in BTF data (CONFIG_DEBUG_INFO_BTF), which also requires building the kernel with debuginfo. I would however keep in mind that producing all DWARF data has some cost. From a quick test, an x86_64-defconfig build is ~30% slower for me with CONFIG_DEBUG_INFO=y. The current genksyms tool allows to have debuginfo disabled when backporting some patches and consequently have a quicker feedback whether modversions changed. This option would disappear with gendwarfksyms but I think it is acceptable. > > The first 12 patches of this series add a small tool for computing > symbol versions from DWARF, called gendwarfksyms. When passed a list > of exported symbols, the tool generates an expanded type string > for each symbol, and computes symbol CRCs similarly to genksyms. > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > because of the existing support for C host tools that use elfutils > (e.g., objtool). In addition to calculating CRCs of exported symbols, genksyms has other features which I think are important. Firstly, the genksyms tool has a human-readable storage format for input data used in the calculation of symbol CRCs. Setting the make variable KBUILD_SYMTYPES enables dumping this data and storing it in *.symtypes files. When a developer later modifies the kernel and wants to check if some symbols have changed, they can take these files and feed them as *.symref back to genksyms. This allows the tool to provide an actual reason why some symbols have changed, instead of just printing that their CRCs are different. Is there any plan to add the same functionality to gendwarfksyms, or do you envison that people will use libabigail, Symbol-Type Graph, or another tool for making this type of comparison? Secondly, when distributions want to maintain stable kABI, they need to be able to deal with patch backports that add new members to structures. One common approach is to have placeholders in important structures which can be later replaced by the new members as needed. __GENKSYMS__ ifdefs are then used at the C source level to hide these kABI-compatible changes from genksyms. Gendwarfksyms works on the resulting binary and so using such ifdefs wouldn't work. Instead, I suspect that what is required is a mechanism to tell the tool that a given change is ok, probably by allowing to specify some map from the original definition to the new one. Is there a plan to implement something like this, or how could it be addressed? > Another compatibility issue is fitting the extremely long mangled > Rust symbol names into struct modversion_info, which can only hold > 55-character names (on 64-bit systems). Previous proposals suggested > changing the structure to support longer names, but the conclusion was > that we cannot break userspace tools that parse the version table. > > The next two patches of the series implement support for hashed > symbol names in struct modversion_info, where names longer than 55 > characters are hashed, and the hash is stored in the name field. To > avoid breaking userspace tools, the binary hash is prefixed with a > null-terminated string containing the name of the hash function. While > userspace tools can later be updated to potentially produce more > useful information about the long symbols, this allows them to > continue working in the meantime. I think this approach with hashed names is quite complex. I'd personally be also in favor of having a new section with variable-length strings to store the names of imported symbols. As yet another alternative, it should be also possible to refer directly into .symtab/.strtab to avoid duplicating the names, but I suspect it would be non-trivial to implement. Cheers, Petr
Hi Petr, On Wed, Jul 10, 2024 at 7:30 AM Petr Pavlu <petr.pavlu@suse.com> wrote: > > On 6/17/24 19:58, Sami Tolvanen wrote: > > Hi folks, > > > > This series implements CONFIG_MODVERSIONS for Rust, an important > > feature for distributions like Android that want to ship Rust > > kernel modules, and depend on modversions to help ensure module ABI > > compatibility. > > Thanks for working on this. Below is some feedback with my (open)SUSE > hat on, although it should be quite general. Great, thanks for taking a look! > > There have been earlier proposals [1][2] that would allow Rust > > modules to coexist with modversions, but none that actually implement > > symbol versioning. 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 improved performance [3], > > for example, which makes using a source code parser like genksyms > > a non-starter. Based on Matt's suggestion and previous feedback > > from maintainers, this series uses DWARF debugging information for > > computing versions. DWARF is an established and 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. > > Using the DWARF data makes sense to me. Distribution kernels are > normally built with debuginfo because one has to be able to debug them > later, unsurprisingly. Besides that, one now typically wants to use BPF > together with built-in BTF data (CONFIG_DEBUG_INFO_BTF), which also > requires building the kernel with debuginfo. > > I would however keep in mind that producing all DWARF data has some > cost. From a quick test, an x86_64-defconfig build is ~30% slower for me > with CONFIG_DEBUG_INFO=y. The current genksyms tool allows to have > debuginfo disabled when backporting some patches and consequently have > a quicker feedback whether modversions changed. This option would > disappear with gendwarfksyms but I think it is acceptable. Yes, this matches my benchmarks too. Presumably folks who care about build times and are not interested in debugging information or Rust support would prefer genksyms instead. I'm planning to turn this into a Kconfig choice in the next version. > > The first 12 patches of this series add a small tool for computing > > symbol versions from DWARF, called gendwarfksyms. When passed a list > > of exported symbols, the tool generates an expanded type string > > for each symbol, and computes symbol CRCs similarly to genksyms. > > gendwarfksyms is written in C and uses libdw to process DWARF, mainly > > because of the existing support for C host tools that use elfutils > > (e.g., objtool). > > In addition to calculating CRCs of exported symbols, genksyms has other > features which I think are important. > > Firstly, the genksyms tool has a human-readable storage format for input > data used in the calculation of symbol CRCs. Setting the make variable > KBUILD_SYMTYPES enables dumping this data and storing it in *.symtypes > files. > > When a developer later modifies the kernel and wants to check if some > symbols have changed, they can take these files and feed them as > *.symref back to genksyms. This allows the tool to provide an actual > reason why some symbols have changed, instead of just printing that > their CRCs are different. > > Is there any plan to add the same functionality to gendwarfksyms, or do > you envison that people will use libabigail, Symbol-Type Graph, or > another tool for making this type of comparison? gendwarfksyms also uses human-readable input for the CRC calculations, and it prints out the input strings with the --debug option. I plan to hook this up to KBUILD_SYMTYPES in v2. It should be convenient enough to simply compare the pretty-printed output with diff, so I'm not sure if a built-in comparison option is needed. Any other DWARF analysis tool can be used to spot the differences too, as you mentioned. > Secondly, when distributions want to maintain stable kABI, they need to > be able to deal with patch backports that add new members to structures. > One common approach is to have placeholders in important structures > which can be later replaced by the new members as needed. __GENKSYMS__ > ifdefs are then used at the C source level to hide these kABI-compatible > changes from genksyms. > > Gendwarfksyms works on the resulting binary and so using such ifdefs > wouldn't work. Instead, I suspect that what is required is a mechanism > to tell the tool that a given change is ok, probably by allowing to > specify some map from the original definition to the new one. > > Is there a plan to implement something like this, or how could it be > addressed? That's a great question. Here's what Android uses currently to maintain a stable kABI, I assume you're doing something similar? https://android.googlesource.com/kernel/common/+/refs/heads/android15-6.6/include/linux/android_kabi.h 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? Greg, I know you've been dealing with this for a long time, any thoughts? > > Another compatibility issue is fitting the extremely long mangled > > Rust symbol names into struct modversion_info, which can only hold > > 55-character names (on 64-bit systems). Previous proposals suggested > > changing the structure to support longer names, but the conclusion was > > that we cannot break userspace tools that parse the version table. > > > > The next two patches of the series implement support for hashed > > symbol names in struct modversion_info, where names longer than 55 > > characters are hashed, and the hash is stored in the name field. To > > avoid breaking userspace tools, the binary hash is prefixed with a > > null-terminated string containing the name of the hash function. While > > userspace tools can later be updated to potentially produce more > > useful information about the long symbols, this allows them to > > continue working in the meantime. > > I think this approach with hashed names is quite complex. I'd personally > be also in favor of having a new section with variable-length strings to > store the names of imported symbols. As yet another alternative, it > should be also possible to refer directly into .symtab/.strtab to avoid > duplicating the names, but I suspect it would be non-trivial to > implement. Thanks for the feedback. I think for the next version we'll look into reviving the relevant bits of Matt's previous patch series, which implemented a new section that supports variable-length names: https://lore.kernel.org/lkml/CAGSQo03R+HYcX2JJDSm7LA8V370s_3hrbTBc2s51Pp9nxWTz5w@mail.gmail.com/ Sami
On Mon, Jul 15, 2024 at 08:39:59PM +0000, Sami Tolvanen wrote: > 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]; Don't use u8, use u64 please, it makes things simpler :) > After replacement: > > union { > u64 new_member; > struct { > u8 __kabi_reserved_1[8]; > }; > } Note, such a thing would only be for the distros that want it, you can add support for this to the tool, but there is no need for any __kabi_reserved fields in mainline. > Here gendwarfksyms would see the __kabi_reserved prefix and only use > u8 [8] for the CRC calculation. Does this sound reasonable? > > Greg, I know you've been dealing with this for a long time, any thoughts? It's a good start, yes. Also watch out for when structures go from "anonymous" to "fully described" when new #include lines get added to files. The current tooling has issues with that, so we need to use __GENKSYMS__ #ifdef lines in some places to keep crc generation stable. Don't know if dwarf output would be susceptible to the same issues with that or not, but you should check. thanks, greg k-h
On Tue, Jul 16, 2024 at 12:12 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > After replacement: > > > > union { > > u64 new_member; > > struct { > > u8 __kabi_reserved_1[8]; > > }; > > } > > Note, such a thing would only be for the distros that want it, you can > add support for this to the tool, but there is no need for any > __kabi_reserved fields in mainline. Sure. I assume modversions are primarily useful for distros, so I just want to make sure this scheme can handle all the common issues as well. > > Greg, I know you've been dealing with this for a long time, any thoughts? > > It's a good start, yes. Also watch out for when structures go from > "anonymous" to "fully described" when new #include lines get added to > files. The current tooling has issues with that, so we need to use > __GENKSYMS__ #ifdef lines in some places to keep crc generation stable. > Don't know if dwarf output would be susceptible to the same issues with > that or not, but you should check. That's a great point. DWARF output has the exact same problem if a structure declaration later becomes a definition, but I think we can come up with a solution for this issue too in v2. Sami
On 7/15/24 22:39, Sami Tolvanen wrote: > On Wed, Jul 10, 2024 at 7:30 AM Petr Pavlu <petr.pavlu@suse.com> wrote: >> On 6/17/24 19:58, Sami Tolvanen wrote: >>> The first 12 patches of this series add a small tool for computing >>> symbol versions from DWARF, called gendwarfksyms. When passed a list >>> of exported symbols, the tool generates an expanded type string >>> for each symbol, and computes symbol CRCs similarly to genksyms. >>> gendwarfksyms is written in C and uses libdw to process DWARF, mainly >>> because of the existing support for C host tools that use elfutils >>> (e.g., objtool). >> >> In addition to calculating CRCs of exported symbols, genksyms has other >> features which I think are important. >> >> Firstly, the genksyms tool has a human-readable storage format for input >> data used in the calculation of symbol CRCs. Setting the make variable >> KBUILD_SYMTYPES enables dumping this data and storing it in *.symtypes >> files. >> >> When a developer later modifies the kernel and wants to check if some >> symbols have changed, they can take these files and feed them as >> *.symref back to genksyms. This allows the tool to provide an actual >> reason why some symbols have changed, instead of just printing that >> their CRCs are different. >> >> Is there any plan to add the same functionality to gendwarfksyms, or do >> you envison that people will use libabigail, Symbol-Type Graph, or >> another tool for making this type of comparison? > > gendwarfksyms also uses human-readable input for the CRC calculations, > and it prints out the input strings with the --debug option. I plan to > hook this up to KBUILD_SYMTYPES in v2. It should be convenient enough > to simply compare the pretty-printed output with diff, so I'm not sure > if a built-in comparison option is needed. Any other DWARF analysis > tool can be used to spot the differences too, as you mentioned. 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. The second problem is the handling of symtypes files. This data is large and if one wants to store them in a Git repository together with the kernel source, it is advisable to first compress/consolidate it in some way. This is trivial because these files typically contain many duplicates. However, the issue is that to feed the data back to genksyms, they need to be unpacked during each build which can take some time. I think a better approach is to have a tool that can be given a consolidated symtypes file as one input and can compare it with all new symtypes files produced during a kernel build. An example of a tool that takes this approach is the kabi Python script in UEK [1]. 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. 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>' >> Secondly, when distributions want to maintain stable kABI, they need to >> be able to deal with patch backports that add new members to structures. >> One common approach is to have placeholders in important structures >> which can be later replaced by the new members as needed. __GENKSYMS__ >> ifdefs are then used at the C source level to hide these kABI-compatible >> changes from genksyms. >> >> Gendwarfksyms works on the resulting binary and so using such ifdefs >> wouldn't work. Instead, I suspect that what is required is a mechanism >> to tell the tool that a given change is ok, probably by allowing to >> specify some map from the original definition to the new one. >> >> Is there a plan to implement something like this, or how could it be >> addressed? > > That's a great question. Here's what Android uses currently to > maintain a stable kABI, I assume you're doing something similar? Correct, (open)SUSE kernels have placeholders in likely-to-change structs which can be used for new members. Or if no placeholder is present, it might be necessary to place a new member in a gap (padding) in the struct layout. > > https://android.googlesource.com/kernel/common/+/refs/heads/android15-6.6/include/linux/android_kabi.h > > 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. [1] https://github.com/oracle/linux-uek/blob/dbdd7f3611cb03e607e156834497dd2767103530/uek-rpm/tools/kabi Thanks, Petr
Hi Petr, On Mon, Jul 22, 2024 at 8:20 AM Petr Pavlu <petr.pavlu@suse.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
On Friday, July 26, 2024 5:05:22 PM EDT Sami Tolvanen wrote: > Hi Petr, > > On Mon, Jul 22, 2024 at 8:20 AM Petr Pavlu <petr.pavlu@suse.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. > Could you please add myself, Hector, and Janne (along with the Asahi Linux mailing list) to the recipients when you send the v2 patch set? We're also interested in this. :) Thanks in advance!
On 7/26/24 23:05, Sami Tolvanen wrote: > On Mon, Jul 22, 2024 at 8:20 AM Petr Pavlu <petr.pavlu@suse.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 STG is an interesting tool. I've played with it a bit last year. To be frank, I was surprised to see a new tool being proposed by Google to generate modversion CRCs from DWARF instead of potentially extending your STG project for this purpose. I'm not sure if it is something that you folks have considered and evaluated. >> 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. Sure, it might be necessary to extend the symtypes format a bit, for example, by allowing spaces in type names. What other problems do you see? The example I showed preserves the DWARF tag names in type descriptions. Cross-references and the target type names use the s# prefix as they they need to be distinguished from other tokens. >> 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. What I think is needed is the ability to compare an updated kernel with some previous reference and have an output that clearly and accurately shows why CRCs of some symbols changed. The previous reference should be possible to store in Git together with the kernel source. It means it should be ideally some text format and limited in size. This is what distributions that care about stable kABI do in some form currently. This functionality would be needed if some distribution wants to maintain stable Rust kABI (not sure if it is actually feasible), or if the idea is for gendwarfksyms to be a general tool that could replace genksyms. I assume for the sake of argument that this is the case. Gendwarfksyms could implement this functionality on its own, or as discussed, I believe it could provide a symtypes-like dump and a second tool could be used to work with this format and for comparing it. From my point of view, the current --debug format is not suitable for this purpose because its expanded and unstructured form means it is bloated and hard to compare with a previous reference. I'm also not quite yet sold on using separate DWARF tooling, such as libabigail or STG, to actually understand why gendwarfksyms produced a different CRC for some symbol. Using these tools makes sense in the genksyms world, where genksyms operates on the source code level and this additional tooling can only work on debug data. With gendwarfksyms working directly with DWARF data, it doesn't seem appealing to me to first run gendwarfksyms to produce CRCs, compare them with their reference, and if they are different, use a second tool to process the same DWARF data again and with some luck hopefully get an actual answer why the CRCs changed. I'm worried that users might encounter inaccurate answers if the two tools interpret the input data differently. > > 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. Thanks, Petr
Hi Petr, On Thu, Aug 1, 2024 at 4:22 AM Petr Pavlu <petr.pavlu@suse.com> wrote: > > STG is an interesting tool. I've played with it a bit last year. To be > frank, I was surprised to see a new tool being proposed by Google to > generate modversion CRCs from DWARF instead of potentially extending > your STG project for this purpose. I'm not sure if it is something that > you folks have considered and evaluated. Yes. STG is fairly large and written in C++, so it didn't seem like a great candidate for inclusion in the kernel tree, and it's presumably not shipped by most distributions, so it's not really ideal as a dependency either. > Sure, it might be necessary to extend the symtypes format a bit, for > example, by allowing spaces in type names. What other problems do you > see? > > The example I showed preserves the DWARF tag names in type descriptions. > Cross-references and the target type names use the s# prefix as they > they need to be distinguished from other tokens. What I meant is that genksyms output is basically preprocessed C with references scattered in, so if you want something that's fully backwards compatible, that might not be possible. However, if you're happy with just the same database structure and don't care about the exact type description format beyond that, that should be doable, but like you said, still requires extending the format to support more complex type names. > What I think is needed is the ability to compare an updated kernel with > some previous reference and have an output that clearly and accurately > shows why CRCs of some symbols changed. The previous reference should be > possible to store in Git together with the kernel source. It means it > should be ideally some text format and limited in size. This is what > distributions that care about stable kABI do in some form currently. > > This functionality would be needed if some distribution wants to > maintain stable Rust kABI (not sure if it is actually feasible), or if > the idea is for gendwarfksyms to be a general tool that could replace > genksyms. I assume for the sake of argument that this is the case. > > Gendwarfksyms could implement this functionality on its own, or as > discussed, I believe it could provide a symtypes-like dump and a second > tool could be used to work with this format and for comparing it. > > From my point of view, the current --debug format is not suitable for > this purpose because its expanded and unstructured form means it is > bloated and hard to compare with a previous reference. I do see your point, there's a ton of duplication in the --debug output as each symbol expands all the types it uses. > I'm also not quite yet sold on using separate DWARF tooling, such as > libabigail or STG, to actually understand why gendwarfksyms produced > a different CRC for some symbol. Using these tools makes sense in the > genksyms world, where genksyms operates on the source code level and > this additional tooling can only work on debug data. > > With gendwarfksyms working directly with DWARF data, it doesn't seem > appealing to me to first run gendwarfksyms to produce CRCs, compare them > with their reference, and if they are different, use a second tool to > process the same DWARF data again and with some luck hopefully get an > actual answer why the CRCs changed. I'm worried that users might > encounter inaccurate answers if the two tools interpret the input data > differently. I agree, there might be other DWARF changes that we don't care about, but that nevertheless make it harder to figure out what caused the actual CRC to change. My initial thought was that simply running a diff between the --debug output would be sufficient, as it would directly tell you what changed, but I didn't consider that the size of the output dump would be of concern as well. I'll look into adding a symtypes-style output format in the next version. Thanks again for the feedback! Sami