Message-ID: <CAGG=3QUStBe5QKa66vza_nmrs1J=0SjJPdqHQWVoNvHMWe7KSg@mail.gmail.com>
Date:   Fri, 26 Aug 2022 13:41:22 -0700
From:   Bill Wendling <morbo@...gle.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        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 1:16 PM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> 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),)

I think for this you want "gcc-max-version".

>  IS_OLD_GCC = 1
>  endif
>  endif
> ```
>
> Thoughts?
>

I like this idea. Modifying "cc-ifversion" would be great, but you'd
have to somehow specify the checking versions for both gcc and clang
in the macro, and that's messy given the number of arguments that
already exist. It might be possible instead to implement
"cc-ifversion" in terms of the "*-{max,min}-version" macros.

-bw

> >
> > >
> > > -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