Message-ID: <CAK7LNARofo7wawEF4EcA2-wxnQkKw+WFoJ36EOeYFTUrthRfrA@mail.gmail.com>
Date:   Sun, 24 May 2020 12:56:49 +0900
From:   Masahiro Yamada <masahiroy@...nel.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Sedat Dilek <sedat.dilek@...il.com>,
        Fangrui Song <maskray@...gle.com>,
        Nick Clifton <nickc@...hat.com>,
        David Blaikie <blakie@...gle.com>,
        Michal Marek <michal.lkml@...kovi.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Changbin Du <changbin.du@...el.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH v2] Makefile: support compressed debug info

On Fri, May 22, 2020 at 6:57 AM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@...glegroups.com> wrote:
>
> On Wed, May 20, 2020 at 7:48 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
> >
> > > Suggested-by: Fangrui Song <maskray@...gle.com>
> >
> >
> > Suggested-by -> Reviewed-by
> >
> > https://patchwork.kernel.org/patch/11524939/#23349551
>
> Yes, my mistake.
>
> > > Suggested-by: Nick Clifton <nickc@...hat.com>
> >
> >
> > I do not know where this tag came from.
> >
> > Nick Clifton taught us the version rule of binutils,but did not state
> > anything about this patch itself.
> >
> > https://patchwork.kernel.org/patch/11524939/#23355175
> >
> >
> > > Suggested-by: Sedat Dilek <sedat.dilek@...il.com>
> >
> > I do not see the source of this tag, either...
>
> Not all contributions to open source need to be in the form of
> patches.  Both Sedat and Nick gave suggestions which ultimately
> informed the contents of this patch.  They should be rewarded for
> their efforts, and incentivized to help improve the code base further.
> I think suggested by tags are a good way to do that; but if it's
> against a written convention or if you still disagree, it's not the
> end of the world to me, and you may drop those tags from the v3.


Documentation/process/submitting-patches.rst
gives the guideline.


"A Suggested-by: tag indicates that the patch idea is suggested by the person
named and ensures credit to the person for the idea. Please note that this
tag should not be added without the reporter's permission, especially if the
idea was not posted in a public forum. That said, if we diligently credit our
idea reporters, they will, hopefully, be inspired to help us again in the
future."


I think this tag should be given to people who
gave the main idea to come up with
the main part of the patch.


Is that David Blaikie who suggested to
compress the debug info ?
If so, definitely he deserves to have Suggested-by tag.

For the others, I do not think Suggested-by is a good fit.

I appreciate their contribution to improve this patch.
So, maybe you can give credit in other form, for example,
mention it in the commit log explicitly?

Nick Clifton helped us to provide the minimal binutils version.
Sedat Dilet found an increase in size of debug .deb package.


Thanks.

>
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -225,6 +225,21 @@ config DEBUG_INFO_REDUCED
> > >           DEBUG_INFO build and compile times are reduced too.
> > >           Only works with newer gcc versions.
> > >
> > > +config DEBUG_INFO_COMPRESSED
> > > +       bool "Compressed debugging information"
> > > +       depends on DEBUG_INFO
> > > +       depends on $(cc-option,-gz=zlib)
> > > +       depends on $(as-option,-Wa,--compress-debug-sections=zlib)
> >
> > This does not work. (always false)
>
> Technically, always true. `-Wa` disables all warnings from the
> assembler.  Also, I did test this via `make menuconfig`.
>
> > You cannot enable this option.
> >
> > The comma between -Wa and --compress-debug-sections=zlib
> > is eaten by Kconfig parser because commas are delimiters
> > of function parameters.
> >
> >
> > Please write like this.
> >
> >     depends on $(as-option,-Wa$(comma)--compress-debug-sections=zlib)
>
> You're right, I knew this bug forgot. Will fix in v3.
>
> > > +       depends on $(ld-option,--compress-debug-sections=zlib)
> > > +       help
> > > +         Compress the debug information using zlib.  Requires GCC 5.0+ or Clang
> > > +         5.0+, binutils 2.26+, and zlib.
> > > +
> > > +         Users of dpkg-deb via scripts/package/builddeb may
> > > +         wish to set the $KDEB_COMPRESS env var to "none" to avoid recompressing
> > > +         the debug info again with a different compression scheme, which can
> > > +         result in larger binaries.
> >
> > No. This is not correct.
> >
> > CONFIG_DEBUG_INFO_COMPRESSED compresses the only debug info part.
> > The other parts still get by benefit from the default KDEB_COMPRESS=xz.
> >
> >
> > The numbers are here:
> >
> >
> > CONFIG_DEBUG_INFO_COMPRESSED=y
> > -rw-r--r-- 1 masahiro masahiro 209077584 May 21 11:19
> > linux-image-5.7.0-rc5+-dbg_5.7.0-rc5+-26_amd64.deb
> >
> >
> > CONFIG_DEBUG_INFO_COMPRESSED=y and KDEB_COMPRESS=none
> > -rw-r--r-- 1 masahiro masahiro 643051712 May 21 11:22
> > linux-image-5.7.0-rc5+-dbg_5.7.0-rc5+-27_amd64.deb
> >
> >
> > CONFIG_DEBUG_INFO_COMPRESSED=n
> > -rw-r--r-- 1 masahiro masahiro 112200308 May 21 11:40
> > linux-image-5.7.0-rc5+-dbg_5.7.0-rc5+-30_amd64.deb
> >
> >
> >
> >
> > For the deb package size perspective,
> > it is better to keep KDEB_COMPRESS as default.
> >
> > The main motivation for changing KDEB_COMPRESS
> > is the build speed.  (see commit 1a7f0a34ea7d05)
> >
> >
> >
> >
> > CONFIG_DEBUG_INFO_COMPRESSED has a downside
> > for the debug deb package, but we need to accept it.
>
> Ah, I see. Thank you for those measurements.  I'll send a v3 with
> fixed up help text, but ultimately, I don't really care what it says
> here.  Please feel empowered to reword it should you choose to accept
> + apply it.
> --
> Thanks,
> ~Nick Desaulniers
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOd%3DjOr4ZaLx-dSNTqZnGRATY1PZktUfu4JGWKRwRH%3DUjnw%40mail.gmail.com.



-- 
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists