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>
In-Reply-To: <20200610191106.2662548-1-nivedita@alum.mit.edu>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Wed, 10 Jun 2020 14:27:55 -0700
Message-ID: <CAKwvOdnF9xhrs+FP4QXo6uXxgHMfHO8NvPYs1=KDE2ZyhCw2QQ@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 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.

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

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

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.

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