[03/13] Makefile: Generate CLANG_FLAGS even in GCC builds

Commit Message

Miguel Ojeda April 14, 2021, 6:45 p.m. UTC

From: Miguel Ojeda <[e-mail]>

To support Rust under GCC-built kernels, we need to save the flags that
would have been passed if the kernel was being compiled with Clang.

The reason is that bindgen -- the tool we use to generate Rust bindings
to the C side of the kernel -- relies on libclang to parse C. Ideally:

  - bindgen would support a GCC backend (requested at [1]),

  - or the Clang driver would be perfectly compatible with GCC,
    including plugins. Unlikely, of course, but perhaps a big
    subset of configs may be possible to guarantee to be kept
    compatible nevertheless.

This is also the reason why GCC builds are very experimental and some
configurations may not work (e.g. GCC_PLUGIN_RANDSTRUCT). However,
we keep GCC builds working (for some example configs) in the CI
to avoid diverging/regressing further, so that we are better prepared
for the future when a solution might become available.

[1] https://github.com/rust-lang/rust-bindgen/issues/1949

Link: https://github.com/Rust-for-Linux/linux/issues/167

Co-developed-by: Alex Gaynor <[e-mail]>
Signed-off-by: Alex Gaynor <[e-mail]>
Co-developed-by: Geoffrey Thomas <[e-mail]>
Signed-off-by: Geoffrey Thomas <[e-mail]>
Co-developed-by: Finn Behrens <[e-mail]>
Signed-off-by: Finn Behrens <[e-mail]>
Co-developed-by: Adam Bratschi-Kaye <[e-mail]>
Signed-off-by: Adam Bratschi-Kaye <[e-mail]>
Co-developed-by: Wedson Almeida Filho <[e-mail]>
Signed-off-by: Wedson Almeida Filho <[e-mail]>
Signed-off-by: Miguel Ojeda <[e-mail]>
---
 Makefile | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Comments

Nathan Chancellor April 14, 2021, 6:59 p.m. UTC | #1

Hi Miguel,

On Wed, Apr 14, 2021 at 08:45:54PM +0200, [e-mail] wrote:
> From: Miguel Ojeda <[e-mail]>
> 
> To support Rust under GCC-built kernels, we need to save the flags that
> would have been passed if the kernel was being compiled with Clang.
> 
> The reason is that bindgen -- the tool we use to generate Rust bindings
> to the C side of the kernel -- relies on libclang to parse C. Ideally:
> 
>   - bindgen would support a GCC backend (requested at [1]),
> 
>   - or the Clang driver would be perfectly compatible with GCC,
>     including plugins. Unlikely, of course, but perhaps a big
>     subset of configs may be possible to guarantee to be kept
>     compatible nevertheless.
> 
> This is also the reason why GCC builds are very experimental and some
> configurations may not work (e.g. GCC_PLUGIN_RANDSTRUCT). However,
> we keep GCC builds working (for some example configs) in the CI
> to avoid diverging/regressing further, so that we are better prepared
> for the future when a solution might become available.
> 
> [1] https://github.com/rust-lang/rust-bindgen/issues/1949
> 
> Link: https://github.com/Rust-for-Linux/linux/issues/167
> 
> Co-developed-by: Alex Gaynor <[e-mail]>
> Signed-off-by: Alex Gaynor <[e-mail]>
> Co-developed-by: Geoffrey Thomas <[e-mail]>
> Signed-off-by: Geoffrey Thomas <[e-mail]>
> Co-developed-by: Finn Behrens <[e-mail]>
> Signed-off-by: Finn Behrens <[e-mail]>
> Co-developed-by: Adam Bratschi-Kaye <[e-mail]>
> Signed-off-by: Adam Bratschi-Kaye <[e-mail]>
> Co-developed-by: Wedson Almeida Filho <[e-mail]>
> Signed-off-by: Wedson Almeida Filho <[e-mail]>
> Signed-off-by: Miguel Ojeda <[e-mail]>
> ---
>  Makefile | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index d4784d181123..9c75354324ed 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -559,26 +559,31 @@ ifdef building_out_of_srctree
>  	{ echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
>  endif
>  
> -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> -# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1 | sed 's/\#//g')
> +TENTATIVE_CLANG_FLAGS := -Werror=unknown-warning-option
>  
> -ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
>  ifneq ($(CROSS_COMPILE),)
> -CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
> +TENTATIVE_CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
>  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))

Shuffling this around will cause this issue (I never saw you CC'd on the
thread).

https://lore.kernel.org/r/f[e-mail]/

Perhaps that patch should be added to this series?

> -CLANG_FLAGS	+= --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
> +TENTATIVE_CLANG_FLAGS	+= --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
>  GCC_TOOLCHAIN	:= $(realpath $(GCC_TOOLCHAIN_DIR)/..)
>  endif
>  ifneq ($(GCC_TOOLCHAIN),)
> -CLANG_FLAGS	+= --gcc-toolchain=$(GCC_TOOLCHAIN)
> +TENTATIVE_CLANG_FLAGS	+= --gcc-toolchain=$(GCC_TOOLCHAIN)
>  endif
>  ifneq ($(LLVM_IAS),1)
> -CLANG_FLAGS	+= -no-integrated-as
> +TENTATIVE_CLANG_FLAGS	+= -no-integrated-as
>  endif
> -CLANG_FLAGS	+= -Werror=unknown-warning-option
> +
> +export TENTATIVE_CLANG_FLAGS
> +
> +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> +# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1 | sed 's/\#//g')
> +
> +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
> +CLANG_FLAGS	+= $(TENTATIVE_CLANG_FLAGS)
>  KBUILD_CFLAGS	+= $(CLANG_FLAGS)
>  KBUILD_AFLAGS	+= $(CLANG_FLAGS)
>  export CLANG_FLAGS
> -- 
> 2.17.1
>

Nick Desaulniers April 14, 2021, 11:46 p.m. UTC | #2

On Wed, Apr 14, 2021 at 11:48 AM <[e-mail]> wrote:
>
> From: Miguel Ojeda <[e-mail]>
>
> To support Rust under GCC-built kernels, we need to save the flags that
> would have been passed if the kernel was being compiled with Clang.
>
> The reason is that bindgen -- the tool we use to generate Rust bindings
> to the C side of the kernel -- relies on libclang to parse C. Ideally:
>
>   - bindgen would support a GCC backend (requested at [1]),
>
>   - or the Clang driver would be perfectly compatible with GCC,
>     including plugins. Unlikely, of course, but perhaps a big
>     subset of configs may be possible to guarantee to be kept
>     compatible nevertheless.
>
> This is also the reason why GCC builds are very experimental and some
> configurations may not work (e.g. GCC_PLUGIN_RANDSTRUCT). However,
> we keep GCC builds working (for some example configs) in the CI
> to avoid diverging/regressing further, so that we are better prepared
> for the future when a solution might become available.
>
> [1] https://github.com/rust-lang/rust-bindgen/issues/1949
>
> Link: https://github.com/Rust-for-Linux/linux/issues/167
>
> Co-developed-by: Alex Gaynor <[e-mail]>
> Signed-off-by: Alex Gaynor <[e-mail]>
> Co-developed-by: Geoffrey Thomas <[e-mail]>
> Signed-off-by: Geoffrey Thomas <[e-mail]>
> Co-developed-by: Finn Behrens <[e-mail]>
> Signed-off-by: Finn Behrens <[e-mail]>
> Co-developed-by: Adam Bratschi-Kaye <[e-mail]>
> Signed-off-by: Adam Bratschi-Kaye <[e-mail]>
> Co-developed-by: Wedson Almeida Filho <[e-mail]>
> Signed-off-by: Wedson Almeida Filho <[e-mail]>
> Signed-off-by: Miguel Ojeda <[e-mail]>
> ---
>  Makefile | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index d4784d181123..9c75354324ed 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -559,26 +559,31 @@ ifdef building_out_of_srctree
>         { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
>  endif
>
> -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> -# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1 | sed 's/\#//g')
> +TENTATIVE_CLANG_FLAGS := -Werror=unknown-warning-option
>
> -ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
>  ifneq ($(CROSS_COMPILE),)
> -CLANG_FLAGS    += --target=$(notdir $(CROSS_COMPILE:%-=%))
> +TENTATIVE_CLANG_FLAGS  += --target=$(notdir $(CROSS_COMPILE:%-=%))
>  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
> -CLANG_FLAGS    += --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
> +TENTATIVE_CLANG_FLAGS  += --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
>  GCC_TOOLCHAIN  := $(realpath $(GCC_TOOLCHAIN_DIR)/..)
>  endif
>  ifneq ($(GCC_TOOLCHAIN),)
> -CLANG_FLAGS    += --gcc-toolchain=$(GCC_TOOLCHAIN)
> +TENTATIVE_CLANG_FLAGS  += --gcc-toolchain=$(GCC_TOOLCHAIN)
>  endif
>  ifneq ($(LLVM_IAS),1)
> -CLANG_FLAGS    += -no-integrated-as
> +TENTATIVE_CLANG_FLAGS  += -no-integrated-as
>  endif
> -CLANG_FLAGS    += -Werror=unknown-warning-option
> +
> +export TENTATIVE_CLANG_FLAGS

I'm ok with this approach, but I'm curious:
If the user made a copy of the CLANG_FLAGS variable and modified its
copy, would TENTATIVE_CLANG_FLAGS even be necessary? IIUC,
TENTATIVE_CLANG_FLAGS is used to filter out certain flags before
passing them to bindgen?

Or, I'm curious whether you even need to rename this variable (or
create a new variable) at all? Might make for a shorter diff if you
just keep the existing identifier (CLANG_FLAGS), but create them
unconditionally (at least not conditional on CC=clang).


> +
> +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> +# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1 | sed 's/\#//g')
> +
> +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
> +CLANG_FLAGS    += $(TENTATIVE_CLANG_FLAGS)
>  KBUILD_CFLAGS  += $(CLANG_FLAGS)
>  KBUILD_AFLAGS  += $(CLANG_FLAGS)
>  export CLANG_FLAGS
> --
> 2.17.1
>


--
Thanks,
~Nick Desaulniers

Miguel Ojeda April 15, 2021, 12:47 a.m. UTC | #3

On Thu, Apr 15, 2021 at 1:47 AM Nick Desaulniers
<[e-mail]> wrote:
>
> I'm ok with this approach, but I'm curious:
> If the user made a copy of the CLANG_FLAGS variable and modified its
> copy, would TENTATIVE_CLANG_FLAGS even be necessary? IIUC,
> TENTATIVE_CLANG_FLAGS is used to filter out certain flags before
> passing them to bindgen?
>
> Or, I'm curious whether you even need to rename this variable (or
> create a new variable) at all? Might make for a shorter diff if you
> just keep the existing identifier (CLANG_FLAGS), but create them
> unconditionally (at least not conditional on CC=clang).

This is only for the GCC builds -- and yeah, it is a hack. I still
need to think a bit more how to do this better; although ultimately
the proper solution is to have `bindgen` use GCC like it does with
`libclang` and avoid this altogether. That way we can ensure the
bindings are correct, support plugins, etc.

Cheers,
Miguel

Miguel Ojeda April 15, 2021, 10:18 a.m. UTC | #4

Hi Nathan,

Sorry, with all the other things I ended up not replying to you before
going to sleep.

On Wed, Apr 14, 2021 at 8:59 PM Nathan Chancellor <[e-mail]> wrote:
>
> Shuffling this around will cause this issue (I never saw you CC'd on the
> thread).
>
> https://lore.kernel.org/r/f[e-mail]/
>
> Perhaps that patch should be added to this series?

Ah, thanks for the pointer! Yeah, I was not aware of that thread. It
is good to find these bits though, that's why we are in linux-next
after all! :)

Cheers,
Miguel

Patch

diff --git a/Makefile b/Makefile
index d4784d181123..9c75354324ed 100644
--- a/Makefile
+++ b/Makefile
@@ -559,26 +559,31 @@  ifdef building_out_of_srctree
 	{ echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
 endif
 
-# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
-# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
-# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
-# and from include/config/auto.conf.cmd to detect the compiler upgrade.
-CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1 | sed 's/\#//g')
+TENTATIVE_CLANG_FLAGS := -Werror=unknown-warning-option
 
-ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
 ifneq ($(CROSS_COMPILE),)
-CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
+TENTATIVE_CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
 GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
-CLANG_FLAGS	+= --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
+TENTATIVE_CLANG_FLAGS	+= --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
 GCC_TOOLCHAIN	:= $(realpath $(GCC_TOOLCHAIN_DIR)/..)
 endif
 ifneq ($(GCC_TOOLCHAIN),)
-CLANG_FLAGS	+= --gcc-toolchain=$(GCC_TOOLCHAIN)
+TENTATIVE_CLANG_FLAGS	+= --gcc-toolchain=$(GCC_TOOLCHAIN)
 endif
 ifneq ($(LLVM_IAS),1)
-CLANG_FLAGS	+= -no-integrated-as
+TENTATIVE_CLANG_FLAGS	+= -no-integrated-as
 endif
-CLANG_FLAGS	+= -Werror=unknown-warning-option
+
+export TENTATIVE_CLANG_FLAGS
+
+# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
+# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
+# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
+# and from include/config/auto.conf.cmd to detect the compiler upgrade.
+CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1 | sed 's/\#//g')
+
+ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
+CLANG_FLAGS	+= $(TENTATIVE_CLANG_FLAGS)
 KBUILD_CFLAGS	+= $(CLANG_FLAGS)
 KBUILD_AFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS