Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
On Wed, May 12, 2021 at 7:29 PM Segher Boessenkool 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 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
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
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
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
On 11 May 2021 21:24:55 Segher Boessenkool wrote: 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)? i don't know about everything for sure but i checked few configs and in all cases (except vdso) $CPP was receiving cflags. 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. Segher
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
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 --- 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 ww, 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
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
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
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
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
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
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 --- 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
Re: [PATCH kernel v2] powerpc/makefile: Do not redefine $(CPP) for preprocessor
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
