PING: [PATCH] Don't claim a fat IR object if no IR object should be claimed

[H.J. Lu]

On Tue, Mar 26, 2024 at 3:36 AM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi H.J.
>
>    Sorry - I missed this one.
>
> >> When the linker sees an input object containing nothing but IR during
> >> rescan, it should ignore it (LTO phase is over).  But if the input object
> >> is a fat IR object, which has non-IR code as well, it should be used to
> >> resolve references as if it did not contain any IR at all.  This patch
> >> adds lto_type to bfd and linker avoids claiming a fat IR object if no IR
> >> object should be claimed.
>
> That makes sense to me.
>
> I have a few, minor comments on the patch itself:
>
> >> +enum bfd_lto_object_type
> >> +  {
> >> +    lto_non_object,
> >> +    lto_non_ir_object,
> >> +    lto_ir_object,
> >> +    lto_fat_ir_object
> >> +  };
>
> I think that it would be helpful to the reader of this code if there were
> comments explaining what each of these fields actually means.

Fixed.

>
> >> +         if (strncmp (sec->name, ".gnu.lto_", 9) == 0)
>
> With have a startswith() function, so it might as well be used here.

Fixed.

>
> >> +      /* FIXME: Check if it is a fat IR object.  */
> >> +      if (type == lto_ir_object)
>  >> +        {
>
> You could instead have:
>
>            abfd->lto_type = type;
>            if (type == lto_non_ir_object)
>              continue;
>
> This would reduce the level of indentation needed for the following
> code, which makes it easier to read (well in my opinion at least).
>
> I appreciate that this would mean that you will be assigning abfd->lto_type
> in two places, here and at the end of the fat IR detection code below, but
> it also means that with less indentation, you do not have to wrap some of
> code lines that follow.  This is just a suggestion however.
>
> >> +       {
> >> +         long symsize;
> >> +
> >> +         /* Get symbol table size.  */
> >> +         symsize = bfd_get_symtab_upper_bound (abfd);
> >> +         if (symsize > 0)
>
> Then similarly:
>
>               if (symsize < 1)
>                 continue;
>
>
> >> +                     if (name[0] == '_'
> >> +                         && name[1] == '_'
> >> +                         && strcmp (name + (name[2] == '_'),
> >> +                                    "__gnu_lto_slim") == 0)
>
> Clever coding, but hard to read, and not necessarily helpful it someone
> is trying to grep the codebase for references to ___gnu_lto_slim.  May I
> suggest adding a comment explaining what the if-statement is searching for ?
>

I changed it to set lto_type based on LTO bytecode information.

>
> >> -  if (link_info.lto_plugin_active)
> >> +  /* Don't claim a fat IR object if no IR object should be claimed.  */
> >> +  if (link_info.lto_plugin_active
> >> +      && (!no_more_claiming || abfd->lto_type != lto_fat_ir_object))
> >>       {
>
> Given that we have accessor functions for other fields in the BFD, shouldn't
> we have a bfd_get_lto_type() function ?

Added.

Here is the v2 patch:

https://patchwork.sourceware.org/project/binutils/list/?series=32247

OK for master?

Thanks.

-- 
H.J.