Re: [PATCH] kbuild: add $(CLANG_CFLAGS) to KBUILD_CPPFLAGS

[Nathan Chancellor]

On Sun, Jun 04, 2023 at 01:32:21AM +0900, Masahiro Yamada wrote:
> On Sat, Jun 3, 2023 at 12:25 AM Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
> >
> > On Wed, May 31, 2023 at 02:33:23PM -0700, Nathan Chancellor wrote:
> > > Hi Masahiro,
> > >
> > > On Sun, Apr 09, 2023 at 11:53:57PM +0900, Masahiro Yamada wrote:
> > > > When preprocessing arch/*/kernel/vmlinux.lds.S, the target triple is
> > > > not passed to $(CPP) because we add it only to KBUILD_{C,A}FLAGS.
> > > >
> > > > As a result, the linker script is preprocessed with predefined macros
> > > > for the build host instead of the target.
> > > >
> > > > Assuming you use an x86 build machine, compare the following:
> > > >
> > > >  $ clang -dM -E -x c /dev/null
> > > >  $ clang -dM -E -x c /dev/null -target aarch64-linux-gnu
> > > >
> > > > There is no actual problem presumably because our linker scripts do not
> > > > rely on such predefined macros, but it is better to define correct ones.
> > > >
> > > > Move $(CFLAGS_CFLAGS) to KBUILD_CPPFLAGS, so that all *.c, *.S, *.lds.S
> > > > will be processed with the proper target triple.
> > > >
> > > > Reported-by: Tom Rini <trini@xxxxxxxxxxxx>
> > > > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > > > ---
> > > >
> > > >  scripts/Makefile.clang | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
> > > > index 70b354fa1cb4..93ca059cc3b8 100644
> > > > --- a/scripts/Makefile.clang
> > > > +++ b/scripts/Makefile.clang
> > > > @@ -38,6 +38,5 @@ CLANG_FLAGS       += -Werror=unknown-warning-option
> > > >  CLANG_FLAGS        += -Werror=ignored-optimization-argument
> > > >  CLANG_FLAGS        += -Werror=option-ignored
> > > >  CLANG_FLAGS        += -Werror=unused-command-line-argument
> > > > -KBUILD_CFLAGS      += $(CLANG_FLAGS)
> > > > -KBUILD_AFLAGS      += $(CLANG_FLAGS)
> > > > +KBUILD_CPPFLAGS    += $(CLANG_FLAGS)
> > > >  export CLANG_FLAGS
> > > > --
> > > > 2.37.2
> > > >
> > >
> > > I am doubling back to this change, as the lack of '--target' in
> > > KBUILD_CPPFLAGS is now an active bug with clang-17 due to a new change
> > > that rejects '-mbig-endian' and '-mlittle-endian' when not supported by
> > > the target, which breaks the arm64 vDSO build when preprocessing its
> > > linker script:
> > >
> > >   # Turn on CONFIG_CPU_BIG_ENDIAN in menuconfig
> > >   $ make -skj"$(nproc)" ARCH=arm64 LLVM=1 O=build mrproper virtconfig menuconfig arch/arm64/kernel/vdso/
> > >   ...
> > >   clang: error: unsupported option '-mbig-endian' for target 'x86_64-pc-linux-gnu'
> > >   make[3]: *** [.../scripts/Makefile.build:387: arch/arm64/kernel/vdso/vdso.lds] Error 1
> > >   ...
> > >
> > >   https://github.com/llvm/llvm-project/commit/d81ce04587c006b6731198956c522c93d0df1050
> > >   https://github.com/ClangBuiltLinux/linux/issues/1859
> > >
> > > This change resolves that issue. I was able to figure out why those new
> > > warnings appeared for ARCH=mips, it is the shell invocation for
> > > CHECKFLAGS. The following diff resolves it for me:
> > >
> > > diff --git a/arch/mips/Makefile b/arch/mips/Makefile
> > > index a7a4ee66a9d3..ef7b05ae92ce 100644
> > > --- a/arch/mips/Makefile
> > > +++ b/arch/mips/Makefile
> > > @@ -346,7 +346,7 @@ KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
> > >  KBUILD_LDFLAGS               += -m $(ld-emul)
> > >
> > >  ifdef CONFIG_MIPS
> > > -CHECKFLAGS += $(shell $(CC) $(KBUILD_CFLAGS) -dM -E -x c /dev/null | \
> > > +CHECKFLAGS += $(shell $(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) -dM -E -x c /dev/null | \
> > >       grep -E -vw '__GNUC_(MINOR_|PATCHLEVEL_)?_' | \
> > >       sed -e "s/^\#define /-D'/" -e "s/ /'='/" -e "s/$$/'/" -e 's/\$$/&&/g')
> > >  endif
> > >
> > > I will run this change plus that diff through my build matrix to see if
> > > any other issues pop up. If not, I will respond with some tags and
> > > perhaps this could be taken as a fix for 6.4 so that it could
> > > potentially be backported?
> >
> > I found two more issues lurking in PowerPC. I have attached suggested
> > patches for all the issues I have uncovered to this email, please feel
> > free to use them or do something different if you feel there is a better
> > fix. With those issues resolved in one way or another, consider the
> > original change:
> >
> > Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> > Tested-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> >
> > If it would work better for you, I am more than happy to take over this
> > series as well.
> >
> > Cheers,
> > Nathan
> 
> Thanks. All the three patches look good to me.
> 
> I will apply them, then mine on top.

Thanks, sounds like a good plan!

> A nit:
> 
> The first patch for mips will introduce the warning
> as $(KBUILD_CPPFLAGS) does not contain $(CLANG_FLAGS)
> at this point.
> Then, it will be fixed again when my patch is applied.
> 
> The two must be squashed together to avoid the regression,
> but I think it is OK as-is.

Hmmm, what warning would be introduced? KBUILD_CPPFLAGS is being added
to the CHECKFLAGS invocation, rather than replacing KBUILD_CFLAGS, so
'--target' should always be present (first via KBUILD_CFLAGS then via
KBUILD_CPPFLAGS), right?

> The second patch for powerpc reminded me of
> converting $(CC) to $(LD) for linking vDSO.
> I do not remember why powerpc vDSO still uses $(CC).

Nick tried to make the conversion some time ago but ran into issues with
GNU ld crashing:

https://lore.kernel.org/lkml/20200901222523.1941988-1-ndesaulniers@xxxxxxxxxx/
https://lore.kernel.org/lkml/b2066ccd-2b81-6032-08e3-41105b400f75@xxxxxxxxxx/

It might be easier to do this now that the PowerPC vDSO has been
combined into one directory, but we have not had time to sit down and
actually work on it.

Cheers,
Nathan