Re: [PATCH v7 21/25] Kbuild: add Rust support
From: Nick Desaulniers <hidden>
Date: 2022-05-25 22:25:49
Also in:
linux-arm-kernel, linux-kbuild, linux-riscv, linux-um, lkml, rust-for-linux
On Sun, May 22, 2022 at 7:04 PM Miguel Ojeda [off-list ref] wrote:
quoted hunk ↗ jump to hunk
--- a/Makefile +++ b/Makefile@@ -120,6 +120,15 @@ endif export KBUILD_CHECKSRC +# Enable "clippy" (a linter) as part of the Rust compilation. +# +# Use 'make CLIPPY=1' to enable it. +ifeq ("$(origin CLIPPY)", "command line") + KBUILD_CLIPPY := $(CLIPPY) +endif
Is there a reason to not just turn clippy on always? Might be nicer to start off with good practices by default. :^)
quoted hunk ↗ jump to hunk
@@ -1494,7 +1588,8 @@ MRPROPER_FILES += include/config include/generated \ certs/signing_key.pem \ certs/x509.genkey \ vmlinux-gdb.py \ - *.spec + *.spec \ + rust/target.json rust/libmacros.so # clean - Delete most, but leave enough to build external modules #@@ -1519,6 +1614,9 @@ $(mrproper-dirs): mrproper: clean $(mrproper-dirs) $(call cmd,rmfiles) + @find . $(RCS_FIND_IGNORE) \ + \( -name '*.rmeta' \) \ + -type f -print | xargs rm -f
Are there *.rmeta directories that we _don't_ want to remove via `make mrproper`? I'm curious why *.rmeta isn't just part of MRPROPER_FILES?
quoted hunk ↗ jump to hunk
# distclean #@@ -1606,6 +1704,23 @@ help: @echo ' kselftest-merge - Merge all the config dependencies of' @echo ' kselftest to existing .config.' @echo '' + @echo 'Rust targets:' + @echo ' rustavailable - Checks whether the Rust toolchain is' + @echo ' available and, if not, explains why.' + @echo ' rustfmt - Reformat all the Rust code in the kernel' + @echo ' rustfmtcheck - Checks if all the Rust code in the kernel' + @echo ' is formatted, printing a diff otherwise.' + @echo ' rustdoc - Generate Rust documentation' + @echo ' (requires kernel .config)' + @echo ' rusttest - Runs the Rust tests' + @echo ' (requires kernel .config; downloads external repos)' + @echo ' rust-analyzer - Generate rust-project.json rust-analyzer support file' + @echo ' (requires kernel .config)' + @echo ' dir/file.[os] - Build specified target only' + @echo ' dir/file.i - Build macro expanded source, similar to C preprocessing' + @echo ' (run with RUSTFMT=n to skip reformatting if needed)' + @echo ' dir/file.ll - Build the LLVM assembly file'
I don't think we need to repeat dir/* here again for rust. The existing targets listed above (outside this hunk) make sense in both contexts. Does rustc really use .i as a conventional suffix for macro expanded sources? (The C compiler might use the -x flag to override the guess it would make based on the file extension; I'm curious if rustc can ingest .i files or will it warn?)
quoted hunk ↗ jump to hunk
diff --git a/init/Kconfig b/init/Kconfig index ddcbefe535e9..3457cf596588 100644 --- a/init/Kconfig +++ b/init/Kconfig +config RUSTC_VERSION_TEXT + string + depends on RUST + default $(shell,command -v $(RUSTC) >/dev/null 2>&1 && $(RUSTC) --version || echo n) + +config BINDGEN_VERSION_TEXT + string + depends on RUST + default $(shell,command -v $(BINDGEN) >/dev/null 2>&1 && $(BINDGEN) --version || echo n)
Are these two kconfigs used anywhere?
quoted hunk ↗ jump to hunk
diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include index 0496efd6e117..83e850321eb6 100644 --- a/scripts/Kconfig.include +++ b/scripts/Kconfig.include@@ -36,12 +36,12 @@ ld-option = $(success,$(LD) -v $(1)) as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -) # check if $(CC) and $(LD) exist -$(error-if,$(failure,command -v $(CC)),compiler '$(CC)' not found) +$(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found)
Not that it's important to do so, but a couple hunks s/compiler/C compiler/. Those _could_ probably get submitted ahead of this, but not important to do so.
quoted hunk ↗ jump to hunk
diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug index 9f39b0130551..fe87389d52c0 100644 --- a/scripts/Makefile.debug +++ b/scripts/Makefile.debug@@ -1,4 +1,5 @@ DEBUG_CFLAGS := +DEBUG_RUSTFLAGS := ifdef CONFIG_DEBUG_INFO_SPLIT DEBUG_CFLAGS += -gsplit-dwarf@@ -10,6 +11,12 @@ ifndef CONFIG_AS_IS_LLVM KBUILD_AFLAGS += -Wa,-gdwarf-2 endif +ifdef CONFIG_DEBUG_INFO_REDUCED +DEBUG_RUSTFLAGS += -Cdebuginfo=1 +else +DEBUG_RUSTFLAGS += -Cdebuginfo=2 +endif +
How does enabling or disabling debug info work for rustc? I may have missed it, but I was surprised to see no additional flags getting passed to rustc based on CONFIG_DEBUG info.
quoted hunk ↗ jump to hunk
diff --git a/scripts/generate_rust_target.rs b/scripts/generate_rust_target.rs new file mode 100644 index 000000000000..c146a3407183 --- /dev/null +++ b/scripts/generate_rust_target.rs
Ah, that explains the host rust build infra. Bravo! Hard coding the target files was my major concern last I looked at the series. I'm very happy to see it be generated properly from .config! I haven't actually reviewed this yet, but it makes me significantly more confident in the series to see this approach added. Good job whoever wrote this.
quoted hunk ↗ jump to hunk
diff --git a/scripts/is_rust_module.sh b/scripts/is_rust_module.sh new file mode 100755 index 000000000000..277a64d07f22 --- /dev/null +++ b/scripts/is_rust_module.sh@@ -0,0 +1,13 @@ +#!/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. +${NM} "$*" | grep -qE '^[0-9a-fA-F]+ r _R[^[:space:]]+16___IS_RUST_MODULE[^[:space:]]*$'
Does `$(READELF) -p .comment foo.o` print anything about which compiler was used? That seems less brittle IMO. --- Modulo the RUST_OPT_LEVEL_SIMILAR_AS_CHOSEN_FOR_C which I'm not a fan of, this is looking good to me. Masahiro, what are your thoughts on the build system support? -- Thanks, ~Nick Desaulniers