* [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT @ 2022-08-31 18:44 Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions Nick Desaulniers ` (4 more replies) 0 siblings, 5 replies; 19+ messages in thread From: Nick Desaulniers @ 2022-08-31 18:44 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Nick Desaulniers Alexey reported that the fraction of unknown filename instances in kallsyms grew from ~0.3% to ~10% recently; Bill and Greg tracked it down to assembler defined symbols, which regressed as a result of: commit b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") In that commit, I allude to restoring debug info for assembler defined symbols in a follow up patch, but it seems I forgot to do so in commit a66049e2cf0e ("Kbuild: make DWARF version a choice") Do so requires a fixup for as-option, which seems to be failing when used in scripts/Makefile.debug. Also includes a fix for DEBUG_INFO_SPLIT while I'm here. Dmitrii reports that this has been broken since gcc-11+ & clang-12+. I'm guessing no one uses this .config option...since no one else has reported it being broken yet... Changes from v1 -> v2: * 5 patches now, rather than 3. * Split change to arch/x86/boot/compressed/Makefile off of first patch, as per Masahiro. * Introduce compiler specific macros, as per Bill, and eradicate cc-ifversion while I'm at it. * Update commit message of final patch to refer to 866ced950bcd. v1: https://lore.kernel.org/llvm/20220826181035.859042-1-ndesaulniers@google.com/ Nick Desaulniers (5): x86/boot/compressed: prefer cc-option for CFLAGS additions Makefile.compiler: Use KBUILD_AFLAGS for as-option Makefile.compiler: replace cc-ifversion with compiler-specific macros Makefile.debug: re-enable debug info for .S files Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT Documentation/kbuild/makefiles.rst | 44 +++++++++++++++------ Makefile | 4 +- arch/x86/boot/compressed/Makefile | 2 +- drivers/gpu/drm/amd/display/dc/dml/Makefile | 12 ++---- scripts/Makefile.compiler | 21 +++++++--- scripts/Makefile.debug | 26 ++++++++---- 6 files changed, 72 insertions(+), 37 deletions(-) -- 2.37.2.672.g94769d06f0-goog ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions 2022-08-31 18:44 [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT Nick Desaulniers @ 2022-08-31 18:44 ` Nick Desaulniers 2022-08-31 19:41 ` Nathan Chancellor 2022-08-31 18:44 ` [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option Nick Desaulniers ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Nick Desaulniers @ 2022-08-31 18:44 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Nick Desaulniers, Arvind Sankar We have an issue where as-option is testing new options with accumulated CFLAGS. This makes it so that we can't use as-option to update AFLAGS since many CFLAGS aren't valid AFLAGS. This is being fixed in a follow up patch. Before doing so, move the assembler test for -Wa,-mrelax-relocations=no from using as-option to cc-option. Cc: Arvind Sankar <nivedita@alum.mit.edu> Cc: x86@kernel.org Link: https://lore.kernel.org/llvm/CAK7LNATcHt7GcXZ=jMszyH=+M_LC9Qr6yeAGRCBbE6xriLxtUQ@mail.gmail.com/ Suggested-by: Masahiro Yamada <masahiroy@kernel.org> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v1 -> v2: * Split off of v1 [1/3]. * Use cc-option to update CFLAGS, as per Masahiro. * Add Masahiro's Suggested-by, Cc Arvind. arch/x86/boot/compressed/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile index 35ce1a64068b..85934204d905 100644 --- a/arch/x86/boot/compressed/Makefile +++ b/arch/x86/boot/compressed/Makefile @@ -49,7 +49,7 @@ KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=) KBUILD_CFLAGS += -fno-asynchronous-unwind-tables KBUILD_CFLAGS += -D__DISABLE_EXPORTS # Disable relocation relaxation in case the link is not PIE. -KBUILD_CFLAGS += $(call as-option,-Wa$(comma)-mrelax-relocations=no) +KBUILD_CFLAGS += $(call cc-option,-Wa$(comma)-mrelax-relocations=no) KBUILD_CFLAGS += -include $(srctree)/include/linux/hidden.h # sev.c indirectly inludes inat-table.h which is generated during -- 2.37.2.672.g94769d06f0-goog ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions 2022-08-31 18:44 ` [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions Nick Desaulniers @ 2022-08-31 19:41 ` Nathan Chancellor 0 siblings, 0 replies; 19+ messages in thread From: Nathan Chancellor @ 2022-08-31 19:41 UTC (permalink / raw) To: Nick Desaulniers Cc: Masahiro Yamada, Michal Marek, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Arvind Sankar On Wed, Aug 31, 2022 at 11:44:04AM -0700, Nick Desaulniers wrote: > We have an issue where as-option is testing new options with accumulated > CFLAGS. This makes it so that we can't use as-option to update AFLAGS > since many CFLAGS aren't valid AFLAGS. This is being fixed in a follow > up patch. Before doing so, move the assembler test for > -Wa,-mrelax-relocations=no from using as-option to cc-option. I think the first couple of sentences might sound clearer without the "we". Maybe something like the following? "as-option tests new options using KBUILD_CFLAGS, which causes problems when using as-option to update KBUILD_AFLAGS because many compiler options are not valid assembler options." > Cc: Arvind Sankar <nivedita@alum.mit.edu> > Cc: x86@kernel.org > Link: https://lore.kernel.org/llvm/CAK7LNATcHt7GcXZ=jMszyH=+M_LC9Qr6yeAGRCBbE6xriLxtUQ@mail.gmail.com/ > Suggested-by: Masahiro Yamada <masahiroy@kernel.org> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Regardless of the commit message: Reviewed-by: Nathan Chancellor <nathan@kernel.org> Additionally, I did verify that '-Wa,-mrelax-relocations=no' still shows up in arch/x86/boot/compressed so: Tested-by: Nathan Chancellor <nathan@kernel.org> > --- > Changes v1 -> v2: > * Split off of v1 [1/3]. > * Use cc-option to update CFLAGS, as per Masahiro. > * Add Masahiro's Suggested-by, Cc Arvind. > > arch/x86/boot/compressed/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile > index 35ce1a64068b..85934204d905 100644 > --- a/arch/x86/boot/compressed/Makefile > +++ b/arch/x86/boot/compressed/Makefile > @@ -49,7 +49,7 @@ KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=) > KBUILD_CFLAGS += -fno-asynchronous-unwind-tables > KBUILD_CFLAGS += -D__DISABLE_EXPORTS > # Disable relocation relaxation in case the link is not PIE. > -KBUILD_CFLAGS += $(call as-option,-Wa$(comma)-mrelax-relocations=no) > +KBUILD_CFLAGS += $(call cc-option,-Wa$(comma)-mrelax-relocations=no) > KBUILD_CFLAGS += -include $(srctree)/include/linux/hidden.h > > # sev.c indirectly inludes inat-table.h which is generated during > -- > 2.37.2.672.g94769d06f0-goog > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-08-31 18:44 [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions Nick Desaulniers @ 2022-08-31 18:44 ` Nick Desaulniers 2022-08-31 19:53 ` Nathan Chancellor 2022-08-31 18:44 ` [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros Nick Desaulniers ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Nick Desaulniers @ 2022-08-31 18:44 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Nick Desaulniers as-instr uses KBUILD_AFLAGS, but as-option uses KBUILD_CFLAGS. This can cause as-option to fail unexpectedly because clang will emit -Werror,-Wunused-command-line-argument for various -m and -f flags for assembler sources. Callers of as-option (and as-instr) likely want to be adding flags to KBUILD_AFLAGS/aflags-y, not KBUILD_CFLAGS/cflags-y. Link: https://github.com/ClangBuiltLinux/linux/issues/1699 Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v1 -> v2: * Split off changes to arch/x86/boot/compressed/Makefile into parent patch, as per Masahiro. scripts/Makefile.compiler | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler index 94d0d40cddb3..d1739f0d3ce3 100644 --- a/scripts/Makefile.compiler +++ b/scripts/Makefile.compiler @@ -29,13 +29,13 @@ try-run = $(shell set -e; \ fi) # as-option -# Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,) +# Usage: aflags-y += $(call as-option,-Wa$(comma)-isa=foo,) as-option = $(call try-run,\ - $(CC) $(KBUILD_CFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2)) + $(CC) $(KBUILD_AFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2)) # as-instr -# Usage: cflags-y += $(call as-instr,instr,option1,option2) +# Usage: aflags-y += $(call as-instr,instr,option1,option2) as-instr = $(call try-run,\ printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3)) -- 2.37.2.672.g94769d06f0-goog ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-08-31 18:44 ` [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option Nick Desaulniers @ 2022-08-31 19:53 ` Nathan Chancellor 2022-09-05 9:09 ` Masahiro Yamada 0 siblings, 1 reply; 19+ messages in thread From: Nathan Chancellor @ 2022-08-31 19:53 UTC (permalink / raw) To: Nick Desaulniers Cc: Masahiro Yamada, Michal Marek, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Wed, Aug 31, 2022 at 11:44:05AM -0700, Nick Desaulniers wrote: > as-instr uses KBUILD_AFLAGS, but as-option uses KBUILD_CFLAGS. This can > cause as-option to fail unexpectedly because clang will emit > -Werror,-Wunused-command-line-argument for various -m and -f flags for > assembler sources. Now that I am looking closer at it, where does that '-Werror' come from? For cc-option, we add it to elevate clang's warnings about unused '-f', '-m', and '-W' flags to errors so that we do not add those flags. However, I do not see '-Werror' in as-option. I am going to assume it came from CONFIG_WERROR, as I believe Android has that turned on by default. I think that is the real problem: without '-Werror', the only error that should come from as-option is when an option isn't supported by the assembler, as clang will still warn but those will not be fatal but with '-Werror', those warnings turn fatal, causing all subsequent as-option calls to fail. Do not get me wrong, I still believe this is the correct fix but I think it would be good to describe exactly under which conditions this is a real issue in case we ever have to revisit this. > Callers of as-option (and as-instr) likely want to be adding flags to > KBUILD_AFLAGS/aflags-y, not KBUILD_CFLAGS/cflags-y. > > Link: https://github.com/ClangBuiltLinux/linux/issues/1699 > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Regardless of changes to the commit message: Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > Changes v1 -> v2: > * Split off changes to arch/x86/boot/compressed/Makefile into parent > patch, as per Masahiro. > > scripts/Makefile.compiler | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler > index 94d0d40cddb3..d1739f0d3ce3 100644 > --- a/scripts/Makefile.compiler > +++ b/scripts/Makefile.compiler > @@ -29,13 +29,13 @@ try-run = $(shell set -e; \ > fi) > > # as-option > -# Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,) > +# Usage: aflags-y += $(call as-option,-Wa$(comma)-isa=foo,) > > as-option = $(call try-run,\ > - $(CC) $(KBUILD_CFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2)) > + $(CC) $(KBUILD_AFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2)) > > # as-instr > -# Usage: cflags-y += $(call as-instr,instr,option1,option2) > +# Usage: aflags-y += $(call as-instr,instr,option1,option2) > > as-instr = $(call try-run,\ > printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3)) > -- > 2.37.2.672.g94769d06f0-goog > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-08-31 19:53 ` Nathan Chancellor @ 2022-09-05 9:09 ` Masahiro Yamada 2022-09-06 16:08 ` Nathan Chancellor 2022-09-07 3:12 ` Nick Desaulniers 0 siblings, 2 replies; 19+ messages in thread From: Masahiro Yamada @ 2022-09-05 9:09 UTC (permalink / raw) To: Nathan Chancellor Cc: Nick Desaulniers, Michal Marek, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Thu, Sep 1, 2022 at 4:53 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Wed, Aug 31, 2022 at 11:44:05AM -0700, Nick Desaulniers wrote: > > as-instr uses KBUILD_AFLAGS, but as-option uses KBUILD_CFLAGS. This can > > cause as-option to fail unexpectedly because clang will emit > > -Werror,-Wunused-command-line-argument for various -m and -f flags for > > assembler sources. > > Now that I am looking closer at it, where does that '-Werror' come from? The related commit is c3f0d0bc5b01ad90c45276952802455750444b4f The previous discussion with Arnd is https://lore.kernel.org/linux-kbuild/20170314213724.3836900-1-arnd@arndb.de/ > For cc-option, we add it to elevate clang's warnings about unused '-f', > '-m', and '-W' flags to errors so that we do not add those flags. > However, I do not see '-Werror' in as-option. I am going to assume it > came from CONFIG_WERROR, as I believe Android has that turned on by > default. CONFIG_WERROR is added to CFLAGS. But, I guess it is more correct to do likewise for others. (https://patchwork.kernel.org/project/linux-kbuild/patch/20220905083619.672091-1-masahiroy@kernel.org/) > I think that is the real problem: without '-Werror', the only > error that should come from as-option is when an option isn't supported > by the assembler, as clang will still warn but those will not be fatal > but with '-Werror', those warnings turn fatal, causing all subsequent > as-option calls to fail. Presumably, it is correct to add -Werror to as-option as well. We have no reason to add it to cc-option, but not to as-option. I also believe '-x assembler' should be changed to '-x assembler-with-cpp'. As I mentioned somewhere before, our assembly code (*.S) is always preprocessed. There is no *.s file in the kernel source tree. So, '-x assembler-with-cpp' matches the real situation. One interesting thing is, clang does not warn [-Wunused-command-line-argument] for *.S files. $ clang -fomit-frame-pointer -c -x assembler /dev/null -o /dev/null clang: warning: argument unused during compilation: '-fomit-frame-pointer' [-Wunused-command-line-argument] $ clang -fomit-frame-pointer -c -x assembler-with-cpp /dev/null -o /dev/null The root cause is we are using '-x assembler', which never happens in the kernel tree. To sum up, the code I think correct is: as-option = $(call try-run,\ $(CC) -Werror $(KBUILD_AFLAGS) $(1) -c -x assembler-with-cpp /dev/null -o "$$TMP",$(1),$(2)) > Do not get me wrong, I still believe this is the correct fix but I think > it would be good to describe exactly under which conditions this is a > real issue in case we ever have to revisit this. > > > Callers of as-option (and as-instr) likely want to be adding flags to > > KBUILD_AFLAGS/aflags-y, not KBUILD_CFLAGS/cflags-y. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1699 > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > Regardless of changes to the commit message: > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > > --- > > Changes v1 -> v2: > > * Split off changes to arch/x86/boot/compressed/Makefile into parent > > patch, as per Masahiro. > > > > scripts/Makefile.compiler | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler > > index 94d0d40cddb3..d1739f0d3ce3 100644 > > --- a/scripts/Makefile.compiler > > +++ b/scripts/Makefile.compiler > > @@ -29,13 +29,13 @@ try-run = $(shell set -e; \ > > fi) > > > > # as-option > > -# Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,) > > +# Usage: aflags-y += $(call as-option,-Wa$(comma)-isa=foo,) > > > > as-option = $(call try-run,\ > > - $(CC) $(KBUILD_CFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2)) > > + $(CC) $(KBUILD_AFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2)) > > > > # as-instr > > -# Usage: cflags-y += $(call as-instr,instr,option1,option2) > > +# Usage: aflags-y += $(call as-instr,instr,option1,option2) > > > > as-instr = $(call try-run,\ > > printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3)) > > -- > > 2.37.2.672.g94769d06f0-goog > > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-09-05 9:09 ` Masahiro Yamada @ 2022-09-06 16:08 ` Nathan Chancellor 2022-09-07 4:14 ` Masahiro Yamada 2022-09-07 3:12 ` Nick Desaulniers 1 sibling, 1 reply; 19+ messages in thread From: Nathan Chancellor @ 2022-09-06 16:08 UTC (permalink / raw) To: Masahiro Yamada Cc: Nick Desaulniers, Michal Marek, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Mon, Sep 05, 2022 at 06:09:28PM +0900, Masahiro Yamada wrote: > On Thu, Sep 1, 2022 at 4:53 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Wed, Aug 31, 2022 at 11:44:05AM -0700, Nick Desaulniers wrote: > > > as-instr uses KBUILD_AFLAGS, but as-option uses KBUILD_CFLAGS. This can > > > cause as-option to fail unexpectedly because clang will emit > > > -Werror,-Wunused-command-line-argument for various -m and -f flags for > > > assembler sources. > > > > Now that I am looking closer at it, where does that '-Werror' come from? > > The related commit is > c3f0d0bc5b01ad90c45276952802455750444b4f > > The previous discussion with Arnd is > https://lore.kernel.org/linux-kbuild/20170314213724.3836900-1-arnd@arndb.de/ Right, although this is for cc-option, not as-option, no? > > For cc-option, we add it to elevate clang's warnings about unused '-f', > > '-m', and '-W' flags to errors so that we do not add those flags. > > However, I do not see '-Werror' in as-option. I am going to assume it > > came from CONFIG_WERROR, as I believe Android has that turned on by > > default. > > CONFIG_WERROR is added to CFLAGS. > But, I guess it is more correct to do likewise for others. > (https://patchwork.kernel.org/project/linux-kbuild/patch/20220905083619.672091-1-masahiroy@kernel.org/) Ack. > > I think that is the real problem: without '-Werror', the only > > error that should come from as-option is when an option isn't supported > > by the assembler, as clang will still warn but those will not be fatal > > but with '-Werror', those warnings turn fatal, causing all subsequent > > as-option calls to fail. > > Presumably, it is correct to add -Werror to as-option as well. > We have no reason to add it to cc-option, but not to as-option. > > I also believe '-x assembler' should be changed to > '-x assembler-with-cpp'. > > As I mentioned somewhere before, our assembly code (*.S) is always > preprocessed. There is no *.s file in the kernel source tree. > > So, '-x assembler-with-cpp' matches the real situation. All good points, I think that is a good fix as well. > One interesting thing is, clang does not warn > [-Wunused-command-line-argument] for *.S files. > > $ clang -fomit-frame-pointer -c -x assembler /dev/null -o /dev/null > clang: warning: argument unused during compilation: > '-fomit-frame-pointer' [-Wunused-command-line-argument] > > $ clang -fomit-frame-pointer -c -x assembler-with-cpp /dev/null -o /dev/null Interesting... I suspect that is likely intentional, as some compiler flags could be used during preprocessing (it's come up before: https://github.com/llvm/llvm-project/issues/55656) but I was not able to figure out exactly where in clang the flags were consumed but Driver.cpp is quite large and I didn't look too hard :) More importantly, it still allows us to catch invalid assembler arguments: $ clang -c -x assembler-with-cpp /dev/null -o /dev/null -Wa,-foo clang-16: error: unsupported argument '-foo' to option '-Wa,' $ clang -c -x assembler-with-cpp /dev/null -o /dev/null -Wa,--noexecstack > The root cause is we are using '-x assembler', which > never happens in the kernel tree. > > To sum up, the code I think correct is: > > as-option = $(call try-run,\ > $(CC) -Werror $(KBUILD_AFLAGS) $(1) -c -x assembler-with-cpp > /dev/null -o "$$TMP",$(1),$(2)) > Agreed. Thank you as always for your feedback! Cheers, Nathan ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-09-06 16:08 ` Nathan Chancellor @ 2022-09-07 4:14 ` Masahiro Yamada 0 siblings, 0 replies; 19+ messages in thread From: Masahiro Yamada @ 2022-09-07 4:14 UTC (permalink / raw) To: Nathan Chancellor Cc: Nick Desaulniers, Michal Marek, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Wed, Sep 7, 2022 at 1:08 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Mon, Sep 05, 2022 at 06:09:28PM +0900, Masahiro Yamada wrote: > > On Thu, Sep 1, 2022 at 4:53 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > On Wed, Aug 31, 2022 at 11:44:05AM -0700, Nick Desaulniers wrote: > > > > as-instr uses KBUILD_AFLAGS, but as-option uses KBUILD_CFLAGS. This can > > > > cause as-option to fail unexpectedly because clang will emit > > > > -Werror,-Wunused-command-line-argument for various -m and -f flags for > > > > assembler sources. > > > > > > Now that I am looking closer at it, where does that '-Werror' come from? > > > > The related commit is > > c3f0d0bc5b01ad90c45276952802455750444b4f > > > > The previous discussion with Arnd is > > https://lore.kernel.org/linux-kbuild/20170314213724.3836900-1-arnd@arndb.de/ > > Right, although this is for cc-option, not as-option, no? Sorry, I misunderstood your comments. My reference is about -Werror in cc-option. It is unrelated to as-option. You are right. Currently, as-option takes KBUILD_CFLAGS instead of KBUILD_AFLAGS. The '-Werror,' of -Werror,-Wunused-command-line-argument presumably came from CONFIG_WERROR. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-09-05 9:09 ` Masahiro Yamada 2022-09-06 16:08 ` Nathan Chancellor @ 2022-09-07 3:12 ` Nick Desaulniers 2022-09-07 4:27 ` Masahiro Yamada 1 sibling, 1 reply; 19+ messages in thread From: Nick Desaulniers @ 2022-09-07 3:12 UTC (permalink / raw) To: Masahiro Yamada Cc: Nathan Chancellor, Michal Marek, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Mon, Sep 5, 2022 at 2:11 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Sep 1, 2022 at 4:53 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Wed, Aug 31, 2022 at 11:44:05AM -0700, Nick Desaulniers wrote: > > > as-instr uses KBUILD_AFLAGS, but as-option uses KBUILD_CFLAGS. This can > > > cause as-option to fail unexpectedly because clang will emit > > > -Werror,-Wunused-command-line-argument for various -m and -f flags for > > > assembler sources. > > > > Now that I am looking closer at it, where does that '-Werror' come from? > > > > > The related commit is > c3f0d0bc5b01ad90c45276952802455750444b4f > > The previous discussion with Arnd is > https://lore.kernel.org/linux-kbuild/20170314213724.3836900-1-arnd@arndb.de/ > > > > > > > For cc-option, we add it to elevate clang's warnings about unused '-f', > > '-m', and '-W' flags to errors so that we do not add those flags. > > However, I do not see '-Werror' in as-option. I am going to assume it > > came from CONFIG_WERROR, as I believe Android has that turned on by > > default. > > > CONFIG_WERROR is added to CFLAGS. > But, I guess it is more correct to do likewise for others. > (https://patchwork.kernel.org/project/linux-kbuild/patch/20220905083619.672091-1-masahiroy@kernel.org/) > > > > > I think that is the real problem: without '-Werror', the only > > error that should come from as-option is when an option isn't supported > > by the assembler, as clang will still warn but those will not be fatal > > but with '-Werror', those warnings turn fatal, causing all subsequent > > as-option calls to fail. > > > > Presumably, it is correct to add -Werror to as-option as well. > We have no reason to add it to cc-option, but not to as-option. > > > > > I also believe '-x assembler' should be changed to > '-x assembler-with-cpp'. > > > As I mentioned somewhere before, our assembly code (*.S) is always > preprocessed. There is no *.s file in the kernel source tree. > > > So, '-x assembler-with-cpp' matches the real situation. Should I do this for as-instr then as well? In the same patch? > > > One interesting thing is, clang does not warn > [-Wunused-command-line-argument] for *.S files. > > > > > $ clang -fomit-frame-pointer -c -x assembler /dev/null -o /dev/null > clang: warning: argument unused during compilation: > '-fomit-frame-pointer' [-Wunused-command-line-argument] > > $ clang -fomit-frame-pointer -c -x assembler-with-cpp /dev/null -o /dev/null > > > > The root cause is we are using '-x assembler', which > never happens in the kernel tree. > > > > > To sum up, the code I think correct is: > > > as-option = $(call try-run,\ > $(CC) -Werror $(KBUILD_AFLAGS) $(1) -c -x assembler-with-cpp > /dev/null -o "$$TMP",$(1),$(2)) Does your recent patch affect this? https://lore.kernel.org/linux-kbuild/20220905083619.672091-1-masahiroy@kernel.org/ If so, then I should not add -Werror as you suggest above? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option 2022-09-07 3:12 ` Nick Desaulniers @ 2022-09-07 4:27 ` Masahiro Yamada 0 siblings, 0 replies; 19+ messages in thread From: Masahiro Yamada @ 2022-09-07 4:27 UTC (permalink / raw) To: Nick Desaulniers Cc: Nathan Chancellor, Michal Marek, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Wed, Sep 7, 2022 at 12:12 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > > > > > I also believe '-x assembler' should be changed to > > '-x assembler-with-cpp'. > > > > > > As I mentioned somewhere before, our assembly code (*.S) is always > > preprocessed. There is no *.s file in the kernel source tree. > > > > > > So, '-x assembler-with-cpp' matches the real situation. > > Should I do this for as-instr then as well? In the same patch? Probably we should fix as-instr in the same way. You can do it in the same patch, or in a separate one. It is up to you. > > > > > > One interesting thing is, clang does not warn > > [-Wunused-command-line-argument] for *.S files. > > > > > > > > > > $ clang -fomit-frame-pointer -c -x assembler /dev/null -o /dev/null > > clang: warning: argument unused during compilation: > > '-fomit-frame-pointer' [-Wunused-command-line-argument] > > > > $ clang -fomit-frame-pointer -c -x assembler-with-cpp /dev/null -o /dev/null > > > > > > > > The root cause is we are using '-x assembler', which > > never happens in the kernel tree. > > > > > > > > > > To sum up, the code I think correct is: > > > > > > as-option = $(call try-run,\ > > $(CC) -Werror $(KBUILD_AFLAGS) $(1) -c -x assembler-with-cpp > > /dev/null -o "$$TMP",$(1),$(2)) > > Does your recent patch affect this? > https://lore.kernel.org/linux-kbuild/20220905083619.672091-1-masahiroy@kernel.org/ No, I do not think so. > If so, then I should not add -Werror as you suggest above? I think we should always add -Werror to as-option. as-option checks the command line options with /dev/null as the source input (that is, source input is always valid). If as-option results in a warning, that option will sprinkle the same warning for all *.S files in the source tree. So, any warning in as-option should be considered as an error. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros 2022-08-31 18:44 [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option Nick Desaulniers @ 2022-08-31 18:44 ` Nick Desaulniers 2022-08-31 20:08 ` Nathan Chancellor 2022-09-05 6:44 ` Masahiro Yamada 2022-08-31 18:44 ` [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 5/5] Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT Nick Desaulniers 4 siblings, 2 replies; 19+ messages in thread From: Nick Desaulniers @ 2022-08-31 18:44 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Nick Desaulniers, Jonathan Corbet, linux-doc, amd-gfx, dri-devel cc-ifversion is GCC specific. Replace it with compiler specific variants. Update the users of cc-ifversion to use these new macros. Provide a helper for checking compiler versions for GCC and Clang simultaneously, that will be used in a follow up patch. Cc: Jonathan Corbet <corbet@lwn.net> Cc: linux-doc@vger.kernel.org Cc: amd-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Link: https://github.com/ClangBuiltLinux/linux/issues/350 Link: https://lore.kernel.org/llvm/CAGG=3QWSAUakO42kubrCap8fp-gm1ERJJAYXTnP1iHk_wrH=BQ@mail.gmail.com/ Suggested-by: Bill Wendling <morbo@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v1 -> v2: * New patch. Documentation/kbuild/makefiles.rst | 44 +++++++++++++++------ Makefile | 4 +- drivers/gpu/drm/amd/display/dc/dml/Makefile | 12 ++---- scripts/Makefile.compiler | 15 +++++-- 4 files changed, 49 insertions(+), 26 deletions(-) diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst index 11a296e52d68..e46f5b45c422 100644 --- a/Documentation/kbuild/makefiles.rst +++ b/Documentation/kbuild/makefiles.rst @@ -682,22 +682,42 @@ more details, with real examples. In the above example, -Wno-unused-but-set-variable will be added to KBUILD_CFLAGS only if gcc really accepts it. - cc-ifversion - cc-ifversion tests the version of $(CC) and equals the fourth parameter - if version expression is true, or the fifth (if given) if the version - expression is false. + gcc-min-version + gcc-min-version tests if the value of $(CONFIG_GCC_VERSION) is greater than + or equal to the provided value and evaluates to y if so. Example:: - #fs/reiserfs/Makefile - ccflags-y := $(call cc-ifversion, -lt, 0402, -O1) + cflags-$(call gcc-min-version, 70100) := -foo - In this example, ccflags-y will be assigned the value -O1 if the - $(CC) version is less than 4.2. - cc-ifversion takes all the shell operators: - -eq, -ne, -lt, -le, -gt, and -ge - The third parameter may be a text as in this example, but it may also - be an expanded variable or a macro. + In this example, cflags-y will be assigned the value -foo if $(CC) is gcc and + $(CONFIG_GCC_VERSION) is >= 7.1. + + clang-min-version + clang-min-version tests if the value of $(CONFIG_CLANG_VERSION) is greater + than or equal to the provided value and evaluates to y if so. + + Example:: + + cflags-$(call clang-min-version, 110000) := -foo + + In this example, cflags-y will be assigned the value -foo if $(CC) is clang + and $(CONFIG_CLANG_VERSION) is >= 11.0.0. + + cc-min-version + cc-min-version tests if the value of $(CONFIG_GCC_VERSION) is greater + than or equal to the first value provided, or if the value of + $(CONFIG_CLANG_VERSION) is greater than or equal to the second value + provided, and evaluates + to y if so. + + Example:: + + cflags-$(call cc-min-version, 70100, 110000) := -foo + + In this example, cflags-y will be assigned the value -foo if $(CC) is gcc and + $(CONFIG_GCC_VERSION) is >= 7.1, or if $(CC) is clang and + $(CONFIG_CLANG_VERSION) is >= 11.0.0. cc-cross-prefix cc-cross-prefix is used to check if there exists a $(CC) in path with diff --git a/Makefile b/Makefile index 952d354069a4..caa39ecb1136 100644 --- a/Makefile +++ b/Makefile @@ -972,7 +972,7 @@ ifdef CONFIG_CC_IS_GCC KBUILD_CFLAGS += -Wno-maybe-uninitialized endif -ifdef CONFIG_CC_IS_GCC +ifeq ($(call gcc-min-version, 90100),y) # The allocators already balk at large sizes, so silence the compiler # warnings for bounds checks involving those possible values. While # -Wno-alloc-size-larger-than would normally be used here, earlier versions @@ -984,7 +984,7 @@ ifdef CONFIG_CC_IS_GCC # ignored, continuing to default to PTRDIFF_MAX. So, left with no other # choice, we must perform a versioned check to disable this warning. # https://lore.kernel.org/lkml/20210824115859.187f272f@canb.auug.org.au -KBUILD_CFLAGS += $(call cc-ifversion, -ge, 0901, -Wno-alloc-size-larger-than) +KBUILD_CFLAGS += -Wno-alloc-size-larger-than endif # disable invalid "can't wrap" optimizations for signed / pointers diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile index 86a3b5bfd699..d8ee4743b2e3 100644 --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile @@ -33,20 +33,14 @@ ifdef CONFIG_PPC64 dml_ccflags := -mhard-float -maltivec endif -ifdef CONFIG_CC_IS_GCC -ifeq ($(call cc-ifversion, -lt, 0701, y), y) -IS_OLD_GCC = 1 -endif -endif - ifdef CONFIG_X86 -ifdef IS_OLD_GCC +ifeq ($(call gcc-min-version, 70100),y) +dml_ccflags += -msse2 +else # Stack alignment mismatch, proceed with caution. # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3 # (8B stack alignment). dml_ccflags += -mpreferred-stack-boundary=4 -else -dml_ccflags += -msse2 endif endif diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler index d1739f0d3ce3..13dade724fa3 100644 --- a/scripts/Makefile.compiler +++ b/scripts/Makefile.compiler @@ -61,9 +61,18 @@ cc-option-yn = $(call try-run,\ cc-disable-warning = $(call try-run,\ $(CC) -Werror $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1))) -# cc-ifversion -# Usage: EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1) -cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || echo $(4)) +# gcc-min-version +# Usage: cflags-$(call gcc-min-version, 70100) += -foo +gcc-min-version = $(shell [ $(CONFIG_GCC_VERSION) -ge $(1) ] && echo y) + +# clang-min-version +# Usage: cflags-$(call clang-min-version, 110000) += -foo +clang-min-version = $(shell [ $(CONFIG_CLANG_VERSION) -ge $(1) ] && echo y) + +# cc-min-version +# Usage: cflags-$(call cc-min-version, 701000, 110000) +# ^ GCC ^ Clang +cc-min-version = $(filter y, $(call gcc-min-version, $(1)), $(call clang-min-version, $(2))) # ld-option # Usage: KBUILD_LDFLAGS += $(call ld-option, -X, -Y) -- 2.37.2.672.g94769d06f0-goog ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros 2022-08-31 18:44 ` [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros Nick Desaulniers @ 2022-08-31 20:08 ` Nathan Chancellor 2022-09-05 6:44 ` Masahiro Yamada 1 sibling, 0 replies; 19+ messages in thread From: Nathan Chancellor @ 2022-08-31 20:08 UTC (permalink / raw) To: Nick Desaulniers Cc: Masahiro Yamada, Michal Marek, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Jonathan Corbet, linux-doc, amd-gfx, dri-devel On Wed, Aug 31, 2022 at 11:44:06AM -0700, Nick Desaulniers wrote: > cc-ifversion is GCC specific. Replace it with compiler specific > variants. Update the users of cc-ifversion to use these new macros. > Provide a helper for checking compiler versions for GCC and Clang > simultaneously, that will be used in a follow up patch. > > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: linux-doc@vger.kernel.org > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Link: https://github.com/ClangBuiltLinux/linux/issues/350 > Link: https://lore.kernel.org/llvm/CAGG=3QWSAUakO42kubrCap8fp-gm1ERJJAYXTnP1iHk_wrH=BQ@mail.gmail.com/ > Suggested-by: Bill Wendling <morbo@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> These are so much nicer. I find the name kind of awkward but the only thing I could come up with that sounded better was 'gcc-is-at-least' or 'clang-is-at-least' and I don't really feel like painting sheds today, it's hot outside :) Reviewed-by: Nathan Chancellor <nathan@kernel.org> Some comments below. > --- > Changes v1 -> v2: > * New patch. > > Documentation/kbuild/makefiles.rst | 44 +++++++++++++++------ > Makefile | 4 +- > drivers/gpu/drm/amd/display/dc/dml/Makefile | 12 ++---- > scripts/Makefile.compiler | 15 +++++-- > 4 files changed, 49 insertions(+), 26 deletions(-) > > diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst > index 11a296e52d68..e46f5b45c422 100644 > --- a/Documentation/kbuild/makefiles.rst > +++ b/Documentation/kbuild/makefiles.rst > @@ -682,22 +682,42 @@ more details, with real examples. > In the above example, -Wno-unused-but-set-variable will be added to > KBUILD_CFLAGS only if gcc really accepts it. > > - cc-ifversion > - cc-ifversion tests the version of $(CC) and equals the fourth parameter > - if version expression is true, or the fifth (if given) if the version > - expression is false. > + gcc-min-version > + gcc-min-version tests if the value of $(CONFIG_GCC_VERSION) is greater than > + or equal to the provided value and evaluates to y if so. > > Example:: > > - #fs/reiserfs/Makefile > - ccflags-y := $(call cc-ifversion, -lt, 0402, -O1) > + cflags-$(call gcc-min-version, 70100) := -foo > > - In this example, ccflags-y will be assigned the value -O1 if the > - $(CC) version is less than 4.2. > - cc-ifversion takes all the shell operators: > - -eq, -ne, -lt, -le, -gt, and -ge > - The third parameter may be a text as in this example, but it may also > - be an expanded variable or a macro. > + In this example, cflags-y will be assigned the value -foo if $(CC) is gcc and > + $(CONFIG_GCC_VERSION) is >= 7.1. > + > + clang-min-version > + clang-min-version tests if the value of $(CONFIG_CLANG_VERSION) is greater > + than or equal to the provided value and evaluates to y if so. > + > + Example:: > + > + cflags-$(call clang-min-version, 110000) := -foo > + > + In this example, cflags-y will be assigned the value -foo if $(CC) is clang > + and $(CONFIG_CLANG_VERSION) is >= 11.0.0. > + > + cc-min-version > + cc-min-version tests if the value of $(CONFIG_GCC_VERSION) is greater > + than or equal to the first value provided, or if the value of > + $(CONFIG_CLANG_VERSION) is greater than or equal to the second value > + provided, and evaluates > + to y if so. > + > + Example:: > + > + cflags-$(call cc-min-version, 70100, 110000) := -foo > + > + In this example, cflags-y will be assigned the value -foo if $(CC) is gcc and > + $(CONFIG_GCC_VERSION) is >= 7.1, or if $(CC) is clang and > + $(CONFIG_CLANG_VERSION) is >= 11.0.0. > > cc-cross-prefix > cc-cross-prefix is used to check if there exists a $(CC) in path with > diff --git a/Makefile b/Makefile > index 952d354069a4..caa39ecb1136 100644 > --- a/Makefile > +++ b/Makefile > @@ -972,7 +972,7 @@ ifdef CONFIG_CC_IS_GCC > KBUILD_CFLAGS += -Wno-maybe-uninitialized > endif > > -ifdef CONFIG_CC_IS_GCC > +ifeq ($(call gcc-min-version, 90100),y) > # The allocators already balk at large sizes, so silence the compiler > # warnings for bounds checks involving those possible values. While > # -Wno-alloc-size-larger-than would normally be used here, earlier versions > @@ -984,7 +984,7 @@ ifdef CONFIG_CC_IS_GCC > # ignored, continuing to default to PTRDIFF_MAX. So, left with no other > # choice, we must perform a versioned check to disable this warning. > # https://lore.kernel.org/lkml/20210824115859.187f272f@canb.auug.org.au > -KBUILD_CFLAGS += $(call cc-ifversion, -ge, 0901, -Wno-alloc-size-larger-than) > +KBUILD_CFLAGS += -Wno-alloc-size-larger-than > endif It might be interesting to move this up to where KBUILD_CFLAGS-y is used to make it: KBUILD_CFLAGS-$(call gcc-min-version, 90100) += -Wno-alloc-size-larger-than But the comment would have to come with so it probably isn't worth doing. Just throwing it out as an observation. > > # disable invalid "can't wrap" optimizations for signed / pointers > diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile > index 86a3b5bfd699..d8ee4743b2e3 100644 > --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile > +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile > @@ -33,20 +33,14 @@ ifdef CONFIG_PPC64 > dml_ccflags := -mhard-float -maltivec > endif > > -ifdef CONFIG_CC_IS_GCC > -ifeq ($(call cc-ifversion, -lt, 0701, y), y) > -IS_OLD_GCC = 1 > -endif > -endif > - > ifdef CONFIG_X86 > -ifdef IS_OLD_GCC > +ifeq ($(call gcc-min-version, 70100),y) > +dml_ccflags += -msse2 I think you just dropped '-msse2' for clang. I guess this wants to be: ifeq ($(call cc-min-version, 70100, 110000),y) but it kind of feels bad to hardcode the kernel's minimum supported version of clang somewhere that is not super easy to grep for when we upgrade it (I guess I'll add cc-min-version to my list of things to look for, in addition to the Kconfig variables). Perhaps we should codify it in a place that can be used within make (using 'scripts/min-tool-version.sh' even) so that we could do something like: ifeq ($(call cc-min-version, 70100, $(MIN_CLANG_VERSION)),y) Up to you though. > +else > # Stack alignment mismatch, proceed with caution. > # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3 > # (8B stack alignment). > dml_ccflags += -mpreferred-stack-boundary=4 > -else > -dml_ccflags += -msse2 > endif > endif > > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler > index d1739f0d3ce3..13dade724fa3 100644 > --- a/scripts/Makefile.compiler > +++ b/scripts/Makefile.compiler > @@ -61,9 +61,18 @@ cc-option-yn = $(call try-run,\ > cc-disable-warning = $(call try-run,\ > $(CC) -Werror $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1))) > > -# cc-ifversion > -# Usage: EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1) > -cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || echo $(4)) > +# gcc-min-version > +# Usage: cflags-$(call gcc-min-version, 70100) += -foo > +gcc-min-version = $(shell [ $(CONFIG_GCC_VERSION) -ge $(1) ] && echo y) > + > +# clang-min-version > +# Usage: cflags-$(call clang-min-version, 110000) += -foo > +clang-min-version = $(shell [ $(CONFIG_CLANG_VERSION) -ge $(1) ] && echo y) > + > +# cc-min-version > +# Usage: cflags-$(call cc-min-version, 701000, 110000) > +# ^ GCC ^ Clang > +cc-min-version = $(filter y, $(call gcc-min-version, $(1)), $(call clang-min-version, $(2))) > > # ld-option > # Usage: KBUILD_LDFLAGS += $(call ld-option, -X, -Y) > -- > 2.37.2.672.g94769d06f0-goog > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros 2022-08-31 18:44 ` [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros Nick Desaulniers 2022-08-31 20:08 ` Nathan Chancellor @ 2022-09-05 6:44 ` Masahiro Yamada 1 sibling, 0 replies; 19+ messages in thread From: Masahiro Yamada @ 2022-09-05 6:44 UTC (permalink / raw) To: Nick Desaulniers Cc: Michal Marek, Nathan Chancellor, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Jonathan Corbet, open list:DOCUMENTATION, amd-gfx list, dri-devel On Thu, Sep 1, 2022 at 3:44 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > cc-ifversion is GCC specific. Replace it with compiler specific > variants. Update the users of cc-ifversion to use these new macros. > Provide a helper for checking compiler versions for GCC and Clang > simultaneously, that will be used in a follow up patch. > > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: linux-doc@vger.kernel.org > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Link: https://github.com/ClangBuiltLinux/linux/issues/350 > Link: https://lore.kernel.org/llvm/CAGG=3QWSAUakO42kubrCap8fp-gm1ERJJAYXTnP1iHk_wrH=BQ@mail.gmail.com/ > Suggested-by: Bill Wendling <morbo@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > Changes v1 -> v2: > * New patch. > > Documentation/kbuild/makefiles.rst | 44 +++++++++++++++------ > Makefile | 4 +- > drivers/gpu/drm/amd/display/dc/dml/Makefile | 12 ++---- > scripts/Makefile.compiler | 15 +++++-- > 4 files changed, 49 insertions(+), 26 deletions(-) > > diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst > index 11a296e52d68..e46f5b45c422 100644 > --- a/Documentation/kbuild/makefiles.rst > +++ b/Documentation/kbuild/makefiles.rst > @@ -682,22 +682,42 @@ more details, with real examples. > In the above example, -Wno-unused-but-set-variable will be added to > KBUILD_CFLAGS only if gcc really accepts it. > > - cc-ifversion > - cc-ifversion tests the version of $(CC) and equals the fourth parameter > - if version expression is true, or the fifth (if given) if the version > - expression is false. > + gcc-min-version > + gcc-min-version tests if the value of $(CONFIG_GCC_VERSION) is greater than > + or equal to the provided value and evaluates to y if so. > > Example:: > > - #fs/reiserfs/Makefile > - ccflags-y := $(call cc-ifversion, -lt, 0402, -O1) > + cflags-$(call gcc-min-version, 70100) := -foo > > - In this example, ccflags-y will be assigned the value -O1 if the > - $(CC) version is less than 4.2. > - cc-ifversion takes all the shell operators: > - -eq, -ne, -lt, -le, -gt, and -ge > - The third parameter may be a text as in this example, but it may also > - be an expanded variable or a macro. > + In this example, cflags-y will be assigned the value -foo if $(CC) is gcc and > + $(CONFIG_GCC_VERSION) is >= 7.1. > + > + clang-min-version > + clang-min-version tests if the value of $(CONFIG_CLANG_VERSION) is greater > + than or equal to the provided value and evaluates to y if so. > + > + Example:: > + > + cflags-$(call clang-min-version, 110000) := -foo > + > + In this example, cflags-y will be assigned the value -foo if $(CC) is clang > + and $(CONFIG_CLANG_VERSION) is >= 11.0.0. > + > + cc-min-version > + cc-min-version tests if the value of $(CONFIG_GCC_VERSION) is greater > + than or equal to the first value provided, or if the value of > + $(CONFIG_CLANG_VERSION) is greater than or equal to the second value > + provided, and evaluates > + to y if so. > + > + Example:: > + > + cflags-$(call cc-min-version, 70100, 110000) := -foo > + > + In this example, cflags-y will be assigned the value -foo if $(CC) is gcc and > + $(CONFIG_GCC_VERSION) is >= 7.1, or if $(CC) is clang and > + $(CONFIG_CLANG_VERSION) is >= 11.0.0. > > cc-cross-prefix > cc-cross-prefix is used to check if there exists a $(CC) in path with > diff --git a/Makefile b/Makefile > index 952d354069a4..caa39ecb1136 100644 > --- a/Makefile > +++ b/Makefile > @@ -972,7 +972,7 @@ ifdef CONFIG_CC_IS_GCC > KBUILD_CFLAGS += -Wno-maybe-uninitialized > endif > > -ifdef CONFIG_CC_IS_GCC > +ifeq ($(call gcc-min-version, 90100),y) > # The allocators already balk at large sizes, so silence the compiler > # warnings for bounds checks involving those possible values. While > # -Wno-alloc-size-larger-than would normally be used here, earlier versions > @@ -984,7 +984,7 @@ ifdef CONFIG_CC_IS_GCC > # ignored, continuing to default to PTRDIFF_MAX. So, left with no other > # choice, we must perform a versioned check to disable this warning. > # https://lore.kernel.org/lkml/20210824115859.187f272f@canb.auug.org.au > -KBUILD_CFLAGS += $(call cc-ifversion, -ge, 0901, -Wno-alloc-size-larger-than) > +KBUILD_CFLAGS += -Wno-alloc-size-larger-than > endif > > # disable invalid "can't wrap" optimizations for signed / pointers > diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile > index 86a3b5bfd699..d8ee4743b2e3 100644 > --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile > +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile > @@ -33,20 +33,14 @@ ifdef CONFIG_PPC64 > dml_ccflags := -mhard-float -maltivec > endif > > -ifdef CONFIG_CC_IS_GCC > -ifeq ($(call cc-ifversion, -lt, 0701, y), y) > -IS_OLD_GCC = 1 > -endif > -endif > - > ifdef CONFIG_X86 > -ifdef IS_OLD_GCC > +ifeq ($(call gcc-min-version, 70100),y) > +dml_ccflags += -msse2 > +else > # Stack alignment mismatch, proceed with caution. > # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3 > # (8B stack alignment). > dml_ccflags += -mpreferred-stack-boundary=4 > -else > -dml_ccflags += -msse2 > endif > endif > > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler > index d1739f0d3ce3..13dade724fa3 100644 > --- a/scripts/Makefile.compiler > +++ b/scripts/Makefile.compiler > @@ -61,9 +61,18 @@ cc-option-yn = $(call try-run,\ > cc-disable-warning = $(call try-run,\ > $(CC) -Werror $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1))) > > -# cc-ifversion > -# Usage: EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1) > -cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || echo $(4)) > +# gcc-min-version > +# Usage: cflags-$(call gcc-min-version, 70100) += -foo > +gcc-min-version = $(shell [ $(CONFIG_GCC_VERSION) -ge $(1) ] && echo y) > + > +# clang-min-version > +# Usage: cflags-$(call clang-min-version, 110000) += -foo > +clang-min-version = $(shell [ $(CONFIG_CLANG_VERSION) -ge $(1) ] && echo y) > + > +# cc-min-version > +# Usage: cflags-$(call cc-min-version, 701000, 110000) > +# ^ GCC ^ Clang > +cc-min-version = $(filter y, $(call gcc-min-version, $(1)), $(call clang-min-version, $(2))) A more intuitive and more efficient form would be: cc-min-version = $(or $(call gcc-min-version, $(1)), $(call clang-min-version, $(2))) In your implementation, both gcc-min-version and clang-min-version are expanded before being passed to $(filter ...). So the shell is always invoked twice. $(or A, B) is lazily expanded; A is evaluated first. If and only if A is empty, B is expanded. If gcc-min-version is met, the shell invocation in clang-min-version will be short-cut. But, I do not find a place where cc-min-version is useful. Looking at the next patch, # gcc-11+, clang-14+ ifeq ($(call cc-min-version, 110000, 140000),y) dwarf-version-y := 5 else dwarf-version-y := 4 endif ... can be written in a more simpler way: dwarf-version-y := 4 dwarf-version-$(call gcc-min-version, 110000) := 5 dwarf-version-$(call clang-min-version, 140000) := 5 With $(call cc-min-version, 110000, 140000), you never know the meaning of 110000, 140000 until you see the definition of this macro. So, you feel like adding the comment "gcc-11+, clang-14+". The latter form, the code is self-documenting. > # ld-option > # Usage: KBUILD_LDFLAGS += $(call ld-option, -X, -Y) > -- > 2.37.2.672.g94769d06f0-goog > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files 2022-08-31 18:44 [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT Nick Desaulniers ` (2 preceding siblings ...) 2022-08-31 18:44 ` [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros Nick Desaulniers @ 2022-08-31 18:44 ` Nick Desaulniers 2022-08-31 20:22 ` Nathan Chancellor 2022-09-05 7:49 ` Masahiro Yamada 2022-08-31 18:44 ` [PATCH v2 5/5] Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT Nick Desaulniers 4 siblings, 2 replies; 19+ messages in thread From: Nick Desaulniers @ 2022-08-31 18:44 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Nick Desaulniers Alexey reported that the fraction of unknown filename instances in kallsyms grew from ~0.3% to ~10% recently; Bill and Greg tracked it down to assembler defined symbols, which regressed as a result of: commit b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") In that commit, I allude to restoring debug info for assembler defined symbols in a follow up patch, but it seems I forgot to do so in commit a66049e2cf0e ("Kbuild: make DWARF version a choice") This patch does a few things: 1. Add -g to KBUILD_AFLAGS. This will instruct the compiler to instruct the assembler to emit debug info. But this can cause an issue for folks using a newer compiler but older assembler, because the implicit default DWARF version changed from v4 to v5 in gcc-11 and clang-14. 2. If the user is using CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT, use a version check to explicitly set -Wa,-gdwarf-<version> for the assembler. There's another problem with this; GAS only gained support for explicit DWARF versions 3-5 in the 2.36 GNU binutils release. 3. Wrap -Wa,-gdwarf-<version> in as-option call to test whether the assembler supports that explicit DWARF version. Link: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31bf18645d98b4d3d7357353be840e320649a67d Fixes: b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") Reported-by: Alexey Alexandrov <aalexand@google.com> Reported-by: Bill Wendling <morbo@google.com> Reported-by: Greg Thelen <gthelen@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v1 -> v2: * Use newly added compiler-specific macros, as per Bill. scripts/Makefile.debug | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug index 9f39b0130551..46e88f0ca998 100644 --- a/scripts/Makefile.debug +++ b/scripts/Makefile.debug @@ -4,18 +4,32 @@ ifdef CONFIG_DEBUG_INFO_SPLIT DEBUG_CFLAGS += -gsplit-dwarf else DEBUG_CFLAGS += -g +KBUILD_AFLAGS += -g endif -ifndef CONFIG_AS_IS_LLVM -KBUILD_AFLAGS += -Wa,-gdwarf-2 +ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT +# gcc-11+, clang-14+ +ifeq ($(call cc-min-version, 110000, 140000),y) +dwarf-version-y := 5 +else +dwarf-version-y := 4 endif - -ifndef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT +else # !CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT dwarf-version-$(CONFIG_DEBUG_INFO_DWARF4) := 4 dwarf-version-$(CONFIG_DEBUG_INFO_DWARF5) := 5 DEBUG_CFLAGS += -gdwarf-$(dwarf-version-y) endif +# Binutils 2.35+ (or clang) required for -gdwarf-{4|5}. +# https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31bf18645d98b4d3d7357353be840e320649a67d +ifneq ($(call as-option,-Wa$(comma)-gdwarf-$(dwarf-version-y)),) +KBUILD_AFLAGS += -Wa,-gdwarf-$(dwarf-version-y) +else +ifndef CONFIG_AS_IS_LLVM +KBUILD_AFLAGS += -Wa,-gdwarf-2 +endif +endif + ifdef CONFIG_DEBUG_INFO_REDUCED DEBUG_CFLAGS += -fno-var-tracking ifdef CONFIG_CC_IS_GCC -- 2.37.2.672.g94769d06f0-goog ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files 2022-08-31 18:44 ` [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files Nick Desaulniers @ 2022-08-31 20:22 ` Nathan Chancellor 2022-09-05 7:49 ` Masahiro Yamada 1 sibling, 0 replies; 19+ messages in thread From: Nathan Chancellor @ 2022-08-31 20:22 UTC (permalink / raw) To: Nick Desaulniers Cc: Masahiro Yamada, Michal Marek, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Wed, Aug 31, 2022 at 11:44:07AM -0700, Nick Desaulniers wrote: > Alexey reported that the fraction of unknown filename instances in > kallsyms grew from ~0.3% to ~10% recently; Bill and Greg tracked it down > to assembler defined symbols, which regressed as a result of: > > commit b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") > > In that commit, I allude to restoring debug info for assembler defined > symbols in a follow up patch, but it seems I forgot to do so in > > commit a66049e2cf0e ("Kbuild: make DWARF version a choice") > > This patch does a few things: > 1. Add -g to KBUILD_AFLAGS. This will instruct the compiler to instruct > the assembler to emit debug info. But this can cause an issue for > folks using a newer compiler but older assembler, because the > implicit default DWARF version changed from v4 to v5 in gcc-11 and > clang-14. > 2. If the user is using CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT, use a > version check to explicitly set -Wa,-gdwarf-<version> for the > assembler. There's another problem with this; GAS only gained support > for explicit DWARF versions 3-5 in the 2.36 GNU binutils release. > 3. Wrap -Wa,-gdwarf-<version> in as-option call to test whether the > assembler supports that explicit DWARF version. > > Link: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31bf18645d98b4d3d7357353be840e320649a67d > Fixes: b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") > Reported-by: Alexey Alexandrov <aalexand@google.com> > Reported-by: Bill Wendling <morbo@google.com> > Reported-by: Greg Thelen <gthelen@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > Changes v1 -> v2: > * Use newly added compiler-specific macros, as per Bill. > > scripts/Makefile.debug | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug > index 9f39b0130551..46e88f0ca998 100644 > --- a/scripts/Makefile.debug > +++ b/scripts/Makefile.debug > @@ -4,18 +4,32 @@ ifdef CONFIG_DEBUG_INFO_SPLIT > DEBUG_CFLAGS += -gsplit-dwarf > else > DEBUG_CFLAGS += -g > +KBUILD_AFLAGS += -g > endif > > -ifndef CONFIG_AS_IS_LLVM > -KBUILD_AFLAGS += -Wa,-gdwarf-2 > +ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > +# gcc-11+, clang-14+ > +ifeq ($(call cc-min-version, 110000, 140000),y) > +dwarf-version-y := 5 > +else > +dwarf-version-y := 4 > endif > - > -ifndef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > +else # !CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > dwarf-version-$(CONFIG_DEBUG_INFO_DWARF4) := 4 > dwarf-version-$(CONFIG_DEBUG_INFO_DWARF5) := 5 > DEBUG_CFLAGS += -gdwarf-$(dwarf-version-y) > endif > > +# Binutils 2.35+ (or clang) required for -gdwarf-{4|5}. > +# https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31bf18645d98b4d3d7357353be840e320649a67d > +ifneq ($(call as-option,-Wa$(comma)-gdwarf-$(dwarf-version-y)),) > +KBUILD_AFLAGS += -Wa,-gdwarf-$(dwarf-version-y) > +else > +ifndef CONFIG_AS_IS_LLVM > +KBUILD_AFLAGS += -Wa,-gdwarf-2 > +endif > +endif > + > ifdef CONFIG_DEBUG_INFO_REDUCED > DEBUG_CFLAGS += -fno-var-tracking > ifdef CONFIG_CC_IS_GCC > -- > 2.37.2.672.g94769d06f0-goog > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files 2022-08-31 18:44 ` [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files Nick Desaulniers 2022-08-31 20:22 ` Nathan Chancellor @ 2022-09-05 7:49 ` Masahiro Yamada 2022-09-07 4:27 ` Nick Desaulniers 1 sibling, 1 reply; 19+ messages in thread From: Masahiro Yamada @ 2022-09-05 7:49 UTC (permalink / raw) To: Nick Desaulniers Cc: Michal Marek, Nathan Chancellor, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen [-- Attachment #1: Type: text/plain, Size: 3584 bytes --] On Thu, Sep 1, 2022 at 3:44 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Alexey reported that the fraction of unknown filename instances in > kallsyms grew from ~0.3% to ~10% recently; Bill and Greg tracked it down > to assembler defined symbols, which regressed as a result of: > > commit b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") > > In that commit, I allude to restoring debug info for assembler defined > symbols in a follow up patch, but it seems I forgot to do so in > > commit a66049e2cf0e ("Kbuild: make DWARF version a choice") > > This patch does a few things: > 1. Add -g to KBUILD_AFLAGS. This will instruct the compiler to instruct > the assembler to emit debug info. But this can cause an issue for > folks using a newer compiler but older assembler, because the > implicit default DWARF version changed from v4 to v5 in gcc-11 and > clang-14. What kind of bad things happen for "KBUILD_AFLAGS += -g"? I think 'gcc -g -c -o foo.o foo.S' will invoke 'as --gdwarf-2' as the backend if gcc is configured to work with old binutils. > diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug > index 9f39b0130551..46e88f0ca998 100644 > --- a/scripts/Makefile.debug > +++ b/scripts/Makefile.debug > @@ -4,18 +4,32 @@ ifdef CONFIG_DEBUG_INFO_SPLIT > DEBUG_CFLAGS += -gsplit-dwarf > else > DEBUG_CFLAGS += -g > +KBUILD_AFLAGS += -g > endif > > -ifndef CONFIG_AS_IS_LLVM > -KBUILD_AFLAGS += -Wa,-gdwarf-2 > +ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > +# gcc-11+, clang-14+ > +ifeq ($(call cc-min-version, 110000, 140000),y) > +dwarf-version-y := 5 > +else > +dwarf-version-y := 4 If you explicitly specify the DWARF version for CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT, what is the point of CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT? When CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y, I believe the right thing to do is to pass only -g, and let the tool do whatever it thinks is appropriate. > endif > - > -ifndef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > +else # !CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > dwarf-version-$(CONFIG_DEBUG_INFO_DWARF4) := 4 > dwarf-version-$(CONFIG_DEBUG_INFO_DWARF5) := 5 > DEBUG_CFLAGS += -gdwarf-$(dwarf-version-y) > endif > > +# Binutils 2.35+ (or clang) required for -gdwarf-{4|5}. > +# https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31bf18645d98b4d3d7357353be840e320649a67d > +ifneq ($(call as-option,-Wa$(comma)-gdwarf-$(dwarf-version-y)),) When is this as-option supposed to fail? Binutils <= 2.34 always accepts whatever -gdwarf-* option. Surprisingly or not, it accepts -gdwarf-6, -gdwarf-7, ... No matter what DWARF version you specify, GAS silently downgrades it to DWARF-2. masahiro@zoe:~/tools/binutils-2.34/bin$ ./as --version | head -n 1 GNU assembler (GNU Binutils) 2.34 masahiro@zoe:~/tools/binutils-2.34/bin$ cat /dev/null | ./as -gdwarf-5 -o /dev/null - masahiro@zoe:~/tools/binutils-2.34/bin$ echo $? 0 masahiro@zoe:~/tools/binutils-2.34/bin$ cat /dev/null | ./as -gdwarf-100 -o /dev/null - masahiro@zoe:~/tools/binutils-2.34/bin$ echo $? 0 Overall, I am not convinced with this patch. Please see the attached patch. Is there any problem with writing this more simply? > +KBUILD_AFLAGS += -Wa,-gdwarf-$(dwarf-version-y) > +else > +ifndef CONFIG_AS_IS_LLVM > +KBUILD_AFLAGS += -Wa,-gdwarf-2 > +endif > +endif > + > ifdef CONFIG_DEBUG_INFO_REDUCED > DEBUG_CFLAGS += -fno-var-tracking > ifdef CONFIG_CC_IS_GCC > -- > 2.37.2.672.g94769d06f0-goog > -- Best Regards Masahiro Yamada [-- Attachment #2: dwarf.diff --] [-- Type: text/x-patch, Size: 1487 bytes --] diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index bcbe60d6c80c..37aa19f0f7b0 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -264,6 +264,7 @@ config DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT config DEBUG_INFO_DWARF4 bool "Generate DWARF Version 4 debuginfo" select DEBUG_INFO + depends on !CC_IS_CLANG || (CC_IS_CLANG && (AS_IS_LLVM || (AS_IS_GNU && AS_VERSION >= 23500))) help Generate DWARF v4 debug info. This requires gcc 4.5+ and gdb 7.0+. diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug index 9f39b0130551..c5ecb882a638 100644 --- a/scripts/Makefile.debug +++ b/scripts/Makefile.debug @@ -3,17 +3,20 @@ DEBUG_CFLAGS := ifdef CONFIG_DEBUG_INFO_SPLIT DEBUG_CFLAGS += -gsplit-dwarf else -DEBUG_CFLAGS += -g +debug-flags-y += -g endif -ifndef CONFIG_AS_IS_LLVM -KBUILD_AFLAGS += -Wa,-gdwarf-2 -endif +debug-flags-$(CONFIG_DEBUG_INFO_DWARF4) += -gdwarf-4 +debug-flags-$(CONFIG_DEBUG_INFO_DWARF5) += -gdwarf-5 + +DEBUG_CFLAGS += $(debug-flags-y) -ifndef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT -dwarf-version-$(CONFIG_DEBUG_INFO_DWARF4) := 4 -dwarf-version-$(CONFIG_DEBUG_INFO_DWARF5) := 5 -DEBUG_CFLAGS += -gdwarf-$(dwarf-version-y) +ifeq ($(CONFIG_CC_IS_CLANG)$(CONFIG_AS_IS_GNU),yy) +# Clang does not pass -g or -gdwarf-* option down to GAS. +# Add -Wa, prefix to explicitly specify the flags. +KBUILD_AFLAGS += $(addprefix -Wa$(comma), $(debug-flags-y)) +else +KBUILD_AFLAGS += $(debug-flags-y) endif ifdef CONFIG_DEBUG_INFO_REDUCED ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files 2022-09-05 7:49 ` Masahiro Yamada @ 2022-09-07 4:27 ` Nick Desaulniers 0 siblings, 0 replies; 19+ messages in thread From: Nick Desaulniers @ 2022-09-07 4:27 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, Linux Kbuild mailing list, Linux Kernel Mailing List, clang-built-linux, X86 ML, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen On Mon, Sep 5, 2022 at 12:50 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Sep 1, 2022 at 3:44 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > Alexey reported that the fraction of unknown filename instances in > > kallsyms grew from ~0.3% to ~10% recently; Bill and Greg tracked it down > > to assembler defined symbols, which regressed as a result of: > > > > commit b8a9092330da ("Kbuild: do not emit debug info for assembly with LLVM_IAS=1") > > > > In that commit, I allude to restoring debug info for assembler defined > > symbols in a follow up patch, but it seems I forgot to do so in > > > > commit a66049e2cf0e ("Kbuild: make DWARF version a choice") > > > > This patch does a few things: > > 1. Add -g to KBUILD_AFLAGS. This will instruct the compiler to instruct > > the assembler to emit debug info. But this can cause an issue for > > folks using a newer compiler but older assembler, because the > > implicit default DWARF version changed from v4 to v5 in gcc-11 and > > clang-14. > > > > What kind of bad things happen for "KBUILD_AFLAGS += -g"? > > > I think 'gcc -g -c -o foo.o foo.S' will invoke 'as --gdwarf-2' as the backend > if gcc is configured to work with old binutils. That's fine for CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT I think? What other problems were you envisioning? > > > > > > diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug > > index 9f39b0130551..46e88f0ca998 100644 > > --- a/scripts/Makefile.debug > > +++ b/scripts/Makefile.debug > > @@ -4,18 +4,32 @@ ifdef CONFIG_DEBUG_INFO_SPLIT > > DEBUG_CFLAGS += -gsplit-dwarf > > else > > DEBUG_CFLAGS += -g > > +KBUILD_AFLAGS += -g > > endif > > > > -ifndef CONFIG_AS_IS_LLVM > > -KBUILD_AFLAGS += -Wa,-gdwarf-2 > > +ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > > +# gcc-11+, clang-14+ > > +ifeq ($(call cc-min-version, 110000, 140000),y) > > +dwarf-version-y := 5 > > +else > > +dwarf-version-y := 4 > > > > If you explicitly specify the DWARF version > for CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT, > what is the point of CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT? > > > When CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y, > I believe the right thing to do is to pass only -g, > and let the tool do whatever it thinks is appropriate. Ok, sure, I will revise. > > > > > > > > endif > > - > > -ifndef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > > +else # !CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > > dwarf-version-$(CONFIG_DEBUG_INFO_DWARF4) := 4 > > dwarf-version-$(CONFIG_DEBUG_INFO_DWARF5) := 5 > > DEBUG_CFLAGS += -gdwarf-$(dwarf-version-y) > > endif > > > > +# Binutils 2.35+ (or clang) required for -gdwarf-{4|5}. > > +# https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31bf18645d98b4d3d7357353be840e320649a67d > > +ifneq ($(call as-option,-Wa$(comma)-gdwarf-$(dwarf-version-y)),) > > > > When is this as-option supposed to fail? > > > Binutils <= 2.34 always accepts whatever -gdwarf-* option. > Surprisingly or not, it accepts -gdwarf-6, -gdwarf-7, ... > > No matter what DWARF version you specify, GAS silently downgrades > it to DWARF-2. > > > masahiro@zoe:~/tools/binutils-2.34/bin$ ./as --version | head -n 1 > GNU assembler (GNU Binutils) 2.34 > masahiro@zoe:~/tools/binutils-2.34/bin$ cat /dev/null | ./as -gdwarf-5 > -o /dev/null - > masahiro@zoe:~/tools/binutils-2.34/bin$ echo $? > 0 > masahiro@zoe:~/tools/binutils-2.34/bin$ cat /dev/null | ./as > -gdwarf-100 -o /dev/null - > masahiro@zoe:~/tools/binutils-2.34/bin$ echo $? > 0 ah, right. Maybe an explicit version check is necessary then. > > > > > Overall, I am not convinced with this patch. > > > > Please see the attached patch. > Is there any problem with writing this more simply? Thanks for the inspiration, I will use that as an inspiration/base for a new patch. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 5/5] Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT 2022-08-31 18:44 [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT Nick Desaulniers ` (3 preceding siblings ...) 2022-08-31 18:44 ` [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files Nick Desaulniers @ 2022-08-31 18:44 ` Nick Desaulniers 2022-08-31 20:23 ` Nathan Chancellor 4 siblings, 1 reply; 19+ messages in thread From: Nick Desaulniers @ 2022-08-31 18:44 UTC (permalink / raw) To: Masahiro Yamada Cc: Michal Marek, Nathan Chancellor, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Nick Desaulniers, Andi Kleen Dmitrii, Fangrui, and Mashahiro note: Before GCC 11 and Clang 12 -gsplit-dwarf implicitly uses -g2. Fix CONFIG_DEBUG_INFO_SPLIT for gcc-11+ & clang-12+ which now need -g specified in order for -gsplit-dwarf to work at all. -gsplit-dwarf has been mutually exclusive with -g since support for CONFIG_DEBUG_INFO_SPLIT was introduced in commit 866ced950bcd ("kbuild: Support split debug info v4") I don't think it ever needed to be. Link: https://lore.kernel.org/lkml/20220815013317.26121-1-dmitrii.bundin.a@gmail.com/ Link: https://lore.kernel.org/lkml/CAK7LNARPAmsJD5XKAw7m_X2g7Fi-CAAsWDQiP7+ANBjkg7R7ng@mail.gmail.com/ Link: https://reviews.llvm.org/D80391 Cc: Andi Kleen <ak@linux.intel.com> Reported-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com> Reported-by: Fangrui Song <maskray@google.com> Reported-by: Masahiro Yamada <masahiroy@kernel.org> Suggested-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v1 -> v2: * Add reference to 866ced950bcd, cc Andi, in commit message. scripts/Makefile.debug | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug index 46e88f0ca998..b6eb532af3cc 100644 --- a/scripts/Makefile.debug +++ b/scripts/Makefile.debug @@ -1,10 +1,8 @@ -DEBUG_CFLAGS := +DEBUG_CFLAGS := -g +KBUILD_AFLAGS += -g ifdef CONFIG_DEBUG_INFO_SPLIT DEBUG_CFLAGS += -gsplit-dwarf -else -DEBUG_CFLAGS += -g -KBUILD_AFLAGS += -g endif ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT -- 2.37.2.672.g94769d06f0-goog ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 5/5] Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT 2022-08-31 18:44 ` [PATCH v2 5/5] Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT Nick Desaulniers @ 2022-08-31 20:23 ` Nathan Chancellor 0 siblings, 0 replies; 19+ messages in thread From: Nathan Chancellor @ 2022-08-31 20:23 UTC (permalink / raw) To: Nick Desaulniers Cc: Masahiro Yamada, Michal Marek, Tom Rix, linux-kbuild, linux-kernel, llvm, x86, Dmitrii Bundin, Fangrui Song, Alexey Alexandrov, Bill Wendling, Greg Thelen, Andi Kleen On Wed, Aug 31, 2022 at 11:44:08AM -0700, Nick Desaulniers wrote: > Dmitrii, Fangrui, and Mashahiro note: > > Before GCC 11 and Clang 12 -gsplit-dwarf implicitly uses -g2. > > Fix CONFIG_DEBUG_INFO_SPLIT for gcc-11+ & clang-12+ which now need -g > specified in order for -gsplit-dwarf to work at all. > > -gsplit-dwarf has been mutually exclusive with -g since support for > CONFIG_DEBUG_INFO_SPLIT was introduced in > commit 866ced950bcd ("kbuild: Support split debug info v4") > I don't think it ever needed to be. > > Link: https://lore.kernel.org/lkml/20220815013317.26121-1-dmitrii.bundin.a@gmail.com/ > Link: https://lore.kernel.org/lkml/CAK7LNARPAmsJD5XKAw7m_X2g7Fi-CAAsWDQiP7+ANBjkg7R7ng@mail.gmail.com/ > Link: https://reviews.llvm.org/D80391 > Cc: Andi Kleen <ak@linux.intel.com> > Reported-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com> > Reported-by: Fangrui Song <maskray@google.com> > Reported-by: Masahiro Yamada <masahiroy@kernel.org> > Suggested-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > Changes v1 -> v2: > * Add reference to 866ced950bcd, cc Andi, in commit message. > > scripts/Makefile.debug | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug > index 46e88f0ca998..b6eb532af3cc 100644 > --- a/scripts/Makefile.debug > +++ b/scripts/Makefile.debug > @@ -1,10 +1,8 @@ > -DEBUG_CFLAGS := > +DEBUG_CFLAGS := -g > +KBUILD_AFLAGS += -g > > ifdef CONFIG_DEBUG_INFO_SPLIT > DEBUG_CFLAGS += -gsplit-dwarf > -else > -DEBUG_CFLAGS += -g > -KBUILD_AFLAGS += -g > endif > > ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > -- > 2.37.2.672.g94769d06f0-goog > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-09-07 4:28 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-31 18:44 [PATCH v2 0/5] fix debug info for asm and DEBUG_INFO_SPLIT Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 1/5] x86/boot/compressed: prefer cc-option for CFLAGS additions Nick Desaulniers 2022-08-31 19:41 ` Nathan Chancellor 2022-08-31 18:44 ` [PATCH v2 2/5] Makefile.compiler: Use KBUILD_AFLAGS for as-option Nick Desaulniers 2022-08-31 19:53 ` Nathan Chancellor 2022-09-05 9:09 ` Masahiro Yamada 2022-09-06 16:08 ` Nathan Chancellor 2022-09-07 4:14 ` Masahiro Yamada 2022-09-07 3:12 ` Nick Desaulniers 2022-09-07 4:27 ` Masahiro Yamada 2022-08-31 18:44 ` [PATCH v2 3/5] Makefile.compiler: replace cc-ifversion with compiler-specific macros Nick Desaulniers 2022-08-31 20:08 ` Nathan Chancellor 2022-09-05 6:44 ` Masahiro Yamada 2022-08-31 18:44 ` [PATCH v2 4/5] Makefile.debug: re-enable debug info for .S files Nick Desaulniers 2022-08-31 20:22 ` Nathan Chancellor 2022-09-05 7:49 ` Masahiro Yamada 2022-09-07 4:27 ` Nick Desaulniers 2022-08-31 18:44 ` [PATCH v2 5/5] Makefile.debug: set -g unconditional on CONFIG_DEBUG_INFO_SPLIT Nick Desaulniers 2022-08-31 20:23 ` Nathan Chancellor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).