Re: [PATCH] bfd: support section groups with preceding SHF_GROUP sections

[jose dot marchesi at oracle dot com (Jose E. Marchesi)]

    > GAS always places section groups (SHT_GROUP) before the rest of the
    > sections in the output file.  However, other assemblers may place
    > section groups after the group members.
    > 
    > Example in the object attached, xdfdh.o:
    > 
    > COMDAT group section [   12] `.group.__cg_vis_memcpy' [__cg_vis_memcpy]
    > contains 1 sections:
    >    [Index]    Name
    >       [    4]   .text.__cg_vis_memcpy
    > 
    > Note how .text.__cg_vis_memcpy (idx 4) is placed before
    > .group.__cg_vis_memcpy (idx 12).
    > 
    > This is not handled properly by BFD.
    
    I'm sure that used to work at one point.  Hmm, by the look of it,
    broken by git commit 06614111d.  So the following would fix the
    problem.
    
    diff --git a/bfd/elf.c b/bfd/elf.c
    index bab1e16..3ffd409 100644
    --- a/bfd/elf.c
    +++ b/bfd/elf.c
    @@ -2424,7 +2424,7 @@ bfd_section_from_shdr (bfd *abfd, unsigned int shindex)
           if (hdr->contents != NULL)
     	{
     	  Elf_Internal_Group *idx = (Elf_Internal_Group *) hdr->contents;
    -	  unsigned int n_elt = hdr->sh_size / sizeof (* idx);
    +	  unsigned int n_elt = hdr->sh_size / GRP_ENTRY_SIZE;
     	  asection *s;
     
     	  if (n_elt == 0)
    
    But I like your patch better if you also delete all of the above
    hdr->contents != NULL code, which I think should then be unnecessary.
    Ok to commit with that change.  (Please check that what I'm
    suggesting is sane first!)

I think it makes all sense.  That chunk of code in bfd_section_from_shdr
basically:

a) Sets the SHT_GROUP section's linkage flags
   SEC_LINK_ONCE|SEC_LINK_DUPLICATES_DISCARD, and

b) Sets the SHT_GROUP section's next_in_group to point to the first
   section in the group.

Both a) and b) are performed by `setup_group' provided SHT_GROUP
sections are loaded before the SHF_GROUP sections contained in it.  The
patch assures that.

I also just noticed the old approach depends on the SHT_GROUP sections
to have the SHF_GROUP flag set.  GAS always sets that flag for group
sections, as apparently do the Solaris assembler, but AFAIK the ELF spec
doesn't mandate that.

I pushed the patch below after testing linking the problematic object in
sparc64-elf, and checking for regressions in sparc, mips and x86 elf
targets.  No regressions.

commit 78e8a2ff5f01799874be41fdc919d34c670496c6
Author: Jose E. Marchesi <jose.marchesi@oracle.com>
Date:   Tue Jun 6 11:19:06 2017 -0700

    bfd: support section groups with preceding SHF_GROUP sections
    
    GAS always places section groups (SHT_GROUP) before the rest of the
    sections in the output file.  However, other assemblers may place
    section groups after the group members.
    
    This patch fixes handlign such situations, and removes some duplicated
    logic.
    
    bfd/ChangeLog:
    
    2017-06-06  Jose E. Marchesi  <jose.marchesi@oracle.com>
    
    	* elf.c (setup_group): Make sure BFD sections are created for all
    	group sections in the input file when processing SHF_GROUP
    	sections.
    	(bfd_section_from_shdr): Avoid duplicating logic already
    	implemented in `setup_group'.

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 4992fd1..b828633 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,11 @@
+2017-06-06  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* elf.c (setup_group): Make sure BFD sections are created for all
+	group sections in the input file when processing SHF_GROUP
+	sections.
+	(bfd_section_from_shdr): Avoid duplicating logic already
+	implemented in `setup_group'.
+
 2017-06-06  Daniel Bonniot de Ruisselet  <bonniot@gmail.com>
 
 	PR binutils/21546
diff --git a/bfd/elf.c b/bfd/elf.c
index bab1e16..fb106e9 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -636,6 +636,11 @@ setup_group (bfd *abfd, Elf_Internal_Shdr *hdr, asection *newsect)
 		  unsigned char *src;
 		  Elf_Internal_Group *dest;
 
+                  /* Make sure the group section has a BFD section
+                     attached to it.  */
+                  if (!bfd_section_from_shdr (abfd, i))
+                    return FALSE;
+                  
 		  /* Add to list of sections.  */
 		  elf_tdata (abfd)->group_sect_ptr[num_group] = shdr;
 		  num_group += 1;
@@ -2421,34 +2426,6 @@ bfd_section_from_shdr (bfd *abfd, unsigned int shindex)
       if (!_bfd_elf_make_section_from_shdr (abfd, hdr, name, shindex))
 	goto fail;
 
-      if (hdr->contents != NULL)
-	{
-	  Elf_Internal_Group *idx = (Elf_Internal_Group *) hdr->contents;
-	  unsigned int n_elt = hdr->sh_size / sizeof (* idx);
-	  asection *s;
-
-	  if (n_elt == 0)
-	    goto fail;
-	  if (idx->flags & GRP_COMDAT)
-	    hdr->bfd_section->flags
-	      |= SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD;
-
-	  /* We try to keep the same section order as it comes in.  */
-	  idx += n_elt;
-
-	  while (--n_elt != 0)
-	    {
-	      --idx;
-
-	      if (idx->shdr != NULL
-		  && (s = idx->shdr->bfd_section) != NULL
-		  && elf_next_in_group (s) != NULL)
-		{
-		  elf_next_in_group (hdr->bfd_section) = s;
-		  break;
-		}
-	    }
-	}
       goto success;
 
     default: