Re: [PATCH v5 0/3] Kbuild: DWARF v5 support

From: Yonghong Song <yhs@fb.com>
To: <sedat.dilek@gmail.com>, Nick Desaulniers <ndesaulniers@google.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>,
	Nathan Chancellor <natechancellor@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	<linux-kernel@vger.kernel.org>,
	Clang-Built-Linux ML <clang-built-linux@googlegroups.com>,
	<linux-kbuild@vger.kernel.org>, <linux-arch@vger.kernel.org>,
	Jakub Jelinek <jakub@redhat.com>,
	Fangrui Song <maskray@google.com>,
	Caroline Tice <cmtice@google.com>,
	Nick Clifton <nickc@redhat.com>, Jiri Olsa <jolsa@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>
Subject: Re: [PATCH v5 0/3] Kbuild: DWARF v5 support
Date: Fri, 15 Jan 2021 15:24:05 -0800	[thread overview]
Message-ID: <7354583d-de40-b6b9-6534-a4f4c038230f@fb.com> (raw)
In-Reply-To: <CA+icZUVp+JNq89uc_DyWC6zh5=kLtUr7eOxHizfFggnEVGJpqw@mail.gmail.com>

On 1/15/21 1:53 PM, Sedat Dilek wrote:
> On Fri, Jan 15, 2021 at 10:06 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> DWARF v5 is the latest standard of the DWARF debug info format.
>>
>> DWARF5 wins significantly in terms of size when mixed with compression
>> (CONFIG_DEBUG_INFO_COMPRESSED).
>>
>> Link: http://www.dwarfstd.org/doc/DWARF5.pdf
>>
>> Patch 1 is a cleanup from Masahiro and isn't DWARF v5 specific.
>> Patch 2 is a cleanup that lays the ground work and isn't DWARF
>> v5 specific.
>> Patch 3 implements Kconfig and Kbuild support for DWARFv5.
>>
>> Changes from v4:
>> * drop set -e from script as per Nathan.
>> * add dependency on !CONFIG_DEBUG_INFO_BTF for DWARF v5 as per Sedat.
>> * Move LLVM_IAS=1 complexity from patch 2 to patch 3 as per Arvind and
>>    Masahiro. Sorry it took me a few tries to understand the point (I
>>    might still not), but it looks much cleaner this way. Sorry Nathan, I
>>    did not carry forward your previous reviews as a result, but I would
>>    appreciate if you could look again.
>> * Add Nathan's reviewed by tag to patch 1.
>> * Reword commit message for patch 3 to mention LLVM_IAS=1 and -gdwarf-5
>>    binutils addition later, and BTF issue.
>> * I still happen to see a pahole related error spew for the combination
>>    of:
>>    * LLVM=1
>>    * LLVM_IAS=1
>>    * CONFIG_DEBUG_INFO_DWARF4
>>    * CONFIG_DEBUG_INFO_BTF
>>    Though they're non-fatal to the build. I'm not sure yet why removing
>>    any one of the above prevents the warning spew. Maybe we'll need a v6.
>>
> 
> En plus, I encountered breakage with GCC v10.2.1 and LLVM=1 and
> CONFIG_DEBUG_INFO_DWARF4.
> So might be good to add a "depends on !DEBUG_INFO_BTF" in this combination.

I suggested not to add !DEBUG_INFO_BTF to CONFIG_DEBUG_INFO_DWARF4.
It is not there before and adding this may suddenly break some users.

If certain combination of gcc/llvm does not work for 
CONFIG_DEBUG_INFO_DWARF4 with pahole, this is a bug bpf community
should fix.

> 
> I had some other small nits commented in the single patches.
> 
> As requested in your previous patch-series, feel free to add my:
> 
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> 
> - Sedat -
> 
>> Changes from v3:
>>
>> Changes as per Arvind:
>> * only add -Wa,-gdwarf-5 for (LLVM=1|CC=clang)+LLVM_IAS=0 builds.
>> * add -gdwarf-5 to Kconfig shell script.
>> * only run Kconfig shell script for Clang.
>>
>> Apologies to Sedat and Nathan; I appreciate previous testing/review, but
>> I did no carry forward your Tested-by and Reviewed-by tags, as the
>> patches have changed too much IMO.
>>
>> Changes from v2:
>> * Drop two of the earlier patches that have been accepted already.
>> * Add measurements with GCC 10.2 to commit message.
>> * Update help text as per Arvind with help from Caroline.
>> * Improve case/wording between DWARF Versions as per Masahiro.
>>
>> Changes from the RFC:
>> * split patch in 3 patch series, include Fangrui's patch, too.
>> * prefer `DWARF vX` format, as per Fangrui.
>> * use spaces between assignment in Makefile as per Masahiro.
>> * simplify setting dwarf-version-y as per Masahiro.
>> * indent `prompt` in Kconfig change as per Masahiro.
>> * remove explicit default in Kconfig as per Masahiro.
>> * add comments to test_dwarf5_support.sh.
>> * change echo in test_dwarf5_support.sh as per Masahiro.
>> * remove -u from test_dwarf5_support.sh as per Masahiro.
>> * add a -gdwarf-5 cc-option check to Kconfig as per Jakub.
>>
>> *** BLURB HERE ***
>>
>> Masahiro Yamada (1):
>>    Remove $(cc-option,-gdwarf-4) dependency from CONFIG_DEBUG_INFO_DWARF4
>>
>> Nick Desaulniers (2):
>>    Kbuild: make DWARF version a choice
>>    Kbuild: implement support for DWARF v5
>>
>>   Makefile                          | 13 +++++++---
>>   include/asm-generic/vmlinux.lds.h |  6 ++++-
>>   lib/Kconfig.debug                 | 42 +++++++++++++++++++++++++------
>>   scripts/test_dwarf5_support.sh    |  8 ++++++
>>   4 files changed, 57 insertions(+), 12 deletions(-)
>>   create mode 100755 scripts/test_dwarf5_support.sh
>>
>> --
>> 2.30.0.284.gd98b1dd5eaa7-goog
>>