[PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Maciej W. Rozycki]

Hi,

 Equated symbols (defined with .eqv) are not yet fully resolved by the 
time relaxation is made.  As a result, if used in a relaxed expression, 
they cause a failure as follows:

branch-self.s:30: Error: attempt to get value of unresolved symbol `fnord'

 The fix is to manually walk the chain of symbols; we handle additive 
expressions involving constant (positive or negative) addends here only 
like elsewhere.

2010-07-26  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* config/tc-mips.c: Include "struc-symbol.h".
	(md_convert_frag): Resolve equated symbols manually.

 OK to apply?

  Maciej

binutils-gas-mips-eqv-relax.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2010-07-24 02:25:11.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2010-07-24 02:25:11.000000000 +0100
@@ -28,6 +28,7 @@
 #include "config.h"
 #include "subsegs.h"
 #include "safe-ctype.h"
+#include "struc-symbol.h"
 
 #include "opcode/mips.h"
 #include "itbl-ops.h"
@@ -14558,6 +14559,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
       unsigned long insn;
       bfd_boolean use_extend;
       unsigned short extend;
+      symbolS* sym;
 
       type = RELAX_MIPS16_TYPE (fragp->fr_subtype);
       op = mips16_immed_operands;
@@ -14575,8 +14577,15 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
 	  ext = FALSE;
 	}
 
-      resolve_symbol_value (fragp->fr_symbol);
-      val = S_GET_VALUE (fragp->fr_symbol);
+      val = 0;
+      sym = fragp->fr_symbol;
+      resolve_symbol_value (sym);
+      while (symbol_equated_p (sym))
+	{
+	  val += sym->sy_value.X_add_number;
+	  sym = sym->sy_value.X_add_symbol;
+	}
+      val += S_GET_VALUE (sym);
       if (op->pcrel)
 	{
 	  addressT addr;

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> Hi,
>
>  Equated symbols (defined with .eqv) are not yet fully resolved by the 
> time relaxation is made.  As a result, if used in a relaxed expression, 
> they cause a failure as follows:
>
> branch-self.s:30: Error: attempt to get value of unresolved symbol `fnord'
>
>  The fix is to manually walk the chain of symbols; we handle additive 
> expressions involving constant (positive or negative) addends here only 
> like elsewhere.
>
> 2010-07-26  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	gas/
> 	* config/tc-mips.c: Include "struc-symbol.h".
> 	(md_convert_frag): Resolve equated symbols manually.

Can you explain in more detail why the equated symbol handling
in resolve_symbol_value doesn't do the right thing?

Richard

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Maciej W. Rozycki]

On Tue, 27 Jul 2010, Richard Sandiford wrote:

> >  Equated symbols (defined with .eqv) are not yet fully resolved by the 
> > time relaxation is made.  As a result, if used in a relaxed expression, 
> > they cause a failure as follows:
> >
> > branch-self.s:30: Error: attempt to get value of unresolved symbol `fnord'
> >
> >  The fix is to manually walk the chain of symbols; we handle additive 
> > expressions involving constant (positive or negative) addends here only 
> > like elsewhere.
> >
> > 2010-07-26  Maciej W. Rozycki  <macro@codesourcery.com>
> >
> > 	gas/
> > 	* config/tc-mips.c: Include "struc-symbol.h".
> > 	(md_convert_frag): Resolve equated symbols manually.
> 
> Can you explain in more detail why the equated symbol handling
> in resolve_symbol_value doesn't do the right thing?

 finalize_syms isn't set yet and hence resolve_symbol_value() won't make 
the final resolution of the symbol -- see write_object_file().  We get 
here from relax_seg() which is before finalize_syms is possibly set at the 
earliest.  Try to assemble this program:

	.text
	.set	mips16
foo:
	.eqv	bar, foo
	b	bar

to trigger it.

  Maciej

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> On Tue, 27 Jul 2010, Richard Sandiford wrote:
>
>> >  Equated symbols (defined with .eqv) are not yet fully resolved by the 
>> > time relaxation is made.  As a result, if used in a relaxed expression, 
>> > they cause a failure as follows:
>> >
>> > branch-self.s:30: Error: attempt to get value of unresolved symbol `fnord'
>> >
>> >  The fix is to manually walk the chain of symbols; we handle additive 
>> > expressions involving constant (positive or negative) addends here only 
>> > like elsewhere.
>> >
>> > 2010-07-26  Maciej W. Rozycki  <macro@codesourcery.com>
>> >
>> > 	gas/
>> > 	* config/tc-mips.c: Include "struc-symbol.h".
>> > 	(md_convert_frag): Resolve equated symbols manually.
>> 
>> Can you explain in more detail why the equated symbol handling
>> in resolve_symbol_value doesn't do the right thing?
>
>  finalize_syms isn't set yet and hence resolve_symbol_value() won't make 
> the final resolution of the symbol -- see write_object_file().  We get 
> here from relax_seg() which is before finalize_syms is possibly set at the 
> earliest.  Try to assemble this program:
>
> 	.text
> 	.set	mips16
> foo:
> 	.eqv	bar, foo
> 	b	bar
>
> to trigger it.

