Re: [PATCH] Always keep sections marked with SEC_KEEP

[Alan Modra <amodra at gmail dot com>]

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;
 }

-- 
Alan Modra
Australia Development Lab, IBM