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;
MIME-Version: 1.0
References: <20200610063715.v2qrqvwtegdxdwzl@google.com> <20200610191106.2662548-1-nivedita@alum.mit.edu>
 <CAKwvOdnF9xhrs+FP4QXo6uXxgHMfHO8NvPYs1=KDE2ZyhCw2QQ@mail.gmail.com> <20200610233046.GA2941409@rani.riverdale.lan>
In-Reply-To: <20200610233046.GA2941409@rani.riverdale.lan>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Thu, 11 Jun 2020 14:09:21 -0700
Message-ID: <CAKwvOdnddAjiXDKA8fp3n2NN+R=Syp2N5DHbp1j=VRzM1dNnRw@mail.gmail.com>
Subject: Re: [PATCH] Makefile: Improve compressed debug info support detection
To:     Arvind Sankar <nivedita@alum.mit.edu>
Cc:     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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 10, 2020 at 4:30 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> 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.

ah, right, good to fail early in that case then.

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

Not sure whether we'd like to prevent the option from being selectable
in that case, or just to silently not compress the debug info.  Given
that the compression is an optimization, it doesn't hurt to silently
not do it, but it would be nice to notify the user their toolchain may
have been misconfigured.  Though another part of me feels "garbage in,
garbage out."

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

^ Those 2 last sentences should go in the commit message, as the
current form doesn't allude to that.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

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

Yep. Got it.

> 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

I verified that Clang will still produce compressed debug info
sections for .s/.S input with -gz=zlib.  So this doesn't regress
anything, AFAICT.

Thanks for the patch.

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


-- 
Thanks,
~Nick Desaulniers