| Message ID | 20230704052136.155445-1-andrea.righi@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btf, scripts: rust: drop is_rust_module.sh | expand |
On 7/4/23 02:21, Andrea Righi wrote: > With commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole") > we are now able to use pahole directly to identify Rust compilation > units (CUs) and exclude them from generating BTF debugging information > (when DEBUG_INFO_BTF is enabled). > > And if pahole doesn't support the --lang-exclude flag, we can't enable > both RUST and DEBUG_INFO_BTF at the same time. > > So, in any case, the script is_rust_module.sh is just redundant and we > can drop it. > > NOTE: we may also be able to drop the "Rust loadable module" mark > inside Rust modules, but it seems safer to keep it for now to make sure > we are not breaking any external tool that may potentially rely on it. Keep it, I think it can be useful with tooling like kmod. > > Signed-off-by: Andrea Righi <andrea.righi@canonical.com> > --- > [...] Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
On Tue, Jul 4, 2023 at 7:21 AM Andrea Righi <andrea.righi@canonical.com> wrote: > > With commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole") > we are now able to use pahole directly to identify Rust compilation > units (CUs) and exclude them from generating BTF debugging information > (when DEBUG_INFO_BTF is enabled). > > And if pahole doesn't support the --lang-exclude flag, we can't enable > both RUST and DEBUG_INFO_BTF at the same time. > > So, in any case, the script is_rust_module.sh is just redundant and we > can drop it. > > NOTE: we may also be able to drop the "Rust loadable module" mark > inside Rust modules, but it seems safer to keep it for now to make sure > we are not breaking any external tool that may potentially rely on it. Just to recall the history of these changes: - The script got added in order to skip the BTF generation in the `BTF [M]` step (under `DEBUG_INFO_BTF_MODULES`, which depends on `DEBUG_INFO_BTF`). - A few months later, it was noticed that C modules couldn't be loaded if Rust was enabled, due to the base BTF info in `vmlinux`. That triggered the eventual addition of `--lang_exclude=` to `pahole`, but meanwhile, we made `DEBUG_INFO_BTF` and `RUST` exclusive. - Now, this patch removes the script because having a newer `pahole` also correctly skips the Rust CUs in the `BTF [M]` steps (i.e. and not just the `vmlinux` one), since we pass `--lang_exclude=` to both cases (`link-vmlinux.sh` and `Makefile.modfinal`), if I understand correctly (the script could, in principle, have been removed even before `pahole` got the new feature, given the exclusivity of the options). If this is all correct, then the patch looks good to me. I am Cc'ing Arnaldo, Martin and the BPF list. If this goes through the Rust tree, I will also pick the older `Reviewed-by`s. Thanks! Cheers, Miguel
On Tue, Jul 11, 2023 at 04:39:27PM +0200, Miguel Ojeda wrote: > On Tue, Jul 4, 2023 at 7:21 AM Andrea Righi <andrea.righi@canonical.com> wrote: > > > > With commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole") > > we are now able to use pahole directly to identify Rust compilation > > units (CUs) and exclude them from generating BTF debugging information > > (when DEBUG_INFO_BTF is enabled). > > > > And if pahole doesn't support the --lang-exclude flag, we can't enable > > both RUST and DEBUG_INFO_BTF at the same time. > > > > So, in any case, the script is_rust_module.sh is just redundant and we > > can drop it. > > > > NOTE: we may also be able to drop the "Rust loadable module" mark > > inside Rust modules, but it seems safer to keep it for now to make sure > > we are not breaking any external tool that may potentially rely on it. > > Just to recall the history of these changes: > > - The script got added in order to skip the BTF generation in the > `BTF [M]` step (under `DEBUG_INFO_BTF_MODULES`, which depends on > `DEBUG_INFO_BTF`). > > - A few months later, it was noticed that C modules couldn't be > loaded if Rust was enabled, due to the base BTF info in `vmlinux`. > That triggered the eventual addition of `--lang_exclude=` to `pahole`, > but meanwhile, we made `DEBUG_INFO_BTF` and `RUST` exclusive. > > - Now, this patch removes the script because having a newer `pahole` > also correctly skips the Rust CUs in the `BTF [M]` steps (i.e. and not > just the `vmlinux` one), since we pass `--lang_exclude=` to both cases > (`link-vmlinux.sh` and `Makefile.modfinal`), if I understand correctly > (the script could, in principle, have been removed even before > `pahole` got the new feature, given the exclusivity of the options). The history looks correct to me. Also, note that, if pahole doesn't support the new `--lang-exclude=`, we have `RUST` depending on `!DEBUG_INFO_BTF`, so we fallback the old "exclusivity" mode between BTF and Rust and, again, the script is not needed. As you correctly say, in principle, we could have removed the script even before the new `pahole`. > > If this is all correct, then the patch looks good to me. I am Cc'ing > Arnaldo, Martin and the BPF list. > > If this goes through the Rust tree, I will also pick the older `Reviewed-by`s. > > Thanks! > > Cheers, > Miguel Thanks, -Andrea
Hi Miguel, Andrea, On Tue, Jul 11, 2023 at 04:39:27PM +0200, Miguel Ojeda wrote: > On Tue, Jul 4, 2023 at 7:21 AM Andrea Righi <andrea.righi@canonical.com> wrote: > > > > With commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole") > > we are now able to use pahole directly to identify Rust compilation > > units (CUs) and exclude them from generating BTF debugging information > > (when DEBUG_INFO_BTF is enabled). > > > > And if pahole doesn't support the --lang-exclude flag, we can't enable > > both RUST and DEBUG_INFO_BTF at the same time. > > > > So, in any case, the script is_rust_module.sh is just redundant and we > > can drop it. > > > > NOTE: we may also be able to drop the "Rust loadable module" mark > > inside Rust modules, but it seems safer to keep it for now to make sure > > we are not breaking any external tool that may potentially rely on it. > > Just to recall the history of these changes: > > - The script got added in order to skip the BTF generation in the > `BTF [M]` step (under `DEBUG_INFO_BTF_MODULES`, which depends on > `DEBUG_INFO_BTF`). > > - A few months later, it was noticed that C modules couldn't be > loaded if Rust was enabled, due to the base BTF info in `vmlinux`. > That triggered the eventual addition of `--lang_exclude=` to `pahole`, > but meanwhile, we made `DEBUG_INFO_BTF` and `RUST` exclusive. > > - Now, this patch removes the script because having a newer `pahole` > also correctly skips the Rust CUs in the `BTF [M]` steps (i.e. and not > just the `vmlinux` one), since we pass `--lang_exclude=` to both cases > (`link-vmlinux.sh` and `Makefile.modfinal`), if I understand correctly > (the script could, in principle, have been removed even before > `pahole` got the new feature, given the exclusivity of the options). > > If this is all correct, then the patch looks good to me. I am Cc'ing > Arnaldo, Martin and the BPF list. I believe I authored the original script. This all sounds right to me. Acked-by: Daniel Xu <dxu@dxuuu.xyz> Thanks, Daniel
On Tue, Jul 4, 2023 at 7:21 AM Andrea Righi <andrea.righi@canonical.com> wrote: > > With commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole") > we are now able to use pahole directly to identify Rust compilation > units (CUs) and exclude them from generating BTF debugging information > (when DEBUG_INFO_BTF is enabled). > > And if pahole doesn't support the --lang-exclude flag, we can't enable > both RUST and DEBUG_INFO_BTF at the same time. > > So, in any case, the script is_rust_module.sh is just redundant and we > can drop it. > > NOTE: we may also be able to drop the "Rust loadable module" mark > inside Rust modules, but it seems safer to keep it for now to make sure > we are not breaking any external tool that may potentially rely on it. > > Signed-off-by: Andrea Righi <andrea.righi@canonical.com> Applied to `rust-next` -- thanks everyone, and Andrea and Daniel for confirming my summary/recollection sounded right in the other message. Cheers, Miguel
diff --git a/rust/macros/module.rs b/rust/macros/module.rs index fb1244f8c2e6..d62d8710d77a 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -199,7 +199,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { /// Used by the printing macros, e.g. [`info!`]. const __LOG_PREFIX: &[u8] = b\"{name}\\0\"; - /// The \"Rust loadable module\" mark, for `scripts/is_rust_module.sh`. + /// The \"Rust loadable module\" mark. // // This may be best done another way later on, e.g. as a new modinfo // key or a new section. For the moment, keep it simple. diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal index fc19f67039bd..b3a6aa8fbe8c 100644 --- a/scripts/Makefile.modfinal +++ b/scripts/Makefile.modfinal @@ -41,8 +41,6 @@ quiet_cmd_btf_ko = BTF [M] $@ cmd_btf_ko = \ if [ ! -f vmlinux ]; then \ printf "Skipping BTF generation for %s due to unavailability of vmlinux\n" $@ 1>&2; \ - elif [ -n "$(CONFIG_RUST)" ] && $(srctree)/scripts/is_rust_module.sh $@; then \ - printf "Skipping BTF generation for %s because it's a Rust module\n" $@ 1>&2; \ else \ LLVM_OBJCOPY="$(OBJCOPY)" $(PAHOLE) -J $(PAHOLE_FLAGS) --btf_base vmlinux $@; \ $(RESOLVE_BTFIDS) -b vmlinux $@; \ diff --git a/scripts/is_rust_module.sh b/scripts/is_rust_module.sh deleted file mode 100755 index 464761a7cf7f..000000000000 --- a/scripts/is_rust_module.sh +++ /dev/null @@ -1,16 +0,0 @@ -#!/bin/sh -# SPDX-License-Identifier: GPL-2.0 -# -# is_rust_module.sh module.ko -# -# Returns `0` if `module.ko` is a Rust module, `1` otherwise. - -set -e - -# Using the `16_` prefix ensures other symbols with the same substring -# are not picked up (even if it would be unlikely). The last part is -# used just in case LLVM decides to use the `.` suffix. -# -# In the future, checking for the `.comment` section may be another -# option, see https://github.com/rust-lang/rust/pull/97550. -${NM} "$*" | grep -qE '^[0-9a-fA-F]+ [Rr] _R[^[:space:]]+16___IS_RUST_MODULE[^[:space:]]*$'
With commit c1177979af9c ("btf, scripts: Exclude Rust CUs with pahole") we are now able to use pahole directly to identify Rust compilation units (CUs) and exclude them from generating BTF debugging information (when DEBUG_INFO_BTF is enabled). And if pahole doesn't support the --lang-exclude flag, we can't enable both RUST and DEBUG_INFO_BTF at the same time. So, in any case, the script is_rust_module.sh is just redundant and we can drop it. NOTE: we may also be able to drop the "Rust loadable module" mark inside Rust modules, but it seems safer to keep it for now to make sure we are not breaking any external tool that may potentially rely on it. Signed-off-by: Andrea Righi <andrea.righi@canonical.com> --- rust/macros/module.rs | 2 +- scripts/Makefile.modfinal | 2 -- scripts/is_rust_module.sh | 16 ---------------- 3 files changed, 1 insertion(+), 19 deletions(-) delete mode 100755 scripts/is_rust_module.sh