Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Arvind Sankar <nivedita@alum.mit.edu>
Date:   Wed, 10 Jun 2020 19:30:46 -0400
To:     Nick Desaulniers <ndesaulniers@google.com>
Cc:     Arvind Sankar <nivedita@alum.mit.edu>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Fangrui Song <maskray@google.com>,
        Rong Chen <rong.a.chen@intel.com>,
        kernel test robot <lkp@intel.com>, kbuild-all@lists.01.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Makefile: Improve compressed debug info support detection
Message-ID: <20200610233046.GA2941409@rani.riverdale.lan>
References: <20200610063715.v2qrqvwtegdxdwzl@google.com>
 <20200610191106.2662548-1-nivedita@alum.mit.edu>
 <CAKwvOdnF9xhrs+FP4QXo6uXxgHMfHO8NvPYs1=KDE2ZyhCw2QQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAKwvOdnF9xhrs+FP4QXo6uXxgHMfHO8NvPYs1=KDE2ZyhCw2QQ@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 10, 2020 at 02:27:55PM -0700, Nick Desaulniers wrote:
> On Wed, Jun 10, 2020 at 12:11 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Commit
> >   10e68b02c861 ("Makefile: support compressed debug info")
> > added support for compressed debug sections.
> >
> > Support is detected by checking
> > - does the compiler support -gz=zlib
> > - does the assembler support --compressed-debug-sections=zlib
> > - does the linker support --compressed-debug-sections=zlib
> >
> > However, the gcc driver's support for this option is somewhat
> > convoluted. The driver's builtin specs are set based on the version of
> > binutils that it was configured with, and it reports an error only if
> > the assembler (or linker) is actually invoked.
> >
> > The cc-option check in scripts/Kconfig.include does not invoke the
> > assembler, so the gcc driver reports success even if it does not support
> > the option being passed to the assembler.
> 
> Thanks for the patch! In that case, should we consider dropping the
> cc-option check from the Kconfg then, too?
> 
> It seems it would help for clang-4 and older, since they do error
> about the unknown option, but I'm not too worried about trying to
> support that version of Clang with this config.

Also for gcc4 which doesn't support this at all -- that will report
error on cc-option. We don't need to support it, but we shouldn't let it
be enabled either.

> 
> > Combined with an installed
> > version of binutils that is more recent than the one the compiler was
> > built with, it is possible for all three tests to succeed, yet an actual
> > compilation with -gz=zlib to fail.
> 
> It kind of sounds like the assembler must be invoked to verify this
> will work for the cflags then?
> 

Yes, the gcc driver reports an error when deciding what to pass to the
assembler for -gz=zlib, if it was configured with a linker that does not
support the flag. It's even more weird actually -- if it was configured
with a linker that supports the flag but an assembler that doesn't, it
will silently eat the flag when calling the assembler. At least that
won't break anything, though none of the .o files will be compressed.

> >
> > Moreover, it is unnecessary to explicitly pass
> > --compressed-debug-sections=zlib to the assembler via -Wa, since the
> > driver will do that automatically.
> >
> > Convert the as-option to just -gz=zlib, simplifying it as well as
> > performing a better test of the gcc driver's capabilities.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> 
> Does this imply all feature tests of as-option with -W(comma)... are
> broken?  IIUC, then the assembler is still not invoked, much as in
> this case?  (as in they all pass when maybe they should not?)  (or
> based on below, maybe just the Kconfig from 74afda4016a74 in
> arch/arm64/Kconfig, AS_HAS_PAC)

No, as-option does invoke the assembler. The problem here is that with
-Wa, the option is only seen by the assembler, not the gcc driver. So it
will succeed because the assembler supports it, but it will not test
whether the gcc driver also supports it.

> 
> In the other thread, you discussed -c vs -s.  It looks like -c is used
> for as-option, so I expect the assembler to be invoked.
> 
> Maybe we should look at the mismatch between Kbuild and Kconfig
> regarding -c vs -s in scripts/Kbuild.include vs
> scripts/Kconfig.include?  Otherwise it sounds like cc-option cannot be
> used to check for `-Wa,` flags in Kconfig, since if the assembler is
> never invoked, it may appear that GCC has support for
> -Wa,--compress-debug-sections=zlib when it indeed does not.
> 

Yeah, we might want to fix the mismatch there.  In Kconfig, there aren't
any instances of cc-option being used with -Wa, but there certainly are
in Kbuild. It actually originally used to only run the preprocessor and
got fixed to at least run the compiler in commit
  3bed1b7b9d79 ("kbuild: use -S instead of -E for precise cc-option test in Kconfig")

> > ---
> >  Makefile          | 2 +-
> >  lib/Kconfig.debug | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 839f9fee22cb..cb29e56f227a 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -842,7 +842,7 @@ endif
> >
> >  ifdef CONFIG_DEBUG_INFO_COMPRESSED
> >  DEBUG_CFLAGS   += -gz=zlib
> > -KBUILD_AFLAGS  += -Wa,--compress-debug-sections=zlib
> > +KBUILD_AFLAGS  += -gz=zlib
> >  KBUILD_LDFLAGS += --compress-debug-sections=zlib
> >  endif
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index cb98741601bd..94ce36be470c 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -229,7 +229,7 @@ config DEBUG_INFO_COMPRESSED
> >         bool "Compressed debugging information"
> >         depends on DEBUG_INFO
> >         depends on $(cc-option,-gz=zlib)
> > -       depends on $(as-option,-Wa$(comma)--compress-debug-sections=zlib)
> > +       depends on $(as-option,-gz=zlib)
> >         depends on $(ld-option,--compress-debug-sections=zlib)
> >         help
> >           Compress the debug information using zlib.  Requires GCC 5.0+ or Clang
> > --
> > 2.26.2
> >
> 
> 
> --
> Thanks,
> ~Nick Desaulniers