[patch] stop objcopy breaking group sections

[patch] stop objcopy breaking group sections

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

Hi,

objcopy (and strip -g) has a bug that breaks group sections. I've tested 
it using arm-none-eabi, but the problem almost certainly affects all 
targets.

To reproduce:

---test.cxx-------
class X
{
   public:
     int nokey() {return 0;}
};

int b(X*);
void t1()
{
   class X x;
   b(&x);
   x.nokey();
}
-----------------

$ arm-none-eabi-g++ -O0 -c test.cxx
$ arm-none-eabi-readelf -S -g -s test.o
...
   [ 1] .group            GROUP           00000000 000034 000010 04 
17  15  4
...
     15: 00000000    36 FUNC    WEAK   DEFAULT    6 _ZN1X5nokeyEv
...
COMDAT group section [    1] `.group' [_ZN1X5nokeyEv] contains 3 sections:

$ arm-none-eabi-objcopy test.o new.o
$ arm-none-eabi-readelf -S -g -s new.o
...
   [ 1] _ZN1X5nokeyEv     GROUP           00000000 000034 000010 04 
17  14  4
...
     14: 00000000     0 SECTION LOCAL  DEFAULT    1
...
COMDAT group section [    1] `_ZN1X5nokeyEv' [_ZN1X5nokeyEv] contains 3 
sections:

In each case I have shown the symbol indicated by the sh_info field. As 
you can see, it changes from 15 to 14, but the relevant symbol isn't in 
location 14 - it's still at 15. The section name has also been changed, 
but this is harmless, if undisirable. Unfortunately, the behaviour of 
readelf when sh_info doesn't make sense seems to somehow find the right 
value by chance, but the linker doesn't do the same. The new binary will 
no longer link correctly.

On closer investigation it turns out that the number 14 is "random" - 
that is, the algorithm gets it from totally the wrong place, although it 
has a predictable value.

In fact, it seems that code has been written to write sh_info, in the 
case of the assembler, read it, in the case of the linker, but the code 
to copy it is totally missing.

The attached patch should rectify this situation. I did also attempt to 
prevent it modifying the section name, but the linker is incapable of 
linking binaries where multiple sections have the same name (it only 
links in one of them), so I've given up on that.

OK?

Andrew Stubbs

[-- Attachment #2: group-strip.patch --]
[-- Type: text/x-diff, Size: 1272 bytes --]

2008-09-24  Andrew Stubbs  <ams@codesourcery.com>

	* elf.c (elf_fake_sections): Find the signature symbol for
	SHT_GROUP sections when doing objcopy/strip.

Index: bfd/elf.c
===================================================================
--- bfd/elf.c.orig	2008-08-08 09:00:14.000000000 +0100
+++ bfd/elf.c	2008-09-24 15:43:20.000000000 +0100
@@ -2622,6 +2622,25 @@ elf_fake_sections (bfd *abfd, asection *
 
     case SHT_GROUP:
       this_hdr->sh_entsize = GRP_ENTRY_SIZE;
+
+      /* Find the group signature symbol from the name.  This only applies if
+	 we don't already have a valid symbol and there are symbols available:
+	   gas - symbol provided already.
+	   ld  - name given, no symbols available, no symbol needed.
+	   objcopy/strip - name given, symbol required.
+	 This relies on bfd_section_from_shdr naming the section using the
+	 group signature.  */
+      if (elf_group_id (asect) == NULL && bfd_get_outsymbols (abfd) != NULL)
+	{
+	  unsigned int i;
+	  for (i = 0; i < bfd_get_symcount (abfd); i++)
+	    if (strcmp (bfd_section_name (abfd, asect),
+			bfd_asymbol_name (bfd_get_outsymbols (abfd) [i])) == 0)
+	      {
+		elf_group_id (asect) = bfd_get_outsymbols (abfd) [i];
+		break;
+	      }
+	  }
       break;
 
     case SHT_GNU_HASH:

Re: [patch] stop objcopy breaking group sections

[Alan Modra]

On Wed, Sep 24, 2008 at 04:12:04PM +0100, Andrew Stubbs wrote:
> $ arm-none-eabi-readelf -S -g -s new.o
> ...
>   [ 1] _ZN1X5nokeyEv     GROUP           00000000 000034 000010 04 
> 17  14  4
> ...
>     14: 00000000     0 SECTION LOCAL  DEFAULT    1
> ...
> COMDAT group section [    1] `_ZN1X5nokeyEv' [_ZN1X5nokeyEv] contains 3 
> sections:

So the group id symbol is now the section symbol for the group.
What's wrong with that?

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] stop objcopy breaking group sections

[Andrew Stubbs]

Alan Modra wrote:
> So the group id symbol is now the section symbol for the group.
> What's wrong with that?

Two things:

1. I asked it to (obj)copy the file, not modify section names.

2. The sh_info field is pointing at the wrong symbol. The group is broken.

I'm not sure which point you were making, but hopefully that's answered it.

Andrew

Re: [patch] stop objcopy breaking group sections

[Alan Modra]

On Thu, Sep 25, 2008 at 10:20:48AM +0100, Andrew Stubbs wrote:
> Alan Modra wrote:
> >So the group id symbol is now the section symbol for the group.
> >What's wrong with that?
> 
> Two things:
> 
> 1. I asked it to (obj)copy the file, not modify section names.

As you noted, this is minor.  objcopy can and does make many changes
on a simple copy, especially on objects produced by non-GNU tools.

> 2. The sh_info field is pointing at the wrong symbol. The group is broken.
> 
> I'm not sure which point you were making, but hopefully that's answered it.

Nope.

The ELF spec says sh_info for SHT_GROUP is:

The symbol table index of an entry in the associated symbol table. The
name of the specified symbol table entry provides a signature for the
section group.

Since the section name is changed to the group name, it should be
possible to use the section symbol as the id symbol.  Doesn't look
like a "wrong symbol" or "broken" to me.  If ld is having trouble
linking such a group, then we need to fix ld.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] stop objcopy breaking group sections

[Andrew Stubbs]

Alan Modra wrote:
> Nope.
> 
> The ELF spec says sh_info for SHT_GROUP is:
> 
> The symbol table index of an entry in the associated symbol table. The
> name of the specified symbol table entry provides a signature for the
> section group.
> 
> Since the section name is changed to the group name, it should be
> possible to use the section symbol as the id symbol.  Doesn't look
> like a "wrong symbol" or "broken" to me.  If ld is having trouble
> linking such a group, then we need to fix ld.

OK, I'll admit that I do not fully understand how this mechanism works. 
The code is less than easy to understand, and it doesn't help that it 
doesn't even attempt to do copies precisely.

On a second examination it does seem that the value of sh_info is 
meaningful, although confusingly it doesn't use the same value that the 
original file had (and I still can't see the mechanism by which it 
calculates this value).

But I can't decide if this is "wrong" or not. Your quote said "The name 
of the specified symbol table entry provides a signature for the section 
group." In this case readelf does not display a name for the symbol. It 
is this anonymity that is causing the problem, but I don't know why yet.

Hmmm, I'm going to have to go away and think about this one some more.

Thanks

Andrew

Re: [patch] stop objcopy breaking group sections

[Daniel Jacobowitz]

On Thu, Sep 25, 2008 at 08:09:14PM +0930, Alan Modra wrote:
> The ELF spec says sh_info for SHT_GROUP is:
> 
> The symbol table index of an entry in the associated symbol table. The
> name of the specified symbol table entry provides a signature for the
> section group.
> 
> Since the section name is changed to the group name, it should be
> possible to use the section symbol as the id symbol.  Doesn't look
> like a "wrong symbol" or "broken" to me.  If ld is having trouble
> linking such a group, then we need to fix ld.

I vaguely recall that there's a hack involved in the names of section
symbols; BFD wants them to have the section name but we changed it not
to output the name unless it was necessary.  Could that be the
problem?  The spec does talk about the name of the symbol, not the
name of the corresponding section.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] stop objcopy breaking group sections

[Alan Modra]

On Thu, Sep 25, 2008 at 08:21:19AM -0400, Daniel Jacobowitz wrote:
> I vaguely recall that there's a hack involved in the names of section
> symbols; BFD wants them to have the section name but we changed it not
> to output the name unless it was necessary.  Could that be the
> problem?  The spec does talk about the name of the symbol, not the
> name of the corresponding section.

Yes, STT_SECTION symbols have st_name of zero (*).  Their name is that
of their st_shndx section.  See elf.c:bfd_elf_sym_name.

*) At least, that's how GNU ld creates section symbols.  In a quick
look over the ELF gabi, I couldn't see anything that officially
blesses this practice.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] stop objcopy breaking group sections

[Daniel Jacobowitz]

On Fri, Sep 26, 2008 at 08:35:40AM +0930, Alan Modra wrote:
> On Thu, Sep 25, 2008 at 08:21:19AM -0400, Daniel Jacobowitz wrote:
> > I vaguely recall that there's a hack involved in the names of section
> > symbols; BFD wants them to have the section name but we changed it not
> > to output the name unless it was necessary.  Could that be the
> > problem?  The spec does talk about the name of the symbol, not the
> > name of the corresponding section.
> 
> Yes, STT_SECTION symbols have st_name of zero (*).  Their name is that
> of their st_shndx section.  See elf.c:bfd_elf_sym_name.
> 
> *) At least, that's how GNU ld creates section symbols.  In a quick
> look over the ELF gabi, I couldn't see anything that officially
> blesses this practice.

So, it seems to me, we're wrong to use such a section sym as a group
signature.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] stop objcopy breaking group sections

[Ian Lance Taylor]

Daniel Jacobowitz <drow@false.org> writes:

> On Fri, Sep 26, 2008 at 08:35:40AM +0930, Alan Modra wrote:
>> On Thu, Sep 25, 2008 at 08:21:19AM -0400, Daniel Jacobowitz wrote:
>> > I vaguely recall that there's a hack involved in the names of section
>> > symbols; BFD wants them to have the section name but we changed it not
>> > to output the name unless it was necessary.  Could that be the
>> > problem?  The spec does talk about the name of the symbol, not the
>> > name of the corresponding section.
>> 
>> Yes, STT_SECTION symbols have st_name of zero (*).  Their name is that
>> of their st_shndx section.  See elf.c:bfd_elf_sym_name.
>> 
>> *) At least, that's how GNU ld creates section symbols.  In a quick
>> look over the ELF gabi, I couldn't see anything that officially
>> blesses this practice.
>
> So, it seems to me, we're wrong to use such a section sym as a group
> signature.

In gold I had write an explicit workaround to deal with this.

  // It seems that some versions of gas will create a section group
  // associated with a section symbol, and then fail to give a name to
  // the section symbol.  In such a case, use the name of the section.

Whether or not it is acceptable ELF, it's inefficient, because it
means that the group signature string gets stored in the section name
string table.  Since there is usually a symbol with the name of the
group signature, the string is already stored in the symbol name
string table.  The purpose of the little dance to find the signature
string is to permit storing it only once, in the symbol name string
table.  This matters because those signature strings can be very long
for C++ programs--e.g., 1K bytes--and duplicating them is wasteful.

Ian

Re: [patch] stop objcopy breaking group sections

[Alan Modra]

On Fri, Sep 26, 2008 at 07:52:40AM -0700, Ian Lance Taylor wrote:
> Whether or not it is acceptable ELF, it's inefficient, because it
> means that the group signature string gets stored in the section name
> string table.  Since there is usually a symbol with the name of the
> group signature, the string is already stored in the symbol name
> string table.  The purpose of the little dance to find the signature
> string is to permit storing it only once, in the symbol name string
> table.  This matters because those signature strings can be very long
> for C++ programs--e.g., 1K bytes--and duplicating them is wasteful.

gas knows this, and uses a symbol if one exists.  I agree that
objcopy should do the same, for efficiency reasons if no other.
Preferably not by scanning the symbols, but by setting up elf_group_id
in bfd_section_from_shdr when reading the input file.

However, I still think it isn't broken to use the section symbol (and
if it is, then gas needs to change to never use a section symbol
too).

-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] stop objcopy breaking group sections

[Alan Modra]

On Sat, Sep 27, 2008 at 11:14:19AM +0930, Alan Modra wrote:
> Preferably not by scanning the symbols, but by setting up elf_group_id
> in bfd_section_from_shdr when reading the input file.

Actually, not there because the bfd symbols aren't available at that
point.  Instead, objcopy needs to set elf_group_id after it has read
the bfd symbols.

The bfd part of this patch also keeps the original SHT_GROUP section
name.

bfd/
	* elf.c (_bfd_elf_init_private_section_data): Tweak union copy.
	(bfd_section_from_shdr): Don't change SHT_GROUP section name.
	* elflink.c (section_signature): New function.
	(_bfd_elf_section_already_linked): Use it.
binutils/
	* objcopy.c (setup_section): Set elf_group_id.

Index: bfd/elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.462
diff -u -p -r1.462 elf.c
--- bfd/elf.c	8 Aug 2008 08:00:14 -0000	1.462
+++ bfd/elf.c	28 Sep 2008 12:25:27 -0000
@@ -1863,14 +1863,8 @@ bfd_section_from_shdr (bfd *abfd, unsign
       return TRUE;
 
     case SHT_GROUP:
-      /* We need a BFD section for objcopy and relocatable linking,
-	 and it's handy to have the signature available as the section
-	 name.  */
       if (! IS_VALID_GROUP_SECTION_HEADER (hdr))
 	return FALSE;
-      name = group_signature (abfd, hdr);
-      if (name == NULL)
-	return FALSE;
       if (!_bfd_elf_make_section_from_shdr (abfd, hdr, name, shindex))
 	return FALSE;
       if (hdr->contents != NULL)
@@ -6019,7 +6013,7 @@ _bfd_elf_init_private_section_data (bfd 
 	  if (elf_section_flags (isec) & SHF_GROUP)
 	    elf_section_flags (osec) |= SHF_GROUP;
 	  elf_next_in_group (osec) = elf_next_in_group (isec);
-	  elf_group_name (osec) = elf_group_name (isec);
+	  elf_section_data (osec)->group = elf_section_data (isec)->group;
 	}
     }
 
Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.310
diff -u -p -r1.310 elflink.c
--- bfd/elflink.c	16 Sep 2008 14:09:34 -0000	1.310
+++ bfd/elflink.c	28 Sep 2008 12:25:33 -0000
@@ -11997,8 +11997,21 @@ bfd_elf_discard_info (bfd *output_bfd, s
   return ret;
 }
 
+/* For a SHT_GROUP section, return the group signature.  For other
+   sections, return the normal section name.  */
+
+static const char *
+section_signature (asection *sec)
+{
+  if ((sec->flags & SEC_GROUP) != 0
+      && elf_next_in_group (sec) != NULL
+      && elf_group_name (elf_next_in_group (sec)) != NULL)
+    return elf_group_name (elf_next_in_group (sec));
+  return sec->name;
+}
+
 void
-_bfd_elf_section_already_linked (bfd *abfd, struct bfd_section *sec,
+_bfd_elf_section_already_linked (bfd *abfd, asection *sec,
 				 struct bfd_link_info *info)
 {
   flagword flags;
@@ -12038,7 +12051,7 @@ _bfd_elf_section_already_linked (bfd *ab
      causes trouble for MIPS ELF, which relies on link once semantics
      to handle the .reginfo section correctly.  */
 
-  name = bfd_get_section_name (abfd, sec);
+  name = section_signature (sec);
 
   if (CONST_STRNEQ (name, ".gnu.linkonce.")
       && (p = strchr (name + sizeof (".gnu.linkonce.") - 1, '.')) != NULL)
@@ -12053,7 +12066,7 @@ _bfd_elf_section_already_linked (bfd *ab
       /* We may have 2 different types of sections on the list: group
 	 sections and linkonce sections.  Match like sections.  */
       if ((flags & SEC_GROUP) == (l->sec->flags & SEC_GROUP)
-	  && strcmp (name, l->sec->name) == 0
+	  && strcmp (name, section_signature (l->sec)) == 0
 	  && bfd_coff_get_comdat_section (l->sec->owner, l->sec) == NULL)
 	{
 	  /* The section has already been linked.  See if we should
Index: binutils/objcopy.c
===================================================================
RCS file: /cvs/src/src/binutils/objcopy.c,v
retrieving revision 1.123
diff -u -p -r1.123 objcopy.c
--- binutils/objcopy.c	6 Aug 2008 00:42:17 -0000	1.123
+++ binutils/objcopy.c	28 Sep 2008 12:35:20 -0000
@@ -2344,6 +2344,18 @@ setup_section (bfd *ibfd, sec_ptr isecti
   if (extract_symbol)
     return;
 
+  if ((isection->flags & SEC_GROUP) != 0)
+    {
+      asymbol *gsym = group_signature (isection);
+
+      if (gsym != NULL)
+	{
+	  gsym->flags |= BSF_KEEP;
+	  if (ibfd->xvec->flavour == bfd_target_elf_flavour)
+	    elf_group_id (isection) = gsym;
+	}
+    }
+
   /* Allow the BFD backend to copy any private data it understands
      from the input section to the output section.  */
   if (!bfd_copy_private_section_data (ibfd, isection, obfd, osection))
@@ -2351,13 +2363,6 @@ setup_section (bfd *ibfd, sec_ptr isecti
       err = _("failed to copy private data");
       goto loser;
     }
-  else if ((isection->flags & SEC_GROUP) != 0)
-    {
-      asymbol *gsym = group_signature (isection);
-
-      if (gsym != NULL)
-	gsym->flags |= BSF_KEEP;
-    }
 
   /* All went well.  */
   return;


-- 
Alan Modra
Australia Development Lab, IBM

Re: [patch] stop objcopy breaking group sections

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

On Sun, Sep 28, 2008 at 6:28 AM, Alan Modra <amodra@bigpond.net.au> wrote:
> On Sat, Sep 27, 2008 at 11:14:19AM +0930, Alan Modra wrote:
>> Preferably not by scanning the symbols, but by setting up elf_group_id
>> in bfd_section_from_shdr when reading the input file.
>
> Actually, not there because the bfd symbols aren't available at that
> point.  Instead, objcopy needs to set elf_group_id after it has read
> the bfd symbols.
>
> The bfd part of this patch also keeps the original SHT_GROUP section
> name.
>
> bfd/
>        * elf.c (_bfd_elf_init_private_section_data): Tweak union copy.
>        (bfd_section_from_shdr): Don't change SHT_GROUP section name.
>        * elflink.c (section_signature): New function.
>        (_bfd_elf_section_already_linked): Use it.
> binutils/
>        * objcopy.c (setup_section): Set elf_group_id.
>

I am checking in the following testcases for this change.


-- 
H.J.
---
2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* binutils-all/group-2.s: New.
	* binutils-all/strip-4.d: Likewise.
	* binutils-all/strip-5.d: Likewise.

	* binutils-all/objcopy.exp: Test objcopy on group-2.s.  Run
	strip-4 and strip-5.

[-- Attachment #2: binutils-group-1.patch --]
[-- Type: application/octet-stream, Size: 2370 bytes --]

2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* binutils-all/group-2.s: New.
	* binutils-all/strip-4.d: Likewise.
	* binutils-all/strip-5.d: Likewise.

	* binutils-all/objcopy.exp: Test objcopy on group-2.s.  Run
	strip-4 and strip-5.

--- testsuite/binutils-all/group-2.s.grp	2008-10-01 14:44:40.000000000 -0700
+++ testsuite/binutils-all/group-2.s	2008-10-01 14:36:42.000000000 -0700
@@ -0,0 +1,8 @@
+	.section .text.foo,"axG",%progbits,.text.foo,comdat
+	.global foo
+foo:
+	.word 0
+	.section .data.bar,"awG",%progbits,.text.foo,comdat
+	.global bar
+bar:
+	.word 0
--- testsuite/binutils-all/objcopy.exp.grp	2008-07-09 06:11:55.000000000 -0700
+++ testsuite/binutils-all/objcopy.exp	2008-10-01 14:43:20.000000000 -0700
@@ -827,6 +827,7 @@ if { ([istarget "ia64-*-elf*"]
 if [is_elf_format] {
     objcopy_test "ELF unknown section type" unknown.s
     objcopy_test_readelf "ELF group" group.s
+    objcopy_test_readelf "ELF group" group-2.s
     run_dump_test "copy-1"
 }
 
@@ -837,6 +838,8 @@ if [is_elf_format] {
     run_dump_test "strip-1"
     run_dump_test "strip-2"
     run_dump_test "strip-3"
+    run_dump_test "strip-4"
+    run_dump_test "strip-5"
 
     if { [istarget "i*86-*"] || [istarget "x86_64-*-*"] } {
 	# Check to make sure we don't strip a symbol named in relocations.
--- testsuite/binutils-all/strip-4.d.grp	2008-10-01 14:44:31.000000000 -0700
+++ testsuite/binutils-all/strip-4.d	2008-10-01 14:36:15.000000000 -0700
@@ -0,0 +1,11 @@
+#PROG: strip
+#source: group-2.s
+#readelf: -Sg --wide
+#name: strip with section group 4
+
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AX[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#...
+There are no section groups in this file.
--- testsuite/binutils-all/strip-5.d.grp	2008-10-01 14:44:34.000000000 -0700
+++ testsuite/binutils-all/strip-5.d	2008-10-01 14:36:20.000000000 -0700
@@ -0,0 +1,18 @@
+#PROG: strip
+#source: group-2.s
+#strip: --strip-unneeded
+#readelf: -Sg --wide
+#name: strip with section group 5
+
+#...
+  \[[ 0-9]+\] .group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AXG[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WAG[ \t]+.*
+#...
+COMDAT group section \[[ 0-9]+\] `.group' \[.text.foo\] contains 2 sections:
+   \[Index\]    Name
+   \[[ 0-9]+\]   .text.*
+   \[[ 0-9]+\]   .data.*
+#pass

Re: [patch] stop objcopy breaking group sections

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Wed, Oct 1, 2008 at 2:52 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Sep 28, 2008 at 6:28 AM, Alan Modra <amodra@bigpond.net.au> wrote:
>> On Sat, Sep 27, 2008 at 11:14:19AM +0930, Alan Modra wrote:
>>> Preferably not by scanning the symbols, but by setting up elf_group_id
>>> in bfd_section_from_shdr when reading the input file.
>>
>> Actually, not there because the bfd symbols aren't available at that
>> point.  Instead, objcopy needs to set elf_group_id after it has read
>> the bfd symbols.
>>
>> The bfd part of this patch also keeps the original SHT_GROUP section
>> name.
>>
>> bfd/
>>        * elf.c (_bfd_elf_init_private_section_data): Tweak union copy.
>>        (bfd_section_from_shdr): Don't change SHT_GROUP section name.
>>        * elflink.c (section_signature): New function.
>>        (_bfd_elf_section_already_linked): Use it.
>> binutils/
>>        * objcopy.c (setup_section): Set elf_group_id.
>>
>
> I am checking in the following testcases for this change.
>
>

I checked in more tests.

-- 
H.J.
--

2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* binutils-all/group-3.s: New.
	* binutils-all/strip-6.d: Likewise.
	* binutils-all/strip-7.d: Likewise.

	* binutils-all/objcopy.exp: Test objcopy on group-3.s.  Run
	strip-6 and strip-7.

[-- Attachment #2: binutils-group-2.patch --]
[-- Type: application/octet-stream, Size: 2280 bytes --]

2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* binutils-all/group-3.s: New.
	* binutils-all/strip-6.d: Likewise.
	* binutils-all/strip-7.d: Likewise.

	* binutils-all/objcopy.exp: Test objcopy on group-3.s.  Run
	strip-6 and strip-7.

--- binutils-all/group-3.s.grp	2008-10-01 16:08:17.000000000 -0700
+++ binutils-all/group-3.s	2008-10-01 16:05:22.000000000 -0700
@@ -0,0 +1,8 @@
+	.section .text,"axG",%progbits,foo,comdat
+	.global foo
+foo:
+	.word 0
+	.section .data,"awG",%progbits,foo,comdat
+	.global bar
+bar:
+	.word 0
--- binutils-all/objcopy.exp.grp	2008-10-01 14:55:13.000000000 -0700
+++ binutils-all/objcopy.exp	2008-10-01 16:06:21.000000000 -0700
@@ -828,6 +828,7 @@ if [is_elf_format] {
     objcopy_test "ELF unknown section type" unknown.s
     objcopy_test_readelf "ELF group" group.s
     objcopy_test_readelf "ELF group" group-2.s
+    objcopy_test_readelf "ELF group" group-3.s
     run_dump_test "copy-1"
 }
 
@@ -840,6 +841,8 @@ if [is_elf_format] {
     run_dump_test "strip-3"
     run_dump_test "strip-4"
     run_dump_test "strip-5"
+    run_dump_test "strip-6"
+    run_dump_test "strip-7"
 
     if { [istarget "i*86-*"] || [istarget "x86_64-*-*"] } {
 	# Check to make sure we don't strip a symbol named in relocations.
--- binutils-all/strip-6.d.grp	2008-10-01 16:08:17.000000000 -0700
+++ binutils-all/strip-6.d	2008-10-01 16:06:48.000000000 -0700
@@ -0,0 +1,11 @@
+#PROG: strip
+#source: group-3.s
+#readelf: -Sg --wide
+#name: strip with section group 6
+
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AX[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#...
+There are no section groups in this file.
--- binutils-all/strip-7.d.grp	2008-10-01 16:08:17.000000000 -0700
+++ binutils-all/strip-7.d	2008-10-01 16:07:41.000000000 -0700
@@ -0,0 +1,18 @@
+#PROG: strip
+#source: group-3.s
+#strip: --strip-unneeded
+#readelf: -Sg --wide
+#name: strip with section group 7
+
+#...
+  \[[ 0-9]+\] .group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AXG[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WAG[ \t]+.*
+#...
+COMDAT group section \[[ 0-9]+\] `.group' \[foo\] contains 2 sections:
+   \[Index\]    Name
+   \[[ 0-9]+\]   .text.*
+   \[[ 0-9]+\]   .data.*
+#pass

Re: [patch] stop objcopy breaking group sections

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

On Wed, Oct 1, 2008 at 4:11 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Oct 1, 2008 at 2:52 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sun, Sep 28, 2008 at 6:28 AM, Alan Modra <amodra@bigpond.net.au> wrote:
>>> On Sat, Sep 27, 2008 at 11:14:19AM +0930, Alan Modra wrote:
>>>> Preferably not by scanning the symbols, but by setting up elf_group_id
>>>> in bfd_section_from_shdr when reading the input file.
>>>
>>> Actually, not there because the bfd symbols aren't available at that
>>> point.  Instead, objcopy needs to set elf_group_id after it has read
>>> the bfd symbols.
>>>
>>> The bfd part of this patch also keeps the original SHT_GROUP section
>>> name.
>>>
>>> bfd/
>>>        * elf.c (_bfd_elf_init_private_section_data): Tweak union copy.
>>>        (bfd_section_from_shdr): Don't change SHT_GROUP section name.
>>>        * elflink.c (section_signature): New function.
>>>        (_bfd_elf_section_already_linked): Use it.
>>> binutils/
>>>        * objcopy.c (setup_section): Set elf_group_id.
>>>
>>
>> I am checking in the following testcases for this change.
>>
>>
>
> I checked in more tests.
>
> --
> H.J.
> --
>
> 2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>
>
>        * binutils-all/group-3.s: New.
>        * binutils-all/strip-6.d: Likewise.
>        * binutils-all/strip-7.d: Likewise.
>
>        * binutils-all/objcopy.exp: Test objcopy on group-3.s.  Run
>        strip-6 and strip-7.
>

I checked in more tests.


-- 
H.J.
---
2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* binutils-all/group-4.s: New.
	* binutils-all/strip-8.d: Likewise.
	* binutils-all/strip-9.d: Likewise.

	* binutils-all/objcopy.exp: Test objcopy on group-4.s.  Run
	strip-8 and strip-9.

[-- Attachment #2: binutils-group-3.patch --]
[-- Type: application/octet-stream, Size: 2245 bytes --]

2008-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* binutils-all/group-4.s: New.
	* binutils-all/strip-8.d: Likewise.
	* binutils-all/strip-9.d: Likewise.

	* binutils-all/objcopy.exp: Test objcopy on group-4.s.  Run
	strip-8 and strip-9.

--- binutils-all/group-4.s.grp	2008-10-01 18:04:12.000000000 -0700
+++ binutils-all/group-4.s	2008-10-01 18:00:44.000000000 -0700
@@ -0,0 +1,6 @@
+	.section .text,"axG",%progbits,foo,comdat
+foo:
+	.word 0
+	.section .data,"awG",%progbits,foo,comdat
+bar:
+	.word 0
--- binutils-all/objcopy.exp.grp	2008-10-01 17:57:46.000000000 -0700
+++ binutils-all/objcopy.exp	2008-10-01 18:00:34.000000000 -0700
@@ -829,6 +829,7 @@ if [is_elf_format] {
     objcopy_test_readelf "ELF group" group.s
     objcopy_test_readelf "ELF group" group-2.s
     objcopy_test_readelf "ELF group" group-3.s
+    objcopy_test_readelf "ELF group" group-4.s
     run_dump_test "copy-1"
 }
 
@@ -843,6 +844,8 @@ if [is_elf_format] {
     run_dump_test "strip-5"
     run_dump_test "strip-6"
     run_dump_test "strip-7"
+    run_dump_test "strip-8"
+    run_dump_test "strip-9"
 
     if { [istarget "i*86-*"] || [istarget "x86_64-*-*"] } {
 	# Check to make sure we don't strip a symbol named in relocations.
--- binutils-all/strip-8.d.grp	2008-10-01 18:04:12.000000000 -0700
+++ binutils-all/strip-8.d	2008-10-01 18:00:55.000000000 -0700
@@ -0,0 +1,11 @@
+#PROG: strip
+#source: group-4.s
+#readelf: -Sg --wide
+#name: strip with section group 8
+
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AX[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WA[ \t]+.*
+#...
+There are no section groups in this file.
--- binutils-all/strip-9.d.grp	2008-10-01 18:04:12.000000000 -0700
+++ binutils-all/strip-9.d	2008-10-01 18:01:02.000000000 -0700
@@ -0,0 +1,18 @@
+#PROG: strip
+#source: group-4.s
+#strip: --strip-unneeded
+#readelf: -Sg --wide
+#name: strip with section group 9
+
+#...
+  \[[ 0-9]+\] .group[ \t]+GROUP[ \t]+.*
+#...
+  \[[ 0-9]+\] \.text.*[ \t]+PROGBITS[ \t0-9a-f]+AXG[ \t]+.*
+#...
+  \[[ 0-9]+\] \.data.*[ \t]+PROGBITS[ \t0-9a-f]+WAG[ \t]+.*
+#...
+COMDAT group section \[[ 0-9]+\] `.group' \[foo\] contains 2 sections:
+   \[Index\]    Name
+   \[[ 0-9]+\]   .text.*
+   \[[ 0-9]+\]   .data.*
+#pass