Date:   Fri, 26 Aug 2022 13:16:25 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Bill Wendling <morbo@...gle.com>,
        Masahiro Yamada <masahiroy@...nel.org>
Cc:     Michal Marek <michal.lkml@...kovi.net>,
        Nathan Chancellor <nathan@...nel.org>,
        Tom Rix <trix@...hat.com>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux <llvm@...ts.linux.dev>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Dmitrii Bundin <dmitrii.bundin.a@...il.com>,
        Fangrui Song <maskray@...gle.com>,
        Alexey Alexandrov <aalexand@...gle.com>,
        Greg Thelen <gthelen@...gle.com>
Subject: Re: [PATCH 2/3] Makefile.debug: re-enable debug info for .S files

On Fri, Aug 26, 2022 at 12:52 PM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> On Fri, Aug 26, 2022 at 11:41 AM Bill Wendling <morbo@...gle.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 11:10 AM Nick Desaulniers
> > <ndesaulniers@...gle.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.
> > > 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@...gle.com>
> > > Reported-by: Bill Wendling <morbo@...gle.com>
> > > Reported-by: Greg Thelen <gthelen@...gle.com>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>
> > > ---
> > >  scripts/Makefile.debug | 22 ++++++++++++++++++----
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug
> > > index 9f39b0130551..a7a6da7f6e7d 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 ($(shell [ $(CONFIG_GCC_VERSION) -ge 110000 -o $(CONFIG_CLANG_VERSION) -ge 140000 ] && echo y),y)
> >
> > Do you think this would be better as a macro? Maybe something like:
> >
> > if $(call cc-min-version,110000,140000)
> >
> > where the first argument is GCC's min version and second Clang's min
> > version. It would be more readable and reusable.
>
> Yeah!
>
> I was looking at cc-ifversion, which has a bug in that it specifically
> uses CONFIG_GCC_VERSION.  I think I sent a series maybe a year or two
> ago trying to remove all users of that macro; I think most landed but
> not all and I never pursued it to completion.  Also, I think there's
> one user remaining in the AMDGPU drivers; looks like they're been
> reducing their dependency on that SIMD hack I wrote for them years ago
> after propagating it to parts of their tree, but one user remains.
> Perhaps I can just open code it there, or replace it with something
> new like you suggest.
>
> Such a macro would need to consider whether CC_IS_GCC vs CC_IS_CLANG;
> I could imagine we might need a version check for both, or just one of
> the two compilers.
>
> Ugh, logical OR in GNU make is supported by use of the filter macro...yuck.
>
> ifneq ($(filter y, $(call gcc-min-version, 110000), $(call
> clang-min-version, 140000),)
> # add gcc-11+, clang-14+ flag
> endif
>
> or maybe as you suggest:
>
> ifneq ($(call cc-min-version,110000,140000),)
> # add gcc-11+, clang-14+ flag
> endif
>
> where the newly minted cc-min-version is implemented in terms of the
> newly minted gcc-min-version and clang-min-version.  Then I can use
> cc-min-version in this series, and replace cc-ifversion in AMDGPU with
> gcc-min-version.  That way, we create composable wrappers that are
> readable and reusable.

RFC:
```
diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
index d1739f0d3ce3..4809a4c9e5f2 100644
--- a/scripts/Makefile.compiler
+++ b/scripts/Makefile.compiler
@@ -65,6 +65,19 @@ cc-disable-warning = $(call try-run,\
 # 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)
 ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3))
diff --git a/scripts/Makefile.debug b/scripts/Makefile.debug
index 0f9912f7bd4c..80c84f746219 100644
--- a/scripts/Makefile.debug
+++ b/scripts/Makefile.debug
@@ -7,7 +7,7 @@ endif

 ifdef CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
 # gcc-11+, clang-14+
-ifeq ($(shell [ $(CONFIG_GCC_VERSION) -ge 110000 -o
$(CONFIG_CLANG_VERSION) -ge 140000 ] && echo y),y)
+ifeq ($(call cc-min-version, 110000, 140000), y)
 dwarf-version-y := 5
 else
 dwarf-version-y := 4

```

I'd keep the replacement of cc-ifversion with gcc-min-version as a
distinct child patch, since I don't care about backports for that (and
there's probably more cc-ifversion users the further back you go) but
I think we might want cc-min-version in stable.

Technically, the above should also update
Documentation/kbuild/makefiles.rst; but I'm just testing this works;
it seems to.

Oh, it looks like cc-ifversion both permits checking max-version and
min version. But we can implement -ge and -lt with just one or the
other:

```
diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile
b/drivers/gpu/drm/amd/display/dc/dml/Makefile
index 86a3b5bfd699..17e8fd42d0a5 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
@@ -34,7 +34,7 @@ dml_ccflags := -mhard-float -maltivec
 endif

 ifdef CONFIG_CC_IS_GCC
-ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+ifeq ($(call gcc-min-version, 70100),)
 IS_OLD_GCC = 1
 endif
 endif
```

Thoughts?

>
> >
> > -bw
> >
> > > +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
> > >
>
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists