Remove the stripped group section from linker output

Message ID 20180212181340.GA22522@gmail.com
State New
Headers show
Series
  • Remove the stripped group section from linker output
Related show

Commit Message

H.J. Lu Feb. 12, 2018, 6:13 p.m.
GCC 7 and above generates .debug_macro section in COMDAT group with
-g3.  But ld fails to recognize that a group section shouldn't be in
output when all of its members are stripped.  Update ld to remove the
stripped group section from linker output when all members are removed
by moving the stripped group section logic from objcopy.c to bfd.c so
that "ld -r -S" behaves the same as "strip -g".

OK for master?

H.J.
--
bfd/

	PR ld/22836
	* bfd.c (bfd_stripped_group_section_p): New function.
	* elflink.c (bfd_elf_final_link): Remove the stripped group
	section from linker output.
	* bfd-in2.h: Regenerated.

binutils/

	PR ld/22836
	* objcopy.c (strip_group_signature_p): New function.
	(is_strip_section): Call bfd_stripped_group_section_p.

ld/

	PR ld/22836
	* testsuite/ld-elf/pr22836-1.s: New file.
	* testsuite/ld-elf/pr22836-1a.d: Likewise.
	* testsuite/ld-elf/pr22836-1b.d: Likewise.
---
 bfd/bfd-in2.h                    |  6 ++++
 bfd/bfd.c                        | 59 ++++++++++++++++++++++++++++++++++++++
 bfd/elflink.c                    |  6 ++++
 binutils/objcopy.c               | 62 +++++++++++++++++-----------------------
 ld/testsuite/ld-elf/pr22836-1.s  |  4 +++
 ld/testsuite/ld-elf/pr22836-1a.d | 15 ++++++++++
 ld/testsuite/ld-elf/pr22836-1b.d | 15 ++++++++++
 7 files changed, 132 insertions(+), 35 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr22836-1.s
 create mode 100644 ld/testsuite/ld-elf/pr22836-1a.d
 create mode 100644 ld/testsuite/ld-elf/pr22836-1b.d

-- 
2.14.3

Comments

Alan Modra Feb. 13, 2018, 2:01 a.m. | #1
On Mon, Feb 12, 2018 at 10:13:40AM -0800, H.J. Lu wrote:
> GCC 7 and above generates .debug_macro section in COMDAT group with

> -g3.  But ld fails to recognize that a group section shouldn't be in

> output when all of its members are stripped.  Update ld to remove the

> stripped group section from linker output when all members are removed

> by moving the stripped group section logic from objcopy.c to bfd.c so

> that "ld -r -S" behaves the same as "strip -g".

> 

> OK for master?


I think that moving most of objcopy.c:is_strip_section into bfd is a
bad idea.  You've ended up with an ugly interface with two callback
functions needed by objcopy, and a function with confusing parameter
names. 

> +bfd_boolean

> +bfd_stripped_group_section_p

> +  (bfd *abfd ATTRIBUTE_UNUSED, asection *sec,

> +   bfd_boolean relocatable_link,

> +   bfd_boolean (*strip_group_section_p) (bfd *, asection *),

> +   bfd_boolean (*strip_section_p) (bfd *, asection *))

> +{

> +  if ((bfd_get_section_flags (abfd, sec) & SEC_GROUP) != 0)

> +    {

> +      asection *elt, *first;

> +

> +      /* PR binutils/3181

> +	 If we are going to strip the group signature symbol, then

> +	 strip the group section too.  */

> +      if (!relocatable_link && strip_group_section_p (abfd, sec))

> +	return TRUE;


When I first looked at the patch I thought "won't that segfault during
a final link?", because I'd seen that you pass NULL for the callback
in bfd_elf_final_link but hadn't realized that relocatable_link was
always false.  So the name "relocatable_link" is a lie, but I think it
would be better to leave objcopy.c alone and write a small function in
elflink.c that simply iterates over the group elements.

static bfd_boolean
is_discarded_group (asection *sec)
{
  asection *elt, *first;

  if ((sec->flags & SEC_GROUP) == 0)
    return FALSE;

  first = elt = elf_next_in_group (sec);
  while (elt != NULL)
    {
      if (!discarded_section (elt))
	return FALSE;
      elt = elf_next_in_group (elt);
      if (elt == first)
	break;
    }
  return TRUE;
}

I also think it would be a good idea to set SEC_EXCLUDE for the
stripped section, just to be consistent with what happens with other
stripped sections.

-- 
Alan Modra
Australia Development Lab, IBM
H.J. Lu Feb. 13, 2018, 4:06 a.m. | #2
On Mon, Feb 12, 2018 at 6:01 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Feb 12, 2018 at 10:13:40AM -0800, H.J. Lu wrote:

>> GCC 7 and above generates .debug_macro section in COMDAT group with

>> -g3.  But ld fails to recognize that a group section shouldn't be in

>> output when all of its members are stripped.  Update ld to remove the

>> stripped group section from linker output when all members are removed

>> by moving the stripped group section logic from objcopy.c to bfd.c so

>> that "ld -r -S" behaves the same as "strip -g".

>>

>> OK for master?

>

> I think that moving most of objcopy.c:is_strip_section into bfd is a

> bad idea.  You've ended up with an ugly interface with two callback

> functions needed by objcopy, and a function with confusing parameter

> names.

>

>> +bfd_boolean

>> +bfd_stripped_group_section_p

>> +  (bfd *abfd ATTRIBUTE_UNUSED, asection *sec,

>> +   bfd_boolean relocatable_link,

>> +   bfd_boolean (*strip_group_section_p) (bfd *, asection *),

>> +   bfd_boolean (*strip_section_p) (bfd *, asection *))

>> +{

>> +  if ((bfd_get_section_flags (abfd, sec) & SEC_GROUP) != 0)

>> +    {

>> +      asection *elt, *first;

>> +

>> +      /* PR binutils/3181

>> +      If we are going to strip the group signature symbol, then

>> +      strip the group section too.  */

>> +      if (!relocatable_link && strip_group_section_p (abfd, sec))

>> +     return TRUE;

>

> When I first looked at the patch I thought "won't that segfault during

> a final link?", because I'd seen that you pass NULL for the callback

> in bfd_elf_final_link but hadn't realized that relocatable_link was

> always false.  So the name "relocatable_link" is a lie, but I think it

> would be better to leave objcopy.c alone and write a small function in

> elflink.c that simply iterates over the group elements.

>

> static bfd_boolean

> is_discarded_group (asection *sec)

> {

>   asection *elt, *first;

>

>   if ((sec->flags & SEC_GROUP) == 0)

>     return FALSE;

>

>   first = elt = elf_next_in_group (sec);

>   while (elt != NULL)

>     {

>       if (!discarded_section (elt))

>         return FALSE;

>       elt = elf_next_in_group (elt);

>       if (elt == first)

>         break;

>     }

>   return TRUE;

> }

>

> I also think it would be a good idea to set SEC_EXCLUDE for the

> stripped section, just to be consistent with what happens with other

> stripped sections.

>


Like this?


-- 
H.J.
From 3c126842464a5923c0c56e19036681a15c630a32 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 12 Feb 2018 08:27:30 -0800
Subject: [PATCH] Remove the stripped group section from linker output

GCC 7 and above generates .debug_macro section in COMDAT group with
-g3.  But ld fails to recognize that a group section shouldn't be in
output when all of its members are stripped.  Update ld to remove the
stripped group section from linker output when all members are removed.

bfd/

	PR ld/22836
	* elflink.c (is_discarded_group): New function.
	(bfd_elf_final_link): Remove the stripped group section from
	linker output.

ld/

	PR ld/22836
	* testsuite/ld-elf/pr22836-1.s: New file.
	* testsuite/ld-elf/pr22836-1a.d: Likewise.
	* testsuite/ld-elf/pr22836-1b.d: Likewise.
---
 bfd/elflink.c                    | 32 ++++++++++++++++++++++++++++++++
 ld/testsuite/ld-elf/pr22836-1.s  |  4 ++++
 ld/testsuite/ld-elf/pr22836-1a.d | 15 +++++++++++++++
 ld/testsuite/ld-elf/pr22836-1b.d | 15 +++++++++++++++
 4 files changed, 66 insertions(+)
 create mode 100644 ld/testsuite/ld-elf/pr22836-1.s
 create mode 100644 ld/testsuite/ld-elf/pr22836-1a.d
 create mode 100644 ld/testsuite/ld-elf/pr22836-1b.d

diff --git a/bfd/elflink.c b/bfd/elflink.c
index d1eb82020c..2fc26f529f 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -11512,6 +11512,31 @@ elf_final_link_free (bfd *obfd, struct elf_final_link_info *flinfo)
     }
 }
 
+/* Return TRUE if section SEC is a group section with all members
+   removed.  */
+
+static bfd_boolean
+is_discarded_group (asection *sec)
+{
+  asection *elt, *first;
+
+  if ((sec->flags & SEC_GROUP) == 0)
+    return FALSE;
+
+  /* Remove the group section if all members are removed.  */
+  first = elt = elf_next_in_group (sec);
+  while (elt != NULL)
+    {
+      if (!discarded_section (elt))
+	return FALSE;
+      elt = elf_next_in_group (elt);
+      if (elt == first)
+	break;
+    }
+
+  return TRUE;
+}
+
 /* Do the final step of an ELF link.  */
 
 bfd_boolean
@@ -11618,6 +11643,13 @@ bfd_elf_final_link (bfd *abfd, struct bfd_link_info *info)
 	  else
 	    o->flags |= SEC_EXCLUDE;
 	}
+      else if (is_discarded_group (o))
+	{
+	  /* Remove the stripped group section from linker output.  */
+	  o->flags |= SEC_EXCLUDE;
+	  bfd_section_list_remove (abfd, o);
+	  abfd->section_count--;
+	}
     }
 
   /* Count up the number of relocations we will output for each output
diff --git a/ld/testsuite/ld-elf/pr22836-1.s b/ld/testsuite/ld-elf/pr22836-1.s
new file mode 100644
index 0000000000..8be549ecca
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-1.s
@@ -0,0 +1,4 @@
+	.section	.debug_macro,"G",%progbits,foo,comdat
+	.long	.LASF0
+.LASF0:
+	.string	"__STDC__ 1"
diff --git a/ld/testsuite/ld-elf/pr22836-1a.d b/ld/testsuite/ld-elf/pr22836-1a.d
new file mode 100644
index 0000000000..5f8461f48e
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-1a.d
@@ -0,0 +1,15 @@
+#source: pr22836-1.s
+#ld: -r -s
+#readelf: -S --wide
+#notarget: alpha-*-* cr16-*-* crx-*-* d30v-*-* dlx-*-* i960-*-* pj*-*-*
+# Disabled on alpha because alpha has a different .set directive.
+# cr16 and crx use non-standard scripts with memory regions, which don't
+# play well with comdat group sections under ld -r.  Generic linker
+# targets don't support comdat group sections.
+
+#failif
+#...
+  \[[ 0-9]+\] \.debug_.* +.*
+#...
+COMDAT group section .*
+#...
diff --git a/ld/testsuite/ld-elf/pr22836-1b.d b/ld/testsuite/ld-elf/pr22836-1b.d
new file mode 100644
index 0000000000..20adc3a1f3
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-1b.d
@@ -0,0 +1,15 @@
+#source: pr22836-1.s
+#ld: -r -S
+#readelf: -S --wide
+#notarget: alpha-*-* cr16-*-* crx-*-* d30v-*-* dlx-*-* i960-*-* pj*-*-*
+# Disabled on alpha because alpha has a different .set directive.
+# cr16 and crx use non-standard scripts with memory regions, which don't
+# play well with comdat group sections under ld -r.  Generic linker
+# targets don't support comdat group sections.
+
+#failif
+#...
+  \[[ 0-9]+\] \.debug_.* +.*
+#...
+COMDAT group section .*
+#...
Alan Modra Feb. 13, 2018, 7:42 a.m. | #3
On Mon, Feb 12, 2018 at 08:06:29PM -0800, H.J. Lu wrote:
> Like this?


Yes, but I'm wondering now if this is really the correct patch.  We
have _bfd_elf_fixup_group_sections which is supposed to adjust
SHT_GROUP size when group member sections are discarded.  If the size
went down to 4, then you could set SEC_EXCLUDE for the SHT_GROUP
section.

However, it looks like that function doesn't handle group sections
with relocs properly.  Fixing that isn't hard, but then I run into
other problems when setting SEC_EXCLUDE.  :-(

So I guess you might as well commit your patch, and when/if I sort out
the other problems I'll remove is_discarded_group.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra Feb. 13, 2018, 9:58 a.m. | #4
On Tue, Feb 13, 2018 at 06:12:34PM +1030, Alan Modra wrote:
> On Mon, Feb 12, 2018 at 08:06:29PM -0800, H.J. Lu wrote:

> > Like this?

> 

> Yes, but I'm wondering now if this is really the correct patch.  We

> have _bfd_elf_fixup_group_sections which is supposed to adjust

> SHT_GROUP size when group member sections are discarded.  If the size

> went down to 4, then you could set SEC_EXCLUDE for the SHT_GROUP

> section.

> 

> However, it looks like that function doesn't handle group sections

> with relocs properly.  Fixing that isn't hard, but then I run into

> other problems when setting SEC_EXCLUDE.  :-(

> 

> So I guess you might as well commit your patch, and when/if I sort out

> the other problems I'll remove is_discarded_group.


This is what I'm currently testing.  I intend to add some objcopy
and ld -r tests that check full and partial removal of group sections.

	* elf.c (_bfd_elf_fixup_group_sections): Account for removed
	relocation sections.  If size reduces to just the flag word,
	remove that too and mark with SEC_EXCLUDE.
	* elflink.c (bfd_elf_final_link): Strip empty group sections.

diff --git a/bfd/elf.c b/bfd/elf.c
index 0503154..934052d 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -7604,7 +7604,16 @@ _bfd_elf_fixup_group_sections (bfd *ibfd, asection *discarded)
 	       but the SHT_GROUP section is, then adjust its size.  */
 	    else if (s->output_section == discarded
 		     && isec->output_section != discarded)
