[PATCH] gas: Fix internal error in S_SET_SEGMENT

[PATCH] gas: Fix internal error in S_SET_SEGMENT

[Alex Coplan]

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

Hello,

This patch fixes an internal error in GAS when defining a section using
a symbol that has already been named but not defined. For a minimal
reproducer, try the following input:

a=b
.sect a

The problem is that obj_elf_change_section() happily reuses the symbol
"a" created by equals() without clearing the sy_value field: prior to
this patch, it just set bsym. This caused a problem when attempting to
resolve the section symbol, since resolve_symbol_value() ended up
resolving the symbol as if it were the original symbol created by
equals(), which ends up leaving the section symbol in the undefined
section instead of in section a, hence the call to abort() in
S_SET_SEGMENT().

Testing:
 * New test fails before and passes after the change.
 * Regression tested on a wide variety of targets.

OK for master?

Thanks,
Alex

---

gas/ChangeLog:

2020-07-22  Alex Coplan  <alex.coplan@arm.com>

	* config/obj-elf.c (obj_elf_change_section): When
	repurposing an existing symbol, ensure that we set sy_value as
	per other (fresh) section symbols.
	* testsuite/gas/aarch64/section-symbol-redef.d: New test.
	* testsuite/gas/aarch64/section-symbol-redef.s: Input for test.


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1475 bytes --]

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index de22b5a1da8..c11a1da2295 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -759,7 +759,14 @@ obj_elf_change_section (const char *name,
       /* Add a symbol for this section to the symbol table.  */
       secsym = symbol_find (name);
       if (secsym != NULL)
-	symbol_set_bfdsym (secsym, sec->symbol);
+	{
+	  /* We could be repurposing an undefined symbol here: make sure we
+	     reset sy_value to look like other section symbols in order to avoid
+	     trying to incorrectly resolve this section symbol later on.  */
+	  static const expressionS expr = { .X_op = O_constant };
+	  symbol_set_value_expression (secsym, &expr);
+	  symbol_set_bfdsym (secsym, sec->symbol);
+	}
       else
 	symbol_table_insert (section_symbol (sec));
     }
diff --git a/gas/testsuite/gas/aarch64/section-symbol-redef.d b/gas/testsuite/gas/aarch64/section-symbol-redef.d
new file mode 100644
index 00000000000..06a56b50553
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/section-symbol-redef.d
@@ -0,0 +1,8 @@
+#objdump: -D
+
+.*:     file format .*
+
+Disassembly of section myseg:
+
+0+ <.*>:
+.*:	d65f03c0 	ret
diff --git a/gas/testsuite/gas/aarch64/section-symbol-redef.s b/gas/testsuite/gas/aarch64/section-symbol-redef.s
new file mode 100644
index 00000000000..a80398d129c
--- /dev/null
+++ b/gas/testsuite/gas/aarch64/section-symbol-redef.s
@@ -0,0 +1,3 @@
+myseg=not_defined_here
+.section myseg
+ret

Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[H.J. Lu]

On Wed, Jul 22, 2020 at 7:24 AM Alex Coplan <alex.coplan@arm.com> wrote:
>
> Hello,
>
> This patch fixes an internal error in GAS when defining a section using
> a symbol that has already been named but not defined. For a minimal
> reproducer, try the following input:
>
> a=b
> .sect a
>
> The problem is that obj_elf_change_section() happily reuses the symbol
> "a" created by equals() without clearing the sy_value field: prior to
> this patch, it just set bsym. This caused a problem when attempting to
> resolve the section symbol, since resolve_symbol_value() ended up
> resolving the symbol as if it were the original symbol created by
> equals(), which ends up leaving the section symbol in the undefined
> section instead of in section a, hence the call to abort() in
> S_SET_SEGMENT().
>
> Testing:
>  * New test fails before and passes after the change.
>  * Regression tested on a wide variety of targets.
>
> OK for master?
>
> Thanks,
> Alex
>
> ---
>
> gas/ChangeLog:
>
> 2020-07-22  Alex Coplan  <alex.coplan@arm.com>
>
>         * config/obj-elf.c (obj_elf_change_section): When
>         repurposing an existing symbol, ensure that we set sy_value as
>         per other (fresh) section symbols.
>         * testsuite/gas/aarch64/section-symbol-redef.d: New test.
>         * testsuite/gas/aarch64/section-symbol-redef.s: Input for test.
>

Why are tests arm specific?  Can you put them to ELF tests?

-- 
H.J.

Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[Alex Coplan]

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

Hi H.J,

Sorry for the delay; just got back from some time off.

> -----Original Message-----
> From: H.J. Lu <hjl.tools@gmail.com>
> Sent: 22 July 2020 15:40
> To: Alex Coplan <Alex.Coplan@arm.com>
> Cc: Binutils <binutils@sourceware.org>
> Subject: Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT
> 
> > gas/ChangeLog:
> >
> > 2020-07-22  Alex Coplan  <alex.coplan@arm.com>
> >
> >         * config/obj-elf.c (obj_elf_change_section): When
> >         repurposing an existing symbol, ensure that we set sy_value as
> >         per other (fresh) section symbols.
> >         * testsuite/gas/aarch64/section-symbol-redef.d: New test.
> >         * testsuite/gas/aarch64/section-symbol-redef.s: Input for test.
> >
> 
> Why are tests arm specific?  Can you put them to ELF tests?

Thanks for pointing this out. I wasn't sure if there was a
suitably-generic place for these to go, but it seems that the ELF test
directory is the right place.

See the updated patch attached. The test has been tweaked to fix the
indentation for targets that assume anything in the first column is a
label, and to skip on targets that either don't support assignment
(bfin) or require attributes for sections (h8300).

Is the updated version OK for master?

Thanks,
Alex

---

gas/ChangeLog:

2020-08-03  Alex Coplan  <alex.coplan@arm.com>

	* config/obj-elf.c (obj_elf_change_section): When repurposing an
	existing symbol, ensure that we set sy_value as per other (fresh)
	section symbols.
	* testsuite/gas/elf/elf.exp: Add new test.
	* testsuite/gas/elf/section-symbol-redef.d: New test.
	* testsuite/gas/elf/section-symbol-redef.s: Input for test.


[-- Attachment #2: patch_v2.txt --]
[-- Type: text/plain, Size: 1834 bytes --]

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index de22b5a1da8..c11a1da2295 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -759,7 +759,14 @@ obj_elf_change_section (const char *name,
       /* Add a symbol for this section to the symbol table.  */
       secsym = symbol_find (name);
       if (secsym != NULL)
-	symbol_set_bfdsym (secsym, sec->symbol);
+	{
+	  /* We could be repurposing an undefined symbol here: make sure we
+	     reset sy_value to look like other section symbols in order to avoid
+	     trying to incorrectly resolve this section symbol later on.  */
+	  static const expressionS expr = { .X_op = O_constant };
+	  symbol_set_value_expression (secsym, &expr);
+	  symbol_set_bfdsym (secsym, sec->symbol);
+	}
       else
 	symbol_table_insert (section_symbol (sec));
     }
diff --git a/gas/testsuite/gas/elf/elf.exp b/gas/testsuite/gas/elf/elf.exp
index 86b304ae34f..8cd61b36754 100644
--- a/gas/testsuite/gas/elf/elf.exp
+++ b/gas/testsuite/gas/elf/elf.exp
@@ -303,6 +303,7 @@ if { [is_elf_format] } then {
     run_dump_test "strtab"
 
     run_dump_test "bignums"
+    run_dump_test "section-symbol-redef"
     
     load_lib gas-dg.exp
     dg-init
diff --git a/gas/testsuite/gas/elf/section-symbol-redef.d b/gas/testsuite/gas/elf/section-symbol-redef.d
new file mode 100644
index 00000000000..88807a196a7
--- /dev/null
+++ b/gas/testsuite/gas/elf/section-symbol-redef.d
@@ -0,0 +1,6 @@
+#readelf: -x myseg
+#skip: bfin-*-*
+#skip: h8300-*
+
+Hex dump of section .*:
+  0x0+ 2a\s+\*
diff --git a/gas/testsuite/gas/elf/section-symbol-redef.s b/gas/testsuite/gas/elf/section-symbol-redef.s
new file mode 100644
index 00000000000..87e65699b48
--- /dev/null
+++ b/gas/testsuite/gas/elf/section-symbol-redef.s
@@ -0,0 +1,3 @@
+  myseg=not_defined_here
+  .section myseg
+  .byte 42

RE: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[Alex Coplan]

Ping.

> -----Original Message-----
> From: Binutils <binutils-bounces@sourceware.org> On Behalf Of Alex Coplan
> Sent: 03 August 2020 15:23
> To: H.J. Lu <hjl.tools@gmail.com>
> Cc: Binutils <binutils@sourceware.org>
> Subject: Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT
> 
> Hi H.J,
> 
> Sorry for the delay; just got back from some time off.
> 
> > -----Original Message-----
> > From: H.J. Lu <hjl.tools@gmail.com>
> > Sent: 22 July 2020 15:40
> > To: Alex Coplan <Alex.Coplan@arm.com>
> > Cc: Binutils <binutils@sourceware.org>
> > Subject: Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT
> >
> > > gas/ChangeLog:
> > >
> > > 2020-07-22  Alex Coplan  <alex.coplan@arm.com>
> > >
> > >         * config/obj-elf.c (obj_elf_change_section): When
> > >         repurposing an existing symbol, ensure that we set sy_value
> as
> > >         per other (fresh) section symbols.
> > >         * testsuite/gas/aarch64/section-symbol-redef.d: New test.
> > >         * testsuite/gas/aarch64/section-symbol-redef.s: Input for
> test.
> > >
> >
> > Why are tests arm specific?  Can you put them to ELF tests?
> 
> Thanks for pointing this out. I wasn't sure if there was a
> suitably-generic place for these to go, but it seems that the ELF test
> directory is the right place.
> 
> See the updated patch attached. The test has been tweaked to fix the
> indentation for targets that assume anything in the first column is a
> label, and to skip on targets that either don't support assignment
> (bfin) or require attributes for sections (h8300).
> 
> Is the updated version OK for master?
> 
> Thanks,
> Alex
> 
> ---
> 
> gas/ChangeLog:
> 
> 2020-08-03  Alex Coplan  <alex.coplan@arm.com>
> 
> 	* config/obj-elf.c (obj_elf_change_section): When repurposing an
> 	existing symbol, ensure that we set sy_value as per other (fresh)
> 	section symbols.
> 	* testsuite/gas/elf/elf.exp: Add new test.
> 	* testsuite/gas/elf/section-symbol-redef.d: New test.
> 	* testsuite/gas/elf/section-symbol-redef.s: Input for test.

Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[Nick Clifton]

Hi Alex,

> Ping.

Oops - sorry.

>> 2020-08-03  Alex Coplan  <alex.coplan@arm.com>
>>
>> 	* config/obj-elf.c (obj_elf_change_section): When repurposing an
>> 	existing symbol, ensure that we set sy_value as per other (fresh)
>> 	section symbols.
>> 	* testsuite/gas/elf/elf.exp: Add new test.
>> 	* testsuite/gas/elf/section-symbol-redef.d: New test.
>> 	* testsuite/gas/elf/section-symbol-redef.s: Input for test.

Approved - please apply.

Cheers
  Nick

RE: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[Alex Coplan]

Hi Nick,

> -----Original Message-----
> From: Nick Clifton <nickc@redhat.com>
> Sent: 17 August 2020 12:26
> To: Alex Coplan <Alex.Coplan@arm.com>; H.J. Lu <hjl.tools@gmail.com>
> Cc: Binutils <binutils@sourceware.org>
> Subject: Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT
> 
> Hi Alex,
> 
> > Ping.
> 
> Oops - sorry.
> 
> >> 2020-08-03  Alex Coplan  <alex.coplan@arm.com>
> >>
> >> 	* config/obj-elf.c (obj_elf_change_section): When repurposing an
> >> 	existing symbol, ensure that we set sy_value as per other (fresh)
> >> 	section symbols.
> >> 	* testsuite/gas/elf/elf.exp: Add new test.
> >> 	* testsuite/gas/elf/section-symbol-redef.d: New test.
> >> 	* testsuite/gas/elf/section-symbol-redef.s: Input for test.
> 
> Approved - please apply.

Committed with one obvious change: as Alan pointed out here [0], it's
best to use #notarget rather than #skip in order to skip tests on
certain targets.

So the test is now:

--- /dev/null
+++ b/gas/testsuite/gas/elf/section-symbol-redef.d
@@ -0,0 +1,5 @@
+#readelf: -x myseg
+#notarget: bfin-*-* h8300-*
+
+Hex dump of section .*:
+  0x0+ 2a\s+\*

Thanks,
Alex

[0] : https://sourceware.org/pipermail/binutils/2020-August/112743.html

RE: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[Alex Coplan]

> -----Original Message-----
> From: Binutils <binutils-bounces@sourceware.org> On Behalf Of Alex Coplan
> Sent: 17 August 2020 14:29
> To: nickc@redhat.com; H.J. Lu <hjl.tools@gmail.com>
> Cc: Binutils <binutils@sourceware.org>
> Subject: RE: [PATCH] gas: Fix internal error in S_SET_SEGMENT
> 
> Hi Nick,
> 
> > -----Original Message-----
> > From: Nick Clifton <nickc@redhat.com>
> > Sent: 17 August 2020 12:26
> > To: Alex Coplan <Alex.Coplan@arm.com>; H.J. Lu <hjl.tools@gmail.com>
> > Cc: Binutils <binutils@sourceware.org>
> > Subject: Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT
> >
> > Hi Alex,
> >
> > > Ping.
> >
> > Oops - sorry.
> >
> > >> 2020-08-03  Alex Coplan  <alex.coplan@arm.com>
> > >>
> > >> 	* config/obj-elf.c (obj_elf_change_section): When repurposing an
> > >> 	existing symbol, ensure that we set sy_value as per other (fresh)
> > >> 	section symbols.
> > >> 	* testsuite/gas/elf/elf.exp: Add new test.
> > >> 	* testsuite/gas/elf/section-symbol-redef.d: New test.
> > >> 	* testsuite/gas/elf/section-symbol-redef.s: Input for test.
> >
> > Approved - please apply.
> 
> Committed with one obvious change [...]

This applies and tests cleanly on the 2.35 branch. OK to backport?

Thanks,
Alex

Re: [PATCH] gas: Fix internal error in S_SET_SEGMENT

[Alan Modra]

On Tue, Sep 01, 2020 at 03:47:44PM +0000, Alex Coplan wrote:
> > > >> 2020-08-03  Alex Coplan  <alex.coplan@arm.com>
> > > >>
> > > >> 	* config/obj-elf.c (obj_elf_change_section): When repurposing an
> > > >> 	existing symbol, ensure that we set sy_value as per other (fresh)
> > > >> 	section symbols.
> > > >> 	* testsuite/gas/elf/elf.exp: Add new test.
> > > >> 	* testsuite/gas/elf/section-symbol-redef.d: New test.
> > > >> 	* testsuite/gas/elf/section-symbol-redef.s: Input for test.
> > >
> > > Approved - please apply.
> > 
> > Committed with one obvious change [...]
> 
> This applies and tests cleanly on the 2.35 branch. OK to backport?

No, in my opinion this doesn't count as a regression or a serious
enough bug to risk the chance that the fix is imperfect.

-- 
Alan Modra
Australia Development Lab, IBM