What I really meant (and I should have made it clearer) was: why can't
we simply use the value that resolve_symbol_value gives us?  It looks to
me like it should be correct, and is more robust than an out-of-line loop.

The problem with the patch is that it makes us go into an infinite loop for:

	.text
	.set	mips16
	.eqv	bar, foo
	.eqv	foo, bar
	b	bar

Richard


Index: gas/config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.419
diff -u -p -r1.419 tc-mips.c
--- gas/config/tc-mips.c	28 Jun 2010 14:06:57 -0000	1.419
+++ gas/config/tc-mips.c	28 Jul 2010 19:39:34 -0000
@@ -14571,8 +14571,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
 	  ext = FALSE;
 	}
 
-      resolve_symbol_value (fragp->fr_symbol);
-      val = S_GET_VALUE (fragp->fr_symbol);
+      val = resolve_symbol_value (fragp->fr_symbol);
       if (op->pcrel)
 	{
 	  addressT addr;

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> What I really meant (and I should have made it clearer) was: why can't
> we simply use the value that resolve_symbol_value gives us?  It looks to
> me like it should be correct, and is more robust than an out-of-line loop.
>
> The problem with the patch is that it makes us go into an infinite loop for:

Er, for avoidance of doubt, I mean that that's the problem with the
looping patch, not (I hope) the one I attached immediately below...

Richard

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Maciej W. Rozycki]

On Wed, 28 Jul 2010, Richard Sandiford wrote:

> >  finalize_syms isn't set yet and hence resolve_symbol_value() won't make 
> > the final resolution of the symbol -- see write_object_file().  We get 
> > here from relax_seg() which is before finalize_syms is possibly set at the 
> > earliest.  Try to assemble this program:
> >
> > 	.text
> > 	.set	mips16
> > foo:
> > 	.eqv	bar, foo
> > 	b	bar
> >
> > to trigger it.
> 
> What I really meant (and I should have made it clearer) was: why can't
> we simply use the value that resolve_symbol_value gives us?  It looks to
> me like it should be correct, and is more robust than an out-of-line loop.

 We can, I suppose.  I missed the fact resolve_symbol_value() actually 
returns the value calculated.  However there's one problem with that as 
below...

> The problem with the patch is that it makes us go into an infinite loop for:
> 
> 	.text
> 	.set	mips16
> 	.eqv	bar, foo
> 	.eqv	foo, bar
> 	b	bar

 You're right -- I missed this case, however with your patch:

> Index: gas/config/tc-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gas/config/tc-mips.c,v
> retrieving revision 1.419
> diff -u -p -r1.419 tc-mips.c
> --- gas/config/tc-mips.c	28 Jun 2010 14:06:57 -0000	1.419
> +++ gas/config/tc-mips.c	28 Jul 2010 19:39:34 -0000
> @@ -14571,8 +14571,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
>  	  ext = FALSE;
>  	}
>  
> -      resolve_symbol_value (fragp->fr_symbol);
> -      val = S_GET_VALUE (fragp->fr_symbol);
> +      val = resolve_symbol_value (fragp->fr_symbol);
>        if (op->pcrel)
>  	{
>  	  addressT addr;

this program:

	.text
	.set	mips16
baz:
	nop
	.eqv	bar, foo
	.eqv	foo, bar
	b	bar

assembles to this:

Disassembly of section .text:

00000000 <baz>:
   0:	6500      	nop
   2:	17fe      	b	0 <baz>

(no relocations) while it should yield this:

beqv.s: Assembler messages:
beqv.s:7: Error: symbol definition loop encountered at `foo'

as with no patch whatever so more investigation is required.  I can't look 
into it any further now, but I'll try to get back to it as soon as 
possible.  Thanks for your inquisitiveness.

  Maciej

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> Index: gas/config/tc-mips.c
>> ===================================================================
>> RCS file: /cvs/src/src/gas/config/tc-mips.c,v
>> retrieving revision 1.419
>> diff -u -p -r1.419 tc-mips.c
>> --- gas/config/tc-mips.c	28 Jun 2010 14:06:57 -0000	1.419
>> +++ gas/config/tc-mips.c	28 Jul 2010 19:39:34 -0000
>> @@ -14571,8 +14571,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
>>  	  ext = FALSE;
>>  	}
>>  
>> -      resolve_symbol_value (fragp->fr_symbol);
>> -      val = S_GET_VALUE (fragp->fr_symbol);
>> +      val = resolve_symbol_value (fragp->fr_symbol);
>>        if (op->pcrel)
>>  	{
>>  	  addressT addr;
>
> this program:
>
> 	.text
> 	.set	mips16
> baz:
> 	nop
> 	.eqv	bar, foo
> 	.eqv	foo, bar
> 	b	bar
>
> assembles to this:
>
> Disassembly of section .text:
>
> 00000000 <baz>:
>    0:	6500      	nop
>    2:	17fe      	b	0 <baz>
>
> (no relocations) while it should yield this:
>
> beqv.s: Assembler messages:
> beqv.s:7: Error: symbol definition loop encountered at `foo'

Hmm, how are you testing?  I get the expected error with a
mipsisa64-elfoabi toolchain.

Richard

Re: [PATCH 1/4] MIPS/GAS: Fix equated symbols in relaxation

[Maciej W. Rozycki]

On Thu, 29 Jul 2010, Richard Sandiford wrote:

> "Maciej W. Rozycki" <macro@codesourcery.com> writes:
> >> Index: gas/config/tc-mips.c
> >> ===================================================================
> >> RCS file: /cvs/src/src/gas/config/tc-mips.c,v
> >> retrieving revision 1.419
> >> diff -u -p -r1.419 tc-mips.c
> >> --- gas/config/tc-mips.c	28 Jun 2010 14:06:57 -0000	1.419
> >> +++ gas/config/tc-mips.c	28 Jul 2010 19:39:34 -0000
> >> @@ -14571,8 +14571,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
> >>  	  ext = FALSE;
> >>  	}
> >>  
> >> -      resolve_symbol_value (fragp->fr_symbol);
> >> -      val = S_GET_VALUE (fragp->fr_symbol);
> >> +      val = resolve_symbol_value (fragp->fr_symbol);
> >>        if (op->pcrel)
> >>  	{
> >>  	  addressT addr;
> >
> > this program:
> >
> > 	.text
> > 	.set	mips16
> > baz:
> > 	nop
> > 	.eqv	bar, foo
> > 	.eqv	foo, bar
> > 	b	bar
> >
> > assembles to this:
> >
> > Disassembly of section .text:
> >
> > 00000000 <baz>:
> >    0:	6500      	nop
> >    2:	17fe      	b	0 <baz>
> >
> > (no relocations) while it should yield this:
> >
> > beqv.s: Assembler messages:
> > beqv.s:7: Error: symbol definition loop encountered at `foo'
> 
> Hmm, how are you testing?  I get the expected error with a
> mipsisa64-elfoabi toolchain.

 Fair enough -- after retesting I discovered this patch is correct and the 
regression is caused by applying 2/4 on top of it -- which just means 
there's yet another reason to rework 2/4 beside these you mentioned 
already.

 Go ahead with the change or I can do it for you if you like.  And sorry 
it took so long -- I keep running out of free time slots.

  Maciej

[PATCH 1.0/4 v2] MIPS/GAS: Fix equated symbols in relaxation

[Maciej W. Rozycki]

Hi,

 Equated symbols (defined with .eqv) are not yet fully resolved by the 
time relaxation is made.  As a result, if used in a relaxed expression, 
they cause a failure as follows:

branch-self.s:30: Error: attempt to get value of unresolved symbol `fnord'

 The fix is to use the result of the resolver function called for the 
symbol at this point rather than trying to obtain the final value of the 
symbol itself.  As this final fix actually comes from you, you get the 
credit in the ChangeLog entry. :)

2010-10-29  Richard Sandiford  <rdsandiford@googlemail.com>

	gas/
	* config/tc-mips.c (md_convert_frag): Remove a call to
	S_GET_VALUE and use the result of resolve_symbol_value as the 
	value of the symbol processed in MIPS16 relaxation.

 OK to apply?

  Maciej

binutils-gas-mips-eqv-relax.diff
Index: binutils-fsf-trunk-quilt/gas/config/tc-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/config/tc-mips.c	2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/config/tc-mips.c	2010-10-29 09:07:10.000000000 +0100
@@ -14542,8 +14542,7 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
 	  ext = FALSE;
 	}
 
-      resolve_symbol_value (fragp->fr_symbol);
-      val = S_GET_VALUE (fragp->fr_symbol);
+      val = resolve_symbol_value (fragp->fr_symbol);
       if (op->pcrel)
 	{
 	  addressT addr;

Re: [PATCH 1.0/4 v2] MIPS/GAS: Fix equated symbols in relaxation

[Richard Sandiford]

Thanks for perservering with these patches.  I think 1 and 2 are
ready to go in.  However, you're dealing with some very subtle code here,
as the bugs you're fixing show, so please wait until 2.21 has branched
before committing.

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  Equated symbols (defined with .eqv) are not yet fully resolved by the 
> time relaxation is made.  As a result, if used in a relaxed expression, 
> they cause a failure as follows:
>
> branch-self.s:30: Error: attempt to get value of unresolved symbol `fnord'
>
>  The fix is to use the result of the resolver function called for the 
> symbol at this point rather than trying to obtain the final value of the 
> symbol itself.  As this final fix actually comes from you, you get the 
> credit in the ChangeLog entry. :)
>
> 2010-10-29  Richard Sandiford  <rdsandiford@googlemail.com>
>
> 	gas/
> 	* config/tc-mips.c (md_convert_frag): Remove a call to
> 	S_GET_VALUE and use the result of resolve_symbol_value as the 
> 	value of the symbol processed in MIPS16 relaxation.
>
>  OK to apply?

OK once 2.21 has branched.

Richard

Re: [PATCH 1.0/4 v2] MIPS/GAS: Fix equated symbols in relaxation

[Maciej W. Rozycki]

On Sat, 30 Oct 2010, Richard Sandiford wrote:

> > 	gas/
> > 	* config/tc-mips.c (md_convert_frag): Remove a call to
> > 	S_GET_VALUE and use the result of resolve_symbol_value as the 
> > 	value of the symbol processed in MIPS16 relaxation.
> >
> >  OK to apply?
> 
> OK once 2.21 has branched.

 Applied now, thanks.

  Maciej