-	      removed += 4;
+	      {
+		struct bfd_elf_section_data *elf_sec = elf_section_data (s);
+		removed += 4;
+		if (elf_sec->rel.hdr != NULL
+		    && (elf_sec->rel.hdr->sh_flags & SHF_GROUP) != 0)
+		  removed += 4;
+		if (elf_sec->rela.hdr != NULL
+		    && (elf_sec->rela.hdr->sh_flags & SHF_GROUP) != 0)
+		  removed += 4;
+	      }
 	    s = elf_next_in_group (s);
 	    if (s == first)
 	      break;
@@ -7614,18 +7623,26 @@ _bfd_elf_fixup_group_sections (bfd *ibfd, asection *discarded)
 	    if (discarded != NULL)
 	      {
 		/* If we've been called for ld -r, then we need to
-		   adjust the input section size.  This function may
-		   be called multiple times, so save the original
-		   size.  */
+		   adjust the input section size.  */
 		if (isec->rawsize == 0)
 		  isec->rawsize = isec->size;
 		isec->size = isec->rawsize - removed;
+		if (isec->size <= 4)
+		  {
+		    isec->size = 0;
+		    isec->flags |= SEC_EXCLUDE;
+		  }
 	      }
 	    else
 	      {
 		/* Adjust the output section size when called from
 		   objcopy. */
 		isec->output_section->size -= removed;
+		if (isec->output_section->size <= 4)
+		  {
+		    isec->output_section->size = 0;
+		    isec->output_section->flags |= SEC_EXCLUDE;
+		  }
 	      }
 	  }
       }
diff --git a/bfd/elflink.c b/bfd/elflink.c
index d1eb820..6eb47ee 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -11618,6 +11618,13 @@ bfd_elf_final_link (bfd *abfd, struct bfd_link_info *info)
 	  else
 	    o->flags |= SEC_EXCLUDE;
 	}
+      else if ((o->flags & SEC_GROUP) != 0 && o->size == 0)
+	{
+	  /* Remove empty group section from linker output.  */
+	  o->flags |= SEC_EXCLUDE;
+	  bfd_section_list_remove (abfd, o);
+	  abfd->section_count--;
+	}
     }
 
   /* Count up the number of relocations we will output for each output

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra Feb. 13, 2018, 12:29 p.m. | #5
On Tue, Feb 13, 2018 at 08:28:27PM +1030, Alan Modra wrote:
> This is what I'm currently testing.  I intend to add some objcopy

> and ld -r tests that check full and partial removal of group sections.


Committed.  Please push your testcases.

bfd/
	PR 22836
	* elf.c (_bfd_elf_fixup_group_sections): Account for removed
	relocation sections.  If size reduces to just the flag word,
	remove that too and mark with SEC_EXCLUDE.
	* elflink.c (bfd_elf_final_link): Strip empty group sections.
binutils/
	* testsuite/binutils-all/group-7.s,
	* testsuite/binutils-all/group-7a.d,
	* testsuite/binutils-all/group-7b.d,
	* testsuite/binutils-all/group-7c.d: New tests.
	* testsuite/binutils-all/objcopy.exp: Run them.
ld/
	* testsuite/ld-elf/pr22836-2.d,
	* testsuite/ld-elf/pr22836-2.s: New test.

diff --git a/bfd/elf.c b/bfd/elf.c
index 0503154..934052d 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -7604,7 +7604,16 @@ _bfd_elf_fixup_group_sections (bfd *ibfd, asection *discarded)
 	       but the SHT_GROUP section is, then adjust its size.  */
 	    else if (s->output_section == discarded
 		     && isec->output_section != discarded)
-	      removed += 4;
+	      {
+		struct bfd_elf_section_data *elf_sec = elf_section_data (s);
+		removed += 4;
+		if (elf_sec->rel.hdr != NULL
+		    && (elf_sec->rel.hdr->sh_flags & SHF_GROUP) != 0)
+		  removed += 4;
+		if (elf_sec->rela.hdr != NULL
+		    && (elf_sec->rela.hdr->sh_flags & SHF_GROUP) != 0)
+		  removed += 4;
+	      }
 	    s = elf_next_in_group (s);
 	    if (s == first)
 	      break;
@@ -7614,18 +7623,26 @@ _bfd_elf_fixup_group_sections (bfd *ibfd, asection *discarded)
 	    if (discarded != NULL)
 	      {
 		/* If we've been called for ld -r, then we need to
-		   adjust the input section size.  This function may
-		   be called multiple times, so save the original
-		   size.  */
+		   adjust the input section size.  */
 		if (isec->rawsize == 0)
 		  isec->rawsize = isec->size;
 		isec->size = isec->rawsize - removed;
+		if (isec->size <= 4)
+		  {
+		    isec->size = 0;
+		    isec->flags |= SEC_EXCLUDE;
+		  }
 	      }
 	    else
 	      {
 		/* Adjust the output section size when called from
 		   objcopy. */
 		isec->output_section->size -= removed;
+		if (isec->output_section->size <= 4)
+		  {
+		    isec->output_section->size = 0;
+		    isec->output_section->flags |= SEC_EXCLUDE;
+		  }
 	      }
 	  }
       }
diff --git a/bfd/elflink.c b/bfd/elflink.c
index d1eb820..6eb47ee 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -11618,6 +11618,13 @@ bfd_elf_final_link (bfd *abfd, struct bfd_link_info *info)
 	  else
 	    o->flags |= SEC_EXCLUDE;
 	}
+      else if ((o->flags & SEC_GROUP) != 0 && o->size == 0)
+	{
+	  /* Remove empty group section from linker output.  */
+	  o->flags |= SEC_EXCLUDE;
+	  bfd_section_list_remove (abfd, o);
+	  abfd->section_count--;
+	}
     }
 
   /* Count up the number of relocations we will output for each output
diff --git a/binutils/testsuite/binutils-all/group-7.s b/binutils/testsuite/binutils-all/group-7.s
new file mode 100644
index 0000000..5028afc
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7.s
@@ -0,0 +1,6 @@
+	.section        .data.foo,"awG",%progbits,foo,comdat
+here:
+	.dc.a	here
+
+	.section        .data2.foo,"awG",%progbits,foo,comdat
+	.dc.a	0
diff --git a/binutils/testsuite/binutils-all/group-7a.d b/binutils/testsuite/binutils-all/group-7a.d
new file mode 100644
index 0000000..fa8db60
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7a.d
@@ -0,0 +1,16 @@
+#name: copy removing reloc group member
+#source: group-7.s
+#PROG: objcopy
+#DUMPPROG: readelf
+#objcopy: --remove-section .data.foo
+#readelf: -Sg --wide
+
+#...
+  \[[ 0-9]+\] \.group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data2\.foo[ \t]+PROGBITS[ \t0-9a-f]+WAG.*
+#...
+COMDAT group section \[[ 0-9]+\] `\.group' \[foo\] contains 1 section.*
+   \[Index\]    Name
+   \[[ 0-9]+\]   \.data2\.foo
+#pass
diff --git a/binutils/testsuite/binutils-all/group-7b.d b/binutils/testsuite/binutils-all/group-7b.d
new file mode 100644
index 0000000..b674545
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7b.d
@@ -0,0 +1,19 @@
+#name: copy removing non-reloc group member
+#source: group-7.s
+#PROG: objcopy
+#DUMPPROG: readelf
+#objcopy: --remove-section .data2.foo
+#readelf: -Sg --wide
+
+#...
+  \[[ 0-9]+\] \.group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data\.foo[ \t]+PROGBITS[ \t0-9a-f]+WAG.*
+#...
+  \[[ 0-9]+\] \.rela?\.data\.foo[ \t]+RELA?[ \t0-9a-f]+IG.*
+#...
+COMDAT group section \[[ 0-9]+\] `\.group' \[foo\] contains 2 sections:
+   \[Index\]    Name
+   \[[ 0-9]+\]   \.data\.foo
+   \[[ 0-9]+\]   \.rela?\.data\.foo
+#pass
diff --git a/binutils/testsuite/binutils-all/group-7c.d b/binutils/testsuite/binutils-all/group-7c.d
new file mode 100644
index 0000000..83e9115
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7c.d
@@ -0,0 +1,8 @@
+#name: copy removing reloc and non-reloc group member
+#source: group-7.s
+#PROG: objcopy
+#DUMPPROG: readelf
+#objcopy: -R .data.foo -R .data2.foo
+#readelf: -g --wide
+
+There are no section groups in this file\.
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index 377f88c..f4a7692 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1051,6 +1051,9 @@ if [is_elf_format] {
     objcopy_test_readelf "GNU_MBIND section" mbind1.s
     run_dump_test "group-5"
     run_dump_test "group-6"
+    run_dump_test "group-7a"
+    run_dump_test "group-7b"
+    run_dump_test "group-7c"
     run_dump_test "copy-1"
     run_dump_test "note-1"
     if [is_elf64 tmpdir/bintest.o] {
diff --git a/ld/testsuite/ld-elf/pr22836-2.d b/ld/testsuite/ld-elf/pr22836-2.d
new file mode 100644
index 0000000..10133e4
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-2.d
@@ -0,0 +1,7 @@
+#source: pr22836-2.s
+#ld: -r -S
+#readelf: -g --wide
+
+group section \[[ 0-9]+\] `\.group' \[foo\] contains 1 section.*
+   \[Index\]    Name
+   \[[ 0-9]+\]   \.comment
diff --git a/ld/testsuite/ld-elf/pr22836-2.s b/ld/testsuite/ld-elf/pr22836-2.s
new file mode 100644
index 0000000..77cd83a
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-2.s
@@ -0,0 +1,7 @@
+	.section	.debug_macro,"G",%progbits,foo
+	.long	.LASF0
+.LASF0:
+	.string	"__STDC__ 1"
+
+	.section	.comment,"G",%progbits,foo
+	.asciz "hi"

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra Feb. 14, 2018, 1:50 a.m. | #6
On Tue, Feb 13, 2018 at 10:59:40PM +1030, Alan Modra wrote:
> Committed.  Please push your testcases.


Never mind, I've done so for you, with some improvements to the .d
files.  git commit 60f763ee16f.

-- 
Alan Modra
Australia Development Lab, IBM
H.J. Lu March 19, 2018, 1:26 p.m. | #7
On Tue, Feb 13, 2018 at 5:50 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Feb 13, 2018 at 10:59:40PM +1030, Alan Modra wrote:

>> Committed.  Please push your testcases.

>

> Never mind, I've done so for you, with some improvements to the .d

> files.  git commit 60f763ee16f.

>


There is a request to backport the fix to 2.30 branch.

-- 
H.J.
Nick Clifton March 19, 2018, 1:48 p.m. | #8
Hi H.J. 

> There is a request to backport the fix to 2.30 branch.

This is OK with me.

Cheers
  Nick
H.J. Lu March 19, 2018, 3:10 p.m. | #9
On Mon, Mar 19, 2018 at 6:48 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi H.J.

>

>> There is a request to backport the fix to 2.30 branch.

> This is OK with me.

>


This are what I checked into 2.30 branch.

Thanks.

-- 
H.J.
From d957f81cb38d7e82ae546cd03265ee3087ba8a85 Mon Sep 17 00:00:00 2001
From: Alan Modra <amodra@gmail.com>
Date: Tue, 13 Feb 2018 14:09:48 +1030
Subject: [PATCH 1/2] PR22836, "-r -s" doesn't work with -g3 using GCC 7

This fixes the case where all of a group is removed with ld -r, the
situation in the PR, and failures where part of a group is removed
that contain relocs.

bfd/
	PR 22836
	* elf.c (_bfd_elf_fixup_group_sections): Account for removed
	relocation sections.  If size reduces to just the flag word,
	remove that too and mark with SEC_EXCLUDE.
	* elflink.c (bfd_elf_final_link): Strip empty group sections.
binutils/
	* testsuite/binutils-all/group-7.s,
	* testsuite/binutils-all/group-7a.d,
	* testsuite/binutils-all/group-7b.d,
	* testsuite/binutils-all/group-7c.d: New tests.
	* testsuite/binutils-all/objcopy.exp: Run them.
ld/
	* testsuite/ld-elf/pr22836-2.d,
	* testsuite/ld-elf/pr22836-2.s: New test.

(cherry picked from commit 6e5e9d58c1eeef5677c90886578a895cb8c164c5)
---
 bfd/ChangeLog                               | 11 +++++++++++
 bfd/elf.c                                   | 25 +++++++++++++++++++++----
 bfd/elflink.c                               |  7 +++++++
 binutils/ChangeLog                          | 12 ++++++++++++
 binutils/testsuite/binutils-all/group-7.s   |  6 ++++++
 binutils/testsuite/binutils-all/group-7a.d  | 16 ++++++++++++++++
 binutils/testsuite/binutils-all/group-7b.d  | 19 +++++++++++++++++++
 binutils/testsuite/binutils-all/group-7c.d  |  8 ++++++++
 binutils/testsuite/binutils-all/objcopy.exp |  3 +++
 ld/ChangeLog                                |  9 +++++++++
 ld/testsuite/ld-elf/pr22836-2.d             |  7 +++++++
 ld/testsuite/ld-elf/pr22836-2.s             |  7 +++++++
 12 files changed, 126 insertions(+), 4 deletions(-)
 create mode 100644 binutils/testsuite/binutils-all/group-7.s
 create mode 100644 binutils/testsuite/binutils-all/group-7a.d
 create mode 100644 binutils/testsuite/binutils-all/group-7b.d
 create mode 100644 binutils/testsuite/binutils-all/group-7c.d
 create mode 100644 ld/testsuite/ld-elf/pr22836-2.d
 create mode 100644 ld/testsuite/ld-elf/pr22836-2.s

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 6d1a9678a4..13ccbc5eb0 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,14 @@
+2018-03-19  H.J. Lu  <hongjiu.lu@intel.com>
+
+	Backport from master branch
+	2018-02-13  Alan Modra  <amodra@gmail.com>
+
+	PR 22836
+	* elf.c (_bfd_elf_fixup_group_sections): Account for removed
+	relocation sections.  If size reduces to just the flag word,
+	remove that too and mark with SEC_EXCLUDE.
+	* elflink.c (bfd_elf_final_link): Strip empty group sections.
+
 2018-03-14  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR ld/20882
diff --git a/bfd/elf.c b/bfd/elf.c
index 325bdd545a..e95c8a9c83 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -7579,7 +7579,16 @@ _bfd_elf_fixup_group_sections (bfd *ibfd, asection *discarded)
 	       but the SHT_GROUP section is, then adjust its size.  */
 	    else if (s->output_section == discarded
 		     && isec->output_section != discarded)
-	      removed += 4;
+	      {
+		struct bfd_elf_section_data *elf_sec = elf_section_data (s);
+		removed += 4;
+		if (elf_sec->rel.hdr != NULL
+		    && (elf_sec->rel.hdr->sh_flags & SHF_GROUP) != 0)
+		  removed += 4;
+		if (elf_sec->rela.hdr != NULL
+		    && (elf_sec->rela.hdr->sh_flags & SHF_GROUP) != 0)
+		  removed += 4;
+	      }
 	    s = elf_next_in_group (s);
 	    if (s == first)
 	      break;
@@ -7589,18 +7598,26 @@ _bfd_elf_fixup_group_sections (bfd *ibfd, asection *discarded)
 	    if (discarded != NULL)
 	      {
 		/* If we've been called for ld -r, then we need to
-		   adjust the input section size.  This function may
-		   be called multiple times, so save the original
-		   size.  */
+		   adjust the input section size.  */
 		if (isec->rawsize == 0)
 		  isec->rawsize = isec->size;
 		isec->size = isec->rawsize - removed;
+		if (isec->size <= 4)
+		  {
+		    isec->size = 0;
+		    isec->flags |= SEC_EXCLUDE;
+		  }
 	      }
 	    else
 	      {
 		/* Adjust the output section size when called from
 		   objcopy. */
 		isec->output_section->size -= removed;
+		if (isec->output_section->size <= 4)
+		  {
+		    isec->output_section->size = 0;
+		    isec->output_section->flags |= SEC_EXCLUDE;
+		  }
 	      }
 	  }
       }
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 72aa3ac9c2..69cb5abbac 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -11618,6 +11618,13 @@ bfd_elf_final_link (bfd *abfd, struct bfd_link_info *info)
 	  else
 	    o->flags |= SEC_EXCLUDE;
 	}
+      else if ((o->flags & SEC_GROUP) != 0 && o->size == 0)
+	{
+	  /* Remove empty group section from linker output.  */
+	  o->flags |= SEC_EXCLUDE;
+	  bfd_section_list_remove (abfd, o);
+	  abfd->section_count--;
+	}
     }
 
   /* Count up the number of relocations we will output for each output
diff --git a/binutils/ChangeLog b/binutils/ChangeLog
index 674b9f9d88..0eba8440e8 100644
--- a/binutils/ChangeLog
+++ b/binutils/ChangeLog
@@ -1,3 +1,15 @@
+2018-03-19  H.J. Lu  <hongjiu.lu@intel.com>
+
+	Backport from master branch
+	2018-02-13  Alan Modra  <amodra@gmail.com>
+
+	PR 22836
+	* testsuite/binutils-all/group-7.s,
+	* testsuite/binutils-all/group-7a.d,
+	* testsuite/binutils-all/group-7b.d,
+	* testsuite/binutils-all/group-7c.d: New tests.
+	* testsuite/binutils-all/objcopy.exp: Run them.
+
 2018-02-27  Nick Clifton  <nickc@redhat.com>
 
 	* po/pt.po: New Portuguese translation.
diff --git a/binutils/testsuite/binutils-all/group-7.s b/binutils/testsuite/binutils-all/group-7.s
new file mode 100644
index 0000000000..5028afc1f5
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7.s
@@ -0,0 +1,6 @@
+	.section        .data.foo,"awG",%progbits,foo,comdat
+here:
+	.dc.a	here
+
+	.section        .data2.foo,"awG",%progbits,foo,comdat
+	.dc.a	0
diff --git a/binutils/testsuite/binutils-all/group-7a.d b/binutils/testsuite/binutils-all/group-7a.d
new file mode 100644
index 0000000000..fa8db60d9e
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7a.d
@@ -0,0 +1,16 @@
+#name: copy removing reloc group member
+#source: group-7.s
+#PROG: objcopy
+#DUMPPROG: readelf
+#objcopy: --remove-section .data.foo
+#readelf: -Sg --wide
+
+#...
+  \[[ 0-9]+\] \.group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data2\.foo[ \t]+PROGBITS[ \t0-9a-f]+WAG.*
+#...
+COMDAT group section \[[ 0-9]+\] `\.group' \[foo\] contains 1 section.*
+   \[Index\]    Name
+   \[[ 0-9]+\]   \.data2\.foo
+#pass
diff --git a/binutils/testsuite/binutils-all/group-7b.d b/binutils/testsuite/binutils-all/group-7b.d
new file mode 100644
index 0000000000..b674545362
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7b.d
@@ -0,0 +1,19 @@
+#name: copy removing non-reloc group member
+#source: group-7.s
+#PROG: objcopy
+#DUMPPROG: readelf
+#objcopy: --remove-section .data2.foo
+#readelf: -Sg --wide
+
+#...
+  \[[ 0-9]+\] \.group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data\.foo[ \t]+PROGBITS[ \t0-9a-f]+WAG.*
+#...
+  \[[ 0-9]+\] \.rela?\.data\.foo[ \t]+RELA?[ \t0-9a-f]+IG.*
+#...
+COMDAT group section \[[ 0-9]+\] `\.group' \[foo\] contains 2 sections:
+   \[Index\]    Name
+   \[[ 0-9]+\]   \.data\.foo
+   \[[ 0-9]+\]   \.rela?\.data\.foo
+#pass
diff --git a/binutils/testsuite/binutils-all/group-7c.d b/binutils/testsuite/binutils-all/group-7c.d
new file mode 100644
index 0000000000..83e91156ee
--- /dev/null
+++ b/binutils/testsuite/binutils-all/group-7c.d
@@ -0,0 +1,8 @@
+#name: copy removing reloc and non-reloc group member
+#source: group-7.s
+#PROG: objcopy
+#DUMPPROG: readelf
+#objcopy: -R .data.foo -R .data2.foo
+#readelf: -g --wide
+
+There are no section groups in this file\.
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index 377f88c0e1..f4a7692cdf 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1051,6 +1051,9 @@ if [is_elf_format] {
     objcopy_test_readelf "GNU_MBIND section" mbind1.s
     run_dump_test "group-5"
     run_dump_test "group-6"
+    run_dump_test "group-7a"
+    run_dump_test "group-7b"
+    run_dump_test "group-7c"
     run_dump_test "copy-1"
     run_dump_test "note-1"
     if [is_elf64 tmpdir/bintest.o] {
diff --git a/ld/ChangeLog b/ld/ChangeLog
index b5cf58ace2..f47f7c8373 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,12 @@
+2018-03-19  H.J. Lu  <hongjiu.lu@intel.com>
+
+	Backport from master branch
+	2018-02-13  Alan Modra  <amodra@gmail.com>
+
+	PR 22836
+	* testsuite/ld-elf/pr22836-2.d,
+	* testsuite/ld-elf/pr22836-2.s: New test.
+
 2018-03-14  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR ld/20882
diff --git a/ld/testsuite/ld-elf/pr22836-2.d b/ld/testsuite/ld-elf/pr22836-2.d
new file mode 100644
index 0000000000..10133e4b90
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-2.d
@@ -0,0 +1,7 @@
+#source: pr22836-2.s
+#ld: -r -S
+#readelf: -g --wide
+
+group section \[[ 0-9]+\] `\.group' \[foo\] contains 1 section.*
+   \[Index\]    Name
+   \[[ 0-9]+\]   \.comment
diff --git a/ld/testsuite/ld-elf/pr22836-2.s b/ld/testsuite/ld-elf/pr22836-2.s
new file mode 100644
index 0000000000..77cd83a0c6
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-2.s
@@ -0,0 +1,7 @@
+	.section	.debug_macro,"G",%progbits,foo
+	.long	.LASF0
+.LASF0:
+	.string	"__STDC__ 1"
+
+	.section	.comment,"G",%progbits,foo
+	.asciz "hi"

Patch

diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 42991e7848..85d4a3e195 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -7282,6 +7282,12 @@  bfd_boolean bfd_convert_section_contents
    (bfd *ibfd, asection *isec, bfd *obfd,
     bfd_byte **ptr, bfd_size_type *ptr_size);
 
+bfd_boolean bfd_stripped_group_section_p
+   (bfd *abfd, asection *sec,
+    bfd_boolean relocatable_link,
+    bfd_boolean (*strip_group_section_p) (bfd *, asection *),
+    bfd_boolean (*strip_section_p) (bfd *, asection *));
+
 /* Extracted from archive.c.  */
 symindex bfd_get_next_mapent
    (bfd *abfd, symindex previous, carsym **sym);
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 985c825c69..9603868fee 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -2613,3 +2613,62 @@  bfd_convert_section_contents (bfd *ibfd, sec_ptr isec, bfd *obfd,
   *ptr_size = size;
   return TRUE;
 }
+
+/*
+FUNCTION
+	bfd_stripped_group_section_p
+
+SYNOPSIS
+	bfd_boolean bfd_stripped_group_section_p
+	  (bfd *abfd, asection *sec,
+	   bfd_boolean relocatable_link,
+	   bfd_boolean (*strip_group_section_p) (bfd *, asection *),
+	   bfd_boolean (*strip_section_p) (bfd *, asection *));
+
+DESCRIPTION
+	Return TRUE if the section @var{sec} in BFD @var{abfd} is a
+	group section and should be stripped.  The group section is
+	stripped when all members are removed.  For non-relocatable
+	link, when @var{relocatable_link} is FALSE, the group section
+	is stripped if @var{strip_group_section_p} returns TRUE and a
+	group member is removed if @var{strip_section_p} returns TRUE.
+*/
+
+bfd_boolean
+bfd_stripped_group_section_p
+  (bfd *abfd ATTRIBUTE_UNUSED, asection *sec,
+   bfd_boolean relocatable_link,
+   bfd_boolean (*strip_group_section_p) (bfd *, asection *),
+   bfd_boolean (*strip_section_p) (bfd *, asection *))
+{
+  if ((bfd_get_section_flags (abfd, sec) & SEC_GROUP) != 0)
+    {
+      asection *elt, *first;
+
+      /* PR binutils/3181
+	 If we are going to strip the group signature symbol, then
+	 strip the group section too.  */
+      if (!relocatable_link && strip_group_section_p (abfd, sec))
+	return TRUE;
+
+      /* Remove the group section if all members are removed.  */
+      first = elt = elf_next_in_group (sec);
+      while (elt != NULL)
+	{
+	  if (relocatable_link)
+	    {
+	      if (!discarded_section (elt))
+		return FALSE;
+	    }
+	  else if (!strip_section_p (abfd, elt))
+	    return FALSE;
+	  elt = elf_next_in_group (elt);
+	  if (elt == first)
+	    break;
+	}
+
+      return TRUE;
+    }
+
+  return FALSE;
+}
diff --git a/bfd/elflink.c b/bfd/elflink.c
index d1eb82020c..5e85e25c92 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -11618,6 +11618,12 @@  bfd_elf_final_link (bfd *abfd, struct bfd_link_info *info)
 	  else
 	    o->flags |= SEC_EXCLUDE;
 	}
+      else if (bfd_stripped_group_section_p (abfd, o, TRUE, NULL, NULL))
+	{
+	  /* Remove the stripped group section from linker output.  */
+	  bfd_section_list_remove (abfd, o);
+	  abfd->section_count--;
+	}
     }
 
   /* Count up the number of relocations we will output for each output
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 8cdf27a87e..59f9d50b17 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -1330,48 +1330,40 @@  is_strip_section_1 (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
   return FALSE;
 }
 
-/* See if a section is being removed.  */
+/* Return TRUE if the group signature should be stripped.  */
 
 static bfd_boolean
-is_strip_section (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
+strip_group_signature_p (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
 {
-  if (is_strip_section_1 (abfd, sec))
+  asymbol *gsym;
+  const char *gname;
+
+  /* PR binutils/3181
+     If we are going to strip the group signature symbol, then
+     strip the group section too.  */
+  gsym = group_signature (sec);
+  if (gsym != NULL)
+    gname = gsym->name;
+  else
+    gname = sec->name;
+  if ((strip_symbols == STRIP_ALL
+       && !is_specified_symbol (gname, keep_specific_htab))
+      || is_specified_symbol (gname, strip_specific_htab))
     return TRUE;
 
-  if ((bfd_get_section_flags (abfd, sec) & SEC_GROUP) != 0)
-    {
-      asymbol *gsym;
-      const char *gname;
-      asection *elt, *first;
-
-      /* PR binutils/3181
-	 If we are going to strip the group signature symbol, then
-	 strip the group section too.  */
-      gsym = group_signature (sec);
-      if (gsym != NULL)
-	gname = gsym->name;
-      else
-	gname = sec->name;
-      if ((strip_symbols == STRIP_ALL
-	   && !is_specified_symbol (gname, keep_specific_htab))
-	  || is_specified_symbol (gname, strip_specific_htab))
-	return TRUE;
-
-      /* Remove the group section if all members are removed.  */
-      first = elt = elf_next_in_group (sec);
-      while (elt != NULL)
-	{
-	  if (!is_strip_section_1 (abfd, elt))
-	    return FALSE;
-	  elt = elf_next_in_group (elt);
-	  if (elt == first)
-	    break;
-	}
+  return FALSE;
+}
 
-      return TRUE;
-    }
+/* See if a section is being removed.  */
 
-  return FALSE;
+static bfd_boolean
+is_strip_section (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
+{
+  if (is_strip_section_1 (abfd, sec))
+    return TRUE;
+  return bfd_stripped_group_section_p (abfd, sec, FALSE,
+				       strip_group_signature_p,
+				       is_strip_section_1);
 }
 
 static bfd_boolean
diff --git a/ld/testsuite/ld-elf/pr22836-1.s b/ld/testsuite/ld-elf/pr22836-1.s
new file mode 100644
index 0000000000..8be549ecca
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-1.s
@@ -0,0 +1,4 @@ 
+	.section	.debug_macro,"G",%progbits,foo,comdat
+	.long	.LASF0
+.LASF0:
+	.string	"__STDC__ 1"
diff --git a/ld/testsuite/ld-elf/pr22836-1a.d b/ld/testsuite/ld-elf/pr22836-1a.d
new file mode 100644
index 0000000000..5f8461f48e
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-1a.d
@@ -0,0 +1,15 @@ 
+#source: pr22836-1.s
+#ld: -r -s
+#readelf: -S --wide
+#notarget: alpha-*-* cr16-*-* crx-*-* d30v-*-* dlx-*-* i960-*-* pj*-*-*
+# Disabled on alpha because alpha has a different .set directive.
+# cr16 and crx use non-standard scripts with memory regions, which don't
+# play well with comdat group sections under ld -r.  Generic linker
+# targets don't support comdat group sections.
+
+#failif
+#...
+  \[[ 0-9]+\] \.debug_.* +.*
+#...
+COMDAT group section .*
+#...
diff --git a/ld/testsuite/ld-elf/pr22836-1b.d b/ld/testsuite/ld-elf/pr22836-1b.d
new file mode 100644
index 0000000000..20adc3a1f3
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr22836-1b.d
@@ -0,0 +1,15 @@ 
+#source: pr22836-1.s
+#ld: -r -S
+#readelf: -S --wide
+#notarget: alpha-*-* cr16-*-* crx-*-* d30v-*-* dlx-*-* i960-*-* pj*-*-*
+# Disabled on alpha because alpha has a different .set directive.
+# cr16 and crx use non-standard scripts with memory regions, which don't
+# play well with comdat group sections under ld -r.  Generic linker
+# targets don't support comdat group sections.
+
+#failif
+#...
+  \[[ 0-9]+\] \.debug_.* +.*
+#...
+COMDAT group section .*
+#...