Re: [PATCH] Remove the stripped group section from linker output

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

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 .*
+#...
-- 
2.14.3