Re: [PATCH][1/2] Early LTO debug, simple-object part

["Ian Lance Taylor via gcc-patches" <gcc-patches at gcc dot gnu dot org>]

On Tue, Aug 15, 2017 at 5:51 AM, Richard Biener <rguenther@suse.de> wrote:
> On Tue, 15 Aug 2017, Richard Biener wrote:
>
>> On Mon, 14 Aug 2017, Ian Lance Taylor wrote:
>>
>> > On Mon, Aug 14, 2017 at 6:17 AM, Richard Biener <rguenther@suse.de> wrote:
>> > > On Fri, 4 Aug 2017, Richard Biener wrote:
>> > >
>> > >> On Fri, 28 Jul 2017, Jason Merrill wrote:
>> > >>
>> > >> > On 07/28/2017 05:55 PM, Jason Merrill wrote:
>> > >> > > On Fri, Jul 28, 2017 at 8:54 AM, Richard Biener <rguenther@suse.de> wrote:
>> > >> > > > On Tue, 4 Jul 2017, Richard Biener wrote:
>> > >> > > >
>> > >> > > > > On Tue, 20 Jun 2017, Richard Biener wrote:
>> > >> > > > >
>> > >> > > > > > On Wed, 7 Jun 2017, Richard Biener wrote:
>> > >> > > > > >
>> > >> > > > > > > On Fri, 19 May 2017, Richard Biener wrote:
>> > >> > > > > > >
>> > >> > > > > > > >
>> > >> > > > > > > > This is a repost (unchanged) of the simple-object ELF support for
>> > >> > > > > > > > early LTO debug transfer from IL object to a separate debug-only
>> > >> > > > > > > > object
>> > >> > > > > > > > file.
>> > >> > > > > > > >
>> > >> > > > > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu.
>> > >> > > > > > >
>> > >> > > > > > > Ping.
>> > >> > > > > >
>> > >> > > > > > Ping^2.
>> > >> > > > >
>> > >> > > > > Ping^3.
>> > >> > > >
>> > >> > > > Ping^4.  Adding some more global reviewers to CC.
>> > >> > >
>> > >> > > Looking at it now, sorry for the delay.
>> > >> >
>> > >> > Actually, the simple-object stuff is more Ian's area.  Looking at the other
>> > >> > part.
>> > >>
>> > >> Ian, ping -- we're through with the other part
>> > >> (https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00374.html).  The
>> > >> simple-object part is unchanged at
>> > >>
>> > >> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01541.html
>> > >>
>> > >> nearly unchanged from the post last year (which also includes
>> > >> some description):
>> > >>
>> > >> https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01292.html
>> > >
>> > > Ian, ping again!  (trying now also with the e-mail listed in
>> > > MAINTAINERS)
>> > >
>> > > https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01541.html
>> >
>> > Sorry about that, for some reason GMail didn't flag this message for me.
>> >
>> >
>> > > +  for (ent = buf + entsize; ent < buf + length; ent += entsize)
>> > > +    {
>> > > +      unsigned sec = type_functions->fetch_Elf_Word (ent);
>> > > +      if (pfnret[sec - 1] == 0)
>> > > +         keep = 1;
>> > > +    }
>> > > +  if (keep)
>> > > +    {
>> > > +      pfnret[sh_link - 1] = 0;
>> > > +      pfnret[i - 1] = 0;
>> > > +    }
>> > > + }
>> >
>> > It seems to me that if you keep any section of an SHT_GROUP, you need
>> > to keep all the sections.  Otherwise the section indexes in the group
>> > will be left dangling.
>>
>> Note that all sections are "kept", removed sections are just marked
>> as SHT_NULL.  Which means that indeed some group members might
>> be SHT_NULL.
>>
>> Note that I don't remember if I actually run into any SHT_GROUP
>> member being necessary when another is not (IIRC this was with
>> debug types sections or so).  I'll double-check and add a comment
>> where this matters for early-debug (the simple-object code tries
>> to be more generic but obviously all testing was done with
>> early-debug section copying in mind).
>
> That said, the callback is supposed to mark interesting stuff as not
> deleted.  The simple-object interface is only giving some leeway
> to simplify things (pulling in the container, directly dependent
> sections) to generate a valid ELF output.
>
>> > The symbol table handling is pretty awful.  Can't you just remove the
>> > symbols you don't want?  You will have to update the sh_info field of
>> > the SHT_SYMTAB section, which holds the index of the first STB_GLOBAL
>> > symbol in the table.
>>
>> Maybe that's what I was missing, let me see if I can make it work.
>> It looks like if there's no STB_GLOBAL symbol the index is one after
>> the last symbol and it seems it is the first ! STB_LOCAL symbol
>> ("One greater than the symbol table index of the last local symbol
>> (binding STB_LOCAL)").
>>
>> Ah, and implementing this now I remember why I chose the "awkward"
>> way.  This was to preserve symtab indices to not need to rewrite
>> relocation sections...  There isn't a documented way to make
>> an "none" symtab entry (eventually duplicating UND might work
>> but IIRC it messes up ordering as UND is STB_LOCAL and thus may
>> not appear after the first ! STB_LOCAL symbol).  As comments in
>> the code indicate I tried to mangle things in a way that makes
>> both GNU ld and gold happy...
>>
>> Any suggestion how to improve things when not removing symbols?
>> (I could of course remove things from the end of the symtab)
>> Like replacing local symbols with UND and globals with
>> a COMMON of zero size (and no name)?
>>
>> I suppose rewriting relocation sections is possible but I tried
>> to do as little as necessary (like not actually removing sections).
>
> So the following is an update which changes how I remove symbols
> by making removed locals copies of UND and removed globals
> global commons of size zero:
>
> Symbol table '.symtab' contains 22 entries:
>    Num:    Value          Size Type    Bind   Vis      Ndx Name
>      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>      1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS t.c
>      2: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>      3: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>      4: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>      5: 0000000000000000     0 SECTION LOCAL  DEFAULT    4
>      6: 0000000000000000     0 SECTION LOCAL  DEFAULT    6
>      7: 0000000000000000     0 SECTION LOCAL  DEFAULT    7
>      8: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
> ...
>     18: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
>     19: 0000000000000000     0 NOTYPE  WEAK   HIDDEN     4 t.c.39a678c9
>     20: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  COM
>     21: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  COM
>
> Does that look better?

As noted in crossing message, I think SHN_UNDEF is better than
SHN_COMMON.  Patch is OK with that change, assuming that works.

Ian