Re: [PATCH] Always keep sections marked with SEC_KEEP

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Oct 23, 2015 at 4:08 AM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Oct 23, 2015 at 03:47:38AM -0700, H.J. Lu wrote:
>> On Thu, Oct 22, 2015 at 4:51 PM, Alan Modra <amodra@gmail.com> wrote:
>> > On Thu, Oct 22, 2015 at 12:24:53PM -0700, H.J. Lu wrote:
>> >> diff --git a/bfd/elflink.c b/bfd/elflink.c
>> >> index 73fe469..06df821 100644
>> >> --- a/bfd/elflink.c
>> >> +++ b/bfd/elflink.c
>> >> @@ -12449,7 +12449,8 @@ elf_gc_sweep (bfd *abfd, struct bfd_link_info *info)
>> >>             o->gc_mark = first->gc_mark;
>> >>           }
>> >>
>> >> -       if (o->gc_mark)
>> >> +       /* Always keep sections marked with SEC_KEEP.  */
>> >> +       if (o->gc_mark || (o->flags & SEC_KEEP))
>> >>           continue;
>> >>
>> >>         /* Skip sweeping sections already excluded.  */
>> >
>> > This is wrong.  You're breaking the gc-sections model (and ignoring
>> > the special combination of both SEC_EXCLUDE and SEC_KEEP).  Sections
>> > with SEC_KEEP are supposed to have gc_mark set by the call to
>> > _bfd_elf_gc_mark in bfd_elf_gc_sections.
>>
>> Do you have a testcase to show the regression my patch caused?
>
> No, but my point about breaking the gc-sections model is that if you
> need to test SEC_KEEP in elf_gc_sweep then you have a section that has
> not had its relocations examined for further sections that might need
> to be kept.
>
>> If both SEC_EXCLUDE and SEC_KEEP are set, my patch doesn't
>> change the behavior of  elf_gc_sweep.
>
> Sorry, yes, you might be right about the combination of those flags.
>
> BTW, if we want to keep all input sections matching a __start_* or
> __stop_* symbol reference then something like the following should
> work.  What do you think?
>
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index eba32b5..df2d86a 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -2302,7 +2302,7 @@ extern asection *_bfd_elf_gc_mark_hook
>
>  extern asection *_bfd_elf_gc_mark_rsec
>    (struct bfd_link_info *, asection *, elf_gc_mark_hook_fn,
> -   struct elf_reloc_cookie *);
> +   struct elf_reloc_cookie *, bfd_boolean *);
>
>  extern bfd_boolean _bfd_elf_gc_mark_reloc
>    (struct bfd_link_info *, asection *, elf_gc_mark_hook_fn,
> diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
> index 7d65dae..e303189 100644
> --- a/bfd/elf-eh-frame.c
> +++ b/bfd/elf-eh-frame.c
> @@ -902,7 +902,8 @@ _bfd_elf_parse_eh_frame (bfd *abfd, struct bfd_link_info *info,
>               REQUIRE (GET_RELOC (buf));
>
>               /* Chain together the FDEs for each section.  */
> -             rsec = _bfd_elf_gc_mark_rsec (info, sec, gc_mark_hook, cookie);
> +             rsec = _bfd_elf_gc_mark_rsec (info, sec, gc_mark_hook,
> +                                           cookie, NULL);
>               /* RSEC will be NULL if FDE was cleared out as it was belonging to
>                  a discarded SHT_GROUP.  */
>               if (rsec)
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index 0bec93d..c3b537b 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -12074,8 +12074,6 @@ _bfd_elf_gc_mark_hook (asection *sec,
>                        struct elf_link_hash_entry *h,
>                        Elf_Internal_Sym *sym)
>  {
> -  const char *sec_name;
> -
>    if (h != NULL)
>      {
>        switch (h->root.type)
> @@ -12087,33 +12085,6 @@ _bfd_elf_gc_mark_hook (asection *sec,
>         case bfd_link_hash_common:
>           return h->root.u.c.p->section;
>
> -       case bfd_link_hash_undefined:
> -       case bfd_link_hash_undefweak:
> -         /* To work around a glibc bug, keep all XXX input sections
> -            when there is an as yet undefined reference to __start_XXX
> -            or __stop_XXX symbols.  The linker will later define such
> -            symbols for orphan input sections that have a name
> -            representable as a C identifier.  */
> -         if (strncmp (h->root.root.string, "__start_", 8) == 0)
> -           sec_name = h->root.root.string + 8;
> -         else if (strncmp (h->root.root.string, "__stop_", 7) == 0)
> -           sec_name = h->root.root.string + 7;
> -         else
> -           sec_name = NULL;
> -
> -         if (sec_name && *sec_name != '\0')
> -           {
> -             bfd *i;
> -
> -             for (i = info->input_bfds; i; i = i->link.next)
> -               {
> -                 sec = bfd_get_section_by_name (i, sec_name);
> -                 if (sec)
> -                   return sec;
> -               }
> -           }
> -         break;
> -
>         default:
>           break;
>         }
> @@ -12131,7 +12102,8 @@ _bfd_elf_gc_mark_hook (asection *sec,
>  asection *
>  _bfd_elf_gc_mark_rsec (struct bfd_link_info *info, asection *sec,
>                        elf_gc_mark_hook_fn gc_mark_hook,
> -                      struct elf_reloc_cookie *cookie)
> +                      struct elf_reloc_cookie *cookie,
> +                      bfd_boolean *start_stop)
>  {
>    unsigned long r_symndx;
>    struct elf_link_hash_entry *h;
> @@ -12160,6 +12132,38 @@ _bfd_elf_gc_mark_rsec (struct bfd_link_info *info, asection *sec,
>          handling copy relocs.  */
>        if (h->u.weakdef != NULL)
>         h->u.weakdef->mark = 1;
> +
> +      if (start_stop != NULL
> +         && (h->root.type == bfd_link_hash_undefined
> +             || h->root.type == bfd_link_hash_undefweak))
> +       {
> +         /* To work around a glibc bug, mark all XXX input sections
> +            when there is an as yet undefined reference to __start_XXX
> +            or __stop_XXX symbols.  The linker will later define such
> +            symbols for orphan input sections that have a name
> +            representable as a C identifier.  */
> +         const char *sec_name = NULL;
> +         if (strncmp (h->root.root.string, "__start_", 8) == 0)
> +           sec_name = h->root.root.string + 8;
> +         else if (strncmp (h->root.root.string, "__stop_", 7) == 0)
> +           sec_name = h->root.root.string + 7;
> +
> +         if (sec_name != NULL && *sec_name != '\0')
> +           {
> +             bfd *i;
> +
> +             for (i = info->input_bfds; i != NULL; i = i->link.next)
> +               {
> +                 asection *s = bfd_get_section_by_name (i, sec_name);
> +                 if (s != NULL && !s->gc_mark)
> +                   {
> +                     *start_stop = TRUE;
> +                     return s;
> +                   }
> +               }
> +           }
> +       }
> +
>        return (*gc_mark_hook) (sec, info, cookie->rel, h, NULL);
>      }
>
> @@ -12178,15 +12182,39 @@ _bfd_elf_gc_mark_reloc (struct bfd_link_info *info,
>                         struct elf_reloc_cookie *cookie)
>  {
>    asection *rsec;
> +  bfd_boolean start_stop = FALSE;
>
> -  rsec = _bfd_elf_gc_mark_rsec (info, sec, gc_mark_hook, cookie);
> -  if (rsec && !rsec->gc_mark)
> +  rsec = _bfd_elf_gc_mark_rsec (info, sec, gc_mark_hook, cookie, &start_stop);
> +  while (rsec != NULL)
>      {
> -      if (bfd_get_flavour (rsec->owner) != bfd_target_elf_flavour
> -         || (rsec->owner->flags & DYNAMIC) != 0)
> -       rsec->gc_mark = 1;
> -      else if (!_bfd_elf_gc_mark (info, rsec, gc_mark_hook))
> -       return FALSE;
> +      asection *s;
> +
> +      if (!rsec->gc_mark)
> +       {
> +         if (bfd_get_flavour (rsec->owner) != bfd_target_elf_flavour
> +             || (rsec->owner->flags & DYNAMIC) != 0)
> +           rsec->gc_mark = 1;
> +         else if (!_bfd_elf_gc_mark (info, rsec, gc_mark_hook))
> +           return FALSE;
> +       }
> +      if (!start_stop)
> +       break;
> +      s = bfd_get_next_section_by_name (rsec);
> +      if (s == NULL)
> +       {
> +         bfd *i = rsec->owner;
> +
> +         if (i != NULL)
> +           {
> +             while ((i = i->link.next) != NULL)
> +               {
> +                 s = bfd_get_section_by_name (i, rsec->name);
> +                 if (s != NULL)
> +                   break;
> +               }
> +           }
> +       }
> +      rsec = s;
>      }
>    return TRUE;
>  }
>

Can you try your change on the testcase in

https://sourceware.org/bugzilla/show_bug.cgi?id=19167

I will check in my testcase first.

-- 
H.J.