The $(CPP) (do only preprocessing) macro is already defined in Makefile.
However POWERPC redefines it and adds $(KBUILD_CFLAGS) which results
in flags duplication. Which is not a big deal by itself except for
the flags which depend on other flags and the compiler checks them
as it parses the command line.
Specifically, scripts/Makefile.build:304 generates ksyms for .S files.
If clang+llvm+sanitizer are enabled, this results in
-emit-llvm-bc -fno-lto -flto -fvisibility=hidden \
-fsanitize=cfi-mfcall -fno-lto ...
in the clang command line and triggers error:
clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed with '-flto'
This removes unnecessary CPP redefinition. Which works fine as in most
place KBUILD_CFLAGS is passed to $CPP except
arch/powerpc/kernel/vdso64/vdso(32|64).lds (and probably some others,
not yet detected). To fix vdso, we do:
1. explicitly add -m(big|little)-endian to $CPP
2. (for clang) add $CLANG_FLAGS to $KBUILD_CPPFLAGS as otherwise clang
silently ignores -m(big|little)-endian if the building platform does not
support big endian (such as x86) so --prefix= is required.
While at this, remove some duplication from CPPFLAGS_vdso(32|64)
as cmd_cpp_lds_S has them anyway. It still puzzles me why we need -C
(preserve comments in the preprocessor output) flag here.
Signed-off-by: Alexey Kardashevskiy <[email protected]>
---
Changes:
v2:
* fix KBUILD_CPPFLAGS
* add CLANG_FLAGS to CPPFLAGS
---
Makefile | 1 +
arch/powerpc/Makefile | 3 ++-
arch/powerpc/kernel/vdso32/Makefile | 2 +-
arch/powerpc/kernel/vdso64/Makefile | 2 +-
4 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 72af8e423f11..13acd2183d55 100644
--- a/Makefile
+++ b/Makefile
@@ -591,6 +591,7 @@ CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
endif
CLANG_FLAGS += -Werror=unknown-warning-option
KBUILD_CFLAGS += $(CLANG_FLAGS)
+KBUILD_CPPFLAGS += $(CLANG_FLAGS)
KBUILD_AFLAGS += $(CLANG_FLAGS)
export CLANG_FLAGS
endif
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 3212d076ac6a..306bfd2797ad 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -76,6 +76,7 @@ endif
ifdef CONFIG_CPU_LITTLE_ENDIAN
KBUILD_CFLAGS += -mlittle-endian
+KBUILD_CPPFLAGS += -mlittle-endian
KBUILD_LDFLAGS += -EL
LDEMULATION := lppc
GNUTARGET := powerpcle
@@ -83,6 +84,7 @@ MULTIPLEWORD := -mno-multiple
KBUILD_CFLAGS_MODULE += $(call cc-option,-mno-save-toc-indirect)
else
KBUILD_CFLAGS += $(call cc-option,-mbig-endian)
+KBUILD_CPPFLAGS += $(call cc-option,-mbig-endian)
KBUILD_LDFLAGS += -EB
LDEMULATION := ppc
GNUTARGET := powerpc
@@ -208,7 +210,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
KBUILD_AFLAGS += $(AFLAGS-y)
KBUILD_CFLAGS += $(call cc-option,-msoft-float)
KBUILD_CFLAGS += -pipe $(CFLAGS-y)
-CPP = $(CC) -E $(KBUILD_CFLAGS)
CHECKFLAGS += -m$(BITS) -D__powerpc__ -D__powerpc$(BITS)__
ifdef CONFIG_CPU_BIG_ENDIAN
diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
index 7d9a6fee0e3d..ea001c6df1fa 100644
--- a/arch/powerpc/kernel/vdso32/Makefile
+++ b/arch/powerpc/kernel/vdso32/Makefile
@@ -44,7 +44,7 @@ asflags-y := -D__VDSO32__ -s
obj-y += vdso32_wrapper.o
targets += vdso32.lds
-CPPFLAGS_vdso32.lds += -P -C -Upowerpc
+CPPFLAGS_vdso32.lds += -C
# link rule for the .so file, .lds has to be first
$(obj)/vdso32.so.dbg: $(src)/vdso32.lds $(obj-vdso32) $(obj)/vgettimeofday.o FORCE
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 2813e3f98db6..07eadba48c7a 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -30,7 +30,7 @@ ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
asflags-y := -D__VDSO64__ -s
targets += vdso64.lds
-CPPFLAGS_vdso64.lds += -P -C -U$(ARCH)
+CPPFLAGS_vdso64.lds += -C
# link rule for the .so file, .lds has to be first
$(obj)/vdso64.so.dbg: $(src)/vdso64.lds $(obj-vdso64) $(obj)/vgettimeofday.o FORCE
--
2.30.2
Hi!
On Tue, May 11, 2021 at 02:48:12PM +1000, Alexey Kardashevskiy wrote:
> --- a/arch/powerpc/kernel/vdso32/Makefile
> +++ b/arch/powerpc/kernel/vdso32/Makefile
> @@ -44,7 +44,7 @@ asflags-y := -D__VDSO32__ -s
>
> obj-y += vdso32_wrapper.o
> targets += vdso32.lds
> -CPPFLAGS_vdso32.lds += -P -C -Upowerpc
> +CPPFLAGS_vdso32.lds += -C
>
> # link rule for the .so file, .lds has to be first
> $(obj)/vdso32.so.dbg: $(src)/vdso32.lds $(obj-vdso32) $(obj)/vgettimeofday.o FORCE
> --- a/arch/powerpc/kernel/vdso64/Makefile
> +++ b/arch/powerpc/kernel/vdso64/Makefile
> @@ -30,7 +30,7 @@ ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
> asflags-y := -D__VDSO64__ -s
>
> targets += vdso64.lds
> -CPPFLAGS_vdso64.lds += -P -C -U$(ARCH)
> +CPPFLAGS_vdso64.lds += -C
>
> # link rule for the .so file, .lds has to be first
> $(obj)/vdso64.so.dbg: $(src)/vdso64.lds $(obj-vdso64) $(obj)/vgettimeofday.o FORCE
Why are you removing -P and -Upowerpc here? "powerpc" is a predefined
macro on powerpc-linux (no underscores or anything, just the bareword).
This is historical, like "unix" and "linux". If you use the C
preprocessor for things that are not C code (like the kernel does here)
you need to undefine these macros, if anything in the files you run
through the preprocessor contains those words, or funny / strange / bad
things will happen. Presumably at some time in the past it did contain
"powerpc" somewhere.
-P is to inhibit line number output. Whatever consumes the
preprocessor output will have to handle line directives if you remove
this flag. Did you check if this will work for everything that uses
$(CPP)?
In any case, please mention the reasoning (and the fact that you are
removing these flags!) in the commit message. Thanks!
Segher
On 5/10/2021 9:48 PM, Alexey Kardashevskiy wrote:
> The $(CPP) (do only preprocessing) macro is already defined in Makefile.
> However POWERPC redefines it and adds $(KBUILD_CFLAGS) which results
> in flags duplication. Which is not a big deal by itself except for
> the flags which depend on other flags and the compiler checks them
> as it parses the command line.
>
> Specifically, scripts/Makefile.build:304 generates ksyms for .S files.
> If clang+llvm+sanitizer are enabled, this results in
>
> -emit-llvm-bc -fno-lto -flto -fvisibility=hidden \
> -fsanitize=cfi-mfcall -fno-lto ...
>
> in the clang command line and triggers error:
>
> clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed with '-flto'
>
> This removes unnecessary CPP redefinition. Which works fine as in most
> place KBUILD_CFLAGS is passed to $CPP except
> arch/powerpc/kernel/vdso64/vdso(32|64).lds (and probably some others,
> not yet detected). To fix vdso, we do:
> 1. explicitly add -m(big|little)-endian to $CPP
> 2. (for clang) add $CLANG_FLAGS to $KBUILD_CPPFLAGS as otherwise clang
> silently ignores -m(big|little)-endian if the building platform does not
> support big endian (such as x86) so --prefix= is required.
>
> While at this, remove some duplication from CPPFLAGS_vdso(32|64)
> as cmd_cpp_lds_S has them anyway. It still puzzles me why we need -C
> (preserve comments in the preprocessor output) flag here.
>
> Signed-off-by: Alexey Kardashevskiy <[email protected]>
> ---
> Changes:
> v2:
> * fix KBUILD_CPPFLAGS
> * add CLANG_FLAGS to CPPFLAGS
> ---
> Makefile | 1 +
> arch/powerpc/Makefile | 3 ++-
> arch/powerpc/kernel/vdso32/Makefile | 2 +-
> arch/powerpc/kernel/vdso64/Makefile | 2 +-
> 4 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 72af8e423f11..13acd2183d55 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -591,6 +591,7 @@ CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
> endif
> CLANG_FLAGS += -Werror=unknown-warning-option
> KBUILD_CFLAGS += $(CLANG_FLAGS)
> +KBUILD_CPPFLAGS += $(CLANG_FLAGS)
This is going to cause flag duplication, which would be nice to avoid. I
do not know if we can get away with just adding $(CLANG_FLAGS) to
KBUILD_CPPFLAGS instead of KBUILD_CFLAGS though. It seems like this
assignment might be better in arch/powerpc/Makefile with the
KBUILD_CPPFLAGS additions there.
Cheers,
Nathan
> KBUILD_AFLAGS += $(CLANG_FLAGS)
> export CLANG_FLAGS
> endif
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 3212d076ac6a..306bfd2797ad 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -76,6 +76,7 @@ endif
>
> ifdef CONFIG_CPU_LITTLE_ENDIAN
> KBUILD_CFLAGS += -mlittle-endian
> +KBUILD_CPPFLAGS += -mlittle-endian
> KBUILD_LDFLAGS += -EL
> LDEMULATION := lppc
> GNUTARGET := powerpcle
> @@ -83,6 +84,7 @@ MULTIPLEWORD := -mno-multiple
> KBUILD_CFLAGS_MODULE += $(call cc-option,-mno-save-toc-indirect)
> else
> KBUILD_CFLAGS += $(call cc-option,-mbig-endian)
> +KBUILD_CPPFLAGS += $(call cc-option,-mbig-endian)
> KBUILD_LDFLAGS += -EB
> LDEMULATION := ppc
> GNUTARGET := powerpc
> @@ -208,7 +210,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH) $(asinstr)
> KBUILD_AFLAGS += $(AFLAGS-y)
> KBUILD_CFLAGS += $(call cc-option,-msoft-float)
> KBUILD_CFLAGS += -pipe $(CFLAGS-y)
> -CPP = $(CC) -E $(KBUILD_CFLAGS)
>
> CHECKFLAGS += -m$(BITS) -D__powerpc__ -D__powerpc$(BITS)__
> ifdef CONFIG_CPU_BIG_ENDIAN
> diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
> index 7d9a6fee0e3d..ea001c6df1fa 100644
> --- a/arch/powerpc/kernel/vdso32/Makefile
> +++ b/arch/powerpc/kernel/vdso32/Makefile
> @@ -44,7 +44,7 @@ asflags-y := -D__VDSO32__ -s
>
> obj-y += vdso32_wrapper.o
> targets += vdso32.lds
> -CPPFLAGS_vdso32.lds += -P -C -Upowerpc
> +CPPFLAGS_vdso32.lds += -C
>
> # link rule for the .so file, .lds has to be first
> $(obj)/vdso32.so.dbg: $(src)/vdso32.lds $(obj-vdso32) $(obj)/vgettimeofday.o FORCE
> diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
> index 2813e3f98db6..07eadba48c7a 100644
> --- a/arch/powerpc/kernel/vdso64/Makefile
> +++ b/arch/powerpc/kernel/vdso64/Makefile
> @@ -30,7 +30,7 @@ ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
> asflags-y := -D__VDSO64__ -s
>
> targets += vdso64.lds
> -CPPFLAGS_vdso64.lds += -P -C -U$(ARCH)
> +CPPFLAGS_vdso64.lds += -C
>
> # link rule for the .so file, .lds has to be first
> $(obj)/vdso64.so.dbg: $(src)/vdso64.lds $(obj-vdso64) $(obj)/vgettimeofday.o FORCE
>
On Tue, May 11, 2021 at 11:30:17PM +1000, Alexey Kardashevskiy wrote:
> >In any case, please mention the reasoning (and the fact that you are
> >removing these flags!) in the commit message. Thanks!
>
> but i did mention this, the last paragraph... they are duplicated.
Oh! I completely missed those few lines. Sorry for that :-(
To compensate a bit:
> It still puzzles me why we need -C
> (preserve comments in the preprocessor output) flag here.
It is so that a human can look at the output and read it. Comments are
very significant to human readers :-)
Segher
On 5/12/21 09:16, Segher Boessenkool wrote:
> On Tue, May 11, 2021 at 11:30:17PM +1000, Alexey Kardashevskiy wrote:
>>> In any case, please mention the reasoning (and the fact that you are
>>> removing these flags!) in the commit message. Thanks!
>>
>> but i did mention this, the last paragraph... they are duplicated.
>
> Oh! I completely missed those few lines. Sorry for that :-(
Well, I probably should have made it a separate patch anyway, I'll
repost separately.
> To compensate a bit:
>
>> It still puzzles me why we need -C
>> (preserve comments in the preprocessor output) flag here.
>
> It is so that a human can look at the output and read it. Comments are
> very significant to human readers :-)
I seriously doubt anyone ever read those :) I suspect this is to pull
all the licenses in one place and do some checking but I did not dig deep.
--
Alexey
On 5/12/21 05:18, Nathan Chancellor wrote:
> On 5/10/2021 9:48 PM, Alexey Kardashevskiy wrote:
>> The $(CPP) (do only preprocessing) macro is already defined in Makefile.
>> However POWERPC redefines it and adds $(KBUILD_CFLAGS) which results
>> in flags duplication. Which is not a big deal by itself except for
>> the flags which depend on other flags and the compiler checks them
>> as it parses the command line.
>>
>> Specifically, scripts/Makefile.build:304 generates ksyms for .S files.
>> If clang+llvm+sanitizer are enabled, this results in
>>
>> -emit-llvm-bc -fno-lto -flto -fvisibility=hidden \
>> -fsanitize=cfi-mfcall -fno-lto ...
>>
>> in the clang command line and triggers error:
>>
>> clang-13: error: invalid argument '-fsanitize=cfi-mfcall' only allowed
>> with '-flto'
>>
>> This removes unnecessary CPP redefinition. Which works fine as in most
>> place KBUILD_CFLAGS is passed to $CPP except
>> arch/powerpc/kernel/vdso64/vdso(32|64).lds (and probably some others,
>> not yet detected). To fix vdso, we do:
>> 1. explicitly add -m(big|little)-endian to $CPP
>> 2. (for clang) add $CLANG_FLAGS to $KBUILD_CPPFLAGS as otherwise clang
>> silently ignores -m(big|little)-endian if the building platform does not
>> support big endian (such as x86) so --prefix= is required.
>>
>> While at this, remove some duplication from CPPFLAGS_vdso(32|64)
>> as cmd_cpp_lds_S has them anyway. It still puzzles me why we need -C
>> (preserve comments in the preprocessor output) flag here.
>>
>> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>> ---
>> Changes:
>> v2:
>> * fix KBUILD_CPPFLAGS
>> * add CLANG_FLAGS to CPPFLAGS
>> ---
>> Makefile | 1 +
>> arch/powerpc/Makefile | 3 ++-
>> arch/powerpc/kernel/vdso32/Makefile | 2 +-
>> arch/powerpc/kernel/vdso64/Makefile | 2 +-
>> 4 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 72af8e423f11..13acd2183d55 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -591,6 +591,7 @@ CLANG_FLAGS +=
>> --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
>> endif
>> CLANG_FLAGS += -Werror=unknown-warning-option
>> KBUILD_CFLAGS += $(CLANG_FLAGS)
>> +KBUILD_CPPFLAGS += $(CLANG_FLAGS)
>
> This is going to cause flag duplication, which would be nice to avoid. I
> do not know if we can get away with just adding $(CLANG_FLAGS) to
> KBUILD_CPPFLAGS instead of KBUILD_CFLAGS though. It seems like this
> assignment might be better in arch/powerpc/Makefile with the
> KBUILD_CPPFLAGS additions there.
It is a fair point about the duplication (which is woooow, I often see
-mbig-endian 3 - three - times) and I think I only need --prefix= there
but this is still exactly the place to do such thing as it potentially
affects all archs supporting both endianness (not many though, yeah).
Thanks,
>
> Cheers,
> Nathan
>
>> KBUILD_AFLAGS += $(CLANG_FLAGS)
>> export CLANG_FLAGS
>> endif
>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> index 3212d076ac6a..306bfd2797ad 100644
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -76,6 +76,7 @@ endif
>> ifdef CONFIG_CPU_LITTLE_ENDIAN
>> KBUILD_CFLAGS += -mlittle-endian
>> +KBUILD_CPPFLAGS += -mlittle-endian
>> KBUILD_LDFLAGS += -EL
>> LDEMULATION := lppc
>> GNUTARGET := powerpcle
>> @@ -83,6 +84,7 @@ MULTIPLEWORD := -mno-multiple
>> KBUILD_CFLAGS_MODULE += $(call cc-option,-mno-save-toc-indirect)
>> else
>> KBUILD_CFLAGS += $(call cc-option,-mbig-endian)
>> +KBUILD_CPPFLAGS += $(call cc-option,-mbig-endian)
>> KBUILD_LDFLAGS += -EB
>> LDEMULATION := ppc
>> GNUTARGET := powerpc
>> @@ -208,7 +210,6 @@ KBUILD_CPPFLAGS += -I $(srctree)/arch/$(ARCH)
>> $(asinstr)
>> KBUILD_AFLAGS += $(AFLAGS-y)
>> KBUILD_CFLAGS += $(call cc-option,-msoft-float)
>> KBUILD_CFLAGS += -pipe $(CFLAGS-y)
>> -CPP = $(CC) -E $(KBUILD_CFLAGS)
>> CHECKFLAGS += -m$(BITS) -D__powerpc__ -D__powerpc$(BITS)__
>> ifdef CONFIG_CPU_BIG_ENDIAN
>> diff --git a/arch/powerpc/kernel/vdso32/Makefile
>> b/arch/powerpc/kernel/vdso32/Makefile
>> index 7d9a6fee0e3d..ea001c6df1fa 100644
>> --- a/arch/powerpc/kernel/vdso32/Makefile
>> +++ b/arch/powerpc/kernel/vdso32/Makefile
>> @@ -44,7 +44,7 @@ asflags-y := -D__VDSO32__ -s
>> obj-y += vdso32_wrapper.o
>> targets += vdso32.lds
>> -CPPFLAGS_vdso32.lds += -P -C -Upowerpc
>> +CPPFLAGS_vdso32.lds += -C
>> # link rule for the .so file, .lds has to be first
>> $(obj)/vdso32.so.dbg: $(src)/vdso32.lds $(obj-vdso32)
>> $(obj)/vgettimeofday.o FORCE
>> diff --git a/arch/powerpc/kernel/vdso64/Makefile
>> b/arch/powerpc/kernel/vdso64/Makefile
>> index 2813e3f98db6..07eadba48c7a 100644
>> --- a/arch/powerpc/kernel/vdso64/Makefile
>> +++ b/arch/powerpc/kernel/vdso64/Makefile
>> @@ -30,7 +30,7 @@ ccflags-y := -shared -fno-common -fno-builtin
>> -nostdlib \
>> asflags-y := -D__VDSO64__ -s
>> targets += vdso64.lds
>> -CPPFLAGS_vdso64.lds += -P -C -U$(ARCH)
>> +CPPFLAGS_vdso64.lds += -C
>> # link rule for the .so file, .lds has to be first
>> $(obj)/vdso64.so.dbg: $(src)/vdso64.lds $(obj-vdso64)
>> $(obj)/vgettimeofday.o FORCE
>>
>
--
Alexey
On Wed, May 12, 2021 at 01:48:53PM +1000, Alexey Kardashevskiy wrote:
> >Oh! I completely missed those few lines. Sorry for that :-(
>
> Well, I probably should have made it a separate patch anyway, I'll
> repost separately.
Thanks.
> >To compensate a bit:
> >
> >>It still puzzles me why we need -C
> >>(preserve comments in the preprocessor output) flag here.
> >
> >It is so that a human can look at the output and read it. Comments are
> >very significant to human readers :-)
>
> I seriously doubt anyone ever read those :)
I am pretty sure whoever wrote it did!
> I suspect this is to pull
> all the licenses in one place and do some checking but I did not dig deep.
I don't see the point in that, but :-)
Segher
On Wed, May 12, 2021 at 7:29 PM Segher Boessenkool
<[email protected]> wrote:
>
> On Wed, May 12, 2021 at 01:48:53PM +1000, Alexey Kardashevskiy wrote:
> > >Oh! I completely missed those few lines. Sorry for that :-(
> >
> > Well, I probably should have made it a separate patch anyway, I'll
> > repost separately.
>
> Thanks.
>
> > >To compensate a bit:
> > >
> > >>It still puzzles me why we need -C
> > >>(preserve comments in the preprocessor output) flag here.
> > >
> > >It is so that a human can look at the output and read it. Comments are
> > >very significant to human readers :-)
> >
> > I seriously doubt anyone ever read those :)
>
> I am pretty sure whoever wrote it did!
Keeping comments in the pre-processed linker scripts
is troublesome.
That is why -C was removed from scripts/Makefile.build
commit 5cb0512c02ecd7e6214e912e4c150f4219ac78e0
Author: Linus Torvalds <[email protected]>
Date: Thu Nov 2 14:10:37 2017 -0700
Kbuild: don't pass "-C" to preprocessor when processing linker scripts
You can entirely remove
CPPFLAGS_vdso32.lds += -P -C -Upowerpc
--
Best Regards
Masahiro Yamada