[PATCH 2/4] GAS: Make new fake labels when cloning a symbol

[PATCH 2/4] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

Hi,

 Equated symbols (defined with .eqv) aka forward references are defined to 
reevaluate the expression they are defined to whenever they are referred 
to.  This does not happen for the special "dot" symbol -- the value 
calculated the first time the equated symbol has been used is then used 
over and over again.

 The reason is each time a reference to the "dot" symbol is made a special 
fake label is created.  This label is then recorded in the chain of 
symbols the equated symbol is defined to and never reevaluated.  The 
solution is to create a new fake label each time the equated symbol is 
used.

 Additionally symbol_clone_if_forward_ref() would only be called 
recursively for internal symbols referring to the special expression 
section.  It should happen for all symbols.

 Overall with symbol definitions like this:

	.data
	.eqv	fnord, .
	.eqv	foobar, fnord + 4
	.eqv	foobaz, foobar - 16
	.word	0
	.word	fnord
	.word	fnord
	.word	foobaz
	.word	foobaz

the "dot" symbol would only be calculated once, with the second .word 
directive, i.e. both the second and the third word emitted would have the 
same value (address of the second word).

 This would also cause "foobaz" to be calculated (as a forward reference) 
with the third .eqv directive and then cloned each time when used with 
.word.  Howeved "foobar" would only be calculated with the second and the 
third .eqv directive, but not with the .word directives, thus fixed at 
the value of the first rather than each use.

 In the end data emitted would be:

0, 0, 0, -12, -12

as if .set was used, rather than:

0, 4, 8, 0, 4

as expected.

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

	gas/
	* symbols.c (symbol_clone_if_forward_ref): Don't limit cloning
	to expr_section symbols.  Make fresh fake labels.
	* write.c (TC_FAKE_LABEL): Move over to...
	* write.h (TC_FAKE_LABEL): ... here.

 OK to apply?

  Maciej

binutils-gas-eqv-dot.diff
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c	2010-07-24 02:25:04.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c	2010-07-24 02:25:12.000000000 +0100
@@ -645,7 +645,7 @@ symbol_clone_if_forward_ref (symbolS *sy
 
       /* Re-using sy_resolving here, as this routine cannot get called from
 	 symbol resolution code.  */
-      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
+      if (!symbolP->sy_resolving)
 	{
 	  symbolP->sy_resolving = 1;
 	  add_symbol = symbol_clone_if_forward_ref (add_symbol, is_forward);
@@ -656,7 +656,23 @@ symbol_clone_if_forward_ref (symbolS *sy
       if (symbolP->sy_forward_ref
 	  || add_symbol != symbolP->sy_value.X_add_symbol
 	  || op_symbol != symbolP->sy_value.X_op_symbol)
-	symbolP = symbol_clone (symbolP, 0);
+	{
+	  symbolS *temp_symbol;
+	  int is_temp_add;
+	  int is_temp_op;
+
+	  symbolP = symbol_clone (symbolP, 0);
+	  is_temp_add = add_symbol && TC_FAKE_LABEL (S_GET_NAME (add_symbol));
+	  is_temp_op = op_symbol && TC_FAKE_LABEL (S_GET_NAME (op_symbol));
+	  if (is_temp_add || is_temp_op)
+	    {
+	      temp_symbol = symbol_temp_new_now ();
+	      if (is_temp_add)
+		add_symbol = temp_symbol;
+	      if (is_temp_op)
+		op_symbol = temp_symbol;
+	    }
+	}
 
       symbolP->sy_value.X_add_symbol = add_symbol;
       symbolP->sy_value.X_op_symbol = op_symbol;
Index: binutils-fsf-trunk-quilt/gas/write.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/write.c	2010-07-24 02:25:04.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/write.c	2010-07-24 02:25:12.000000000 +0100
@@ -102,10 +102,6 @@
 #define MD_PCREL_FROM_SECTION(FIX, SEC) md_pcrel_from (FIX)
 #endif
 
-#ifndef TC_FAKE_LABEL
-#define TC_FAKE_LABEL(NAME) (strcmp ((NAME), FAKE_LABEL_NAME) == 0)
-#endif
-
 /* Positive values of TC_FX_SIZE_SLACK allow a target to define
    fixups that far past the end of a frag.  Having such fixups
    is of course most most likely a bug in setting fx_size correctly.
Index: binutils-fsf-trunk-quilt/gas/write.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/write.h	2010-07-24 02:25:04.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/write.h	2010-07-24 02:25:12.000000000 +0100
@@ -29,6 +29,10 @@
 #define FAKE_LABEL_NAME "L0\001"
 #endif
 
+#ifndef TC_FAKE_LABEL
+#define TC_FAKE_LABEL(NAME) (strcmp ((NAME), FAKE_LABEL_NAME) == 0)
+#endif
+
 #include "bit_fix.h"
 
 /*

Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  Equated symbols (defined with .eqv) aka forward references are defined to 
> reevaluate the expression they are defined to whenever they are referred 
> to.  This does not happen for the special "dot" symbol -- the value 
> calculated the first time the equated symbol has been used is then used 
> over and over again.
>
>  The reason is each time a reference to the "dot" symbol is made a special 
> fake label is created.  This label is then recorded in the chain of 
> symbols the equated symbol is defined to and never reevaluated.  The 
> solution is to create a new fake label each time the equated symbol is 
> used.
>
>  Additionally symbol_clone_if_forward_ref() would only be called 
> recursively for internal symbols referring to the special expression 
> section.  It should happen for all symbols.
>
>  Overall with symbol definitions like this:
>
> 	.data
> 	.eqv	fnord, .
> 	.eqv	foobar, fnord + 4
> 	.eqv	foobaz, foobar - 16
> 	.word	0
> 	.word	fnord
> 	.word	fnord
> 	.word	foobaz
> 	.word	foobaz
>
> the "dot" symbol would only be calculated once, with the second .word 
> directive, i.e. both the second and the third word emitted would have the 
> same value (address of the second word).
>
>  This would also cause "foobaz" to be calculated (as a forward reference) 
> with the third .eqv directive and then cloned each time when used with 
> .word.  Howeved "foobar" would only be calculated with the second and the 
> third .eqv directive, but not with the .word directives, thus fixed at 
> the value of the first rather than each use.
>
>  In the end data emitted would be:
>
> 0, 0, 0, -12, -12
>
> as if .set was used, rather than:
>
> 0, 4, 8, 0, 4
>
> as expected.

Unfortunately, keying off FAKE_LABEL_NAME isn't enough to prove that
that the symbol is ".".  FAKE_LABEL_NAME is also used in the DWARF
machinery, and to hold the result of nested expressions.  It's a bit
of a convoluted example, but after the patch:

	.set	noreorder
	.eqv	foo,(x+bar)-frob
	.set	bar,0
	.set	frob,0
x:
	nop
	b	foo
	nop
	nop
	b	foo
	nop

gives:

00000000 <x>:
   0:   00000000        nop
   4:   1000ffff        b       4 <x+0x4>
   8:   00000000        nop
   c:   00000000        nop
  10:   1000ffff        b       10 <x+0x10>
  14:   00000000        nop

Also, as far as:

-      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
+      if (!symbolP->sy_resolving)

goes, I'm not sure that we really should be walking through other
_non-eqv_ symbol definitions.  E.g. after the patch:

	.set	noreorder
	.eqv	foo,bar
	.eqv	baz,.
	.set	bar,baz
x:
	nop
	b	foo
	nop
	.set	bar,baz
	nop
	b	foo
	nop

assembles as:

00000000 <baz>:
   0:   00000000        nop
   4:   1000ffff        b       4 <baz+0x4>
   8:   00000000        nop

0000000c <bar>:
   c:   00000000        nop
  10:   1000ffff        b       10 <bar+0x4>
  14:   00000000        nop

whereas the branches should be to 0x0 and 0xc respectively.

I'm not saying the current code's right, of course.  I agree that:

	.set	noreorder
	.eqv	foo,bar
	.eqv	bar,baz
	.set	baz,.
	nop
	b	foo
	nop
	.set	baz,.
	nop
	b	foo
	nop

ought to be the same as the previous example, and at the moment it isn't.
I just think the recursiveness should be limited to expressions and
forward references.  It might help if we split the problem into two:
fixing the chained-.eqv evaluation, and fixing "." references.

Richard

Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

On Sat, 31 Jul 2010, Richard Sandiford wrote:

> Unfortunately, keying off FAKE_LABEL_NAME isn't enough to prove that
> that the symbol is ".".  FAKE_LABEL_NAME is also used in the DWARF
> machinery, and to hold the result of nested expressions.

 Tricky. ;)

> It's a bit
> of a convoluted example, but after the patch:
> 
> 	.set	noreorder
> 	.eqv	foo,(x+bar)-frob
> 	.set	bar,0
> 	.set	frob,0
> x:
> 	nop
> 	b	foo
> 	nop
> 	nop
> 	b	foo
> 	nop
> 
> gives:
> 
> 00000000 <x>:
>    0:   00000000        nop
>    4:   1000ffff        b       4 <x+0x4>
>    8:   00000000        nop
>    c:   00000000        nop
>   10:   1000ffff        b       10 <x+0x10>
>   14:   00000000        nop

 Correct. :(

> Also, as far as:
> 
> -      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
> +      if (!symbolP->sy_resolving)
> 
> goes, I'm not sure that we really should be walking through other
> _non-eqv_ symbol definitions.

 Fair enough.

> E.g. after the patch:
> 
> 	.set	noreorder
> 	.eqv	foo,bar
> 	.eqv	baz,.
> 	.set	bar,baz
> x:
> 	nop
> 	b	foo
> 	nop
> 	.set	bar,baz
> 	nop
> 	b	foo
> 	nop
> 
> assembles as:
> 
> 00000000 <baz>:
>    0:   00000000        nop
>    4:   1000ffff        b       4 <baz+0x4>
>    8:   00000000        nop
> 
> 0000000c <bar>:
>    c:   00000000        nop
>   10:   1000ffff        b       10 <bar+0x4>
>   14:   00000000        nop
> 
> whereas the branches should be to 0x0 and 0xc respectively.

 Correct again. :(

> I'm not saying the current code's right, of course.  I agree that:
> 
> 	.set	noreorder
> 	.eqv	foo,bar
> 	.eqv	bar,baz
> 	.set	baz,.
> 	nop
> 	b	foo
> 	nop
> 	.set	baz,.
> 	nop
> 	b	foo
> 	nop
> 
> ought to be the same as the previous example, and at the moment it isn't.
> I just think the recursiveness should be limited to expressions and
> forward references.  It might help if we split the problem into two:
> fixing the chained-.eqv evaluation, and fixing "." references.

 I have split the patch into two.  While that doesn't seem particularly 
useful here (and caused me some hassle with development), it fulfils the 
principle of one logical change per patch.

 I made further analysis of what's going on here and took conclusions from 
this observation: symbol_clone_if_forward_ref() is called both when an 
equated symbol is defined and when it is referred to.  The conclusion is 
all the cloning must only be made whenever the symbol is referred to and 
not when it's defined.  This comes from the definition of the equation 
operation.
  
 Taking the conclusion and your concerns into account I have come up with 
the below.  It works for all the three cases plus this one:

	.data
        
	.word	0
	.eqv	foobar, fnord + 4
	.eqv	foobaz, foobar - 16
	.set	fnord, .
	.word	fnord
	.set	fnord, .
	.word	fnord
	.set	fnord, .
	.word	foobaz
	.set	fnord, .
	.word	foobaz

It has passed regression testing with mips-sde-elf too.  If we agree on 
this change, then I'll see how to integrate the tests into the testsuite 
-- it'll be worth having all of them, as generic ones if possible too.

 Comments?

2010-08-27  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* symbols.c (symbol_clone_if_forward_ref): Add is_deferred
	argument.  Clone forwarded symbols too, when referenced.
	* symbols.h (symbol_clone_if_forward_ref): Update prototype
	and macro definition accordingly.
	* expr.c (operand): Let symbol_clone_if_forward_ref know if
	a deferred or actual reference is being made.

2010-08-27  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* symbols.c (symbol_clone_if_forward_ref): Make fresh fake
	labels for dot special symbol references.
	* write.c (TC_FAKE_LABEL): Move over to...
	* write.h (TC_FAKE_LABEL): ... here.

  Maciej

binutils-gas-eqv.diff
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c	2010-08-26 23:31:55.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c	2010-08-27 02:03:42.000000000 +0100
@@ -622,7 +622,7 @@ symbol_clone (symbolS *orgsymP, int repl
 
 #undef symbol_clone_if_forward_ref
 symbolS *
-symbol_clone_if_forward_ref (symbolS *symbolP, int is_forward)
+symbol_clone_if_forward_ref (symbolS *symbolP, int is_deferred, int is_forward)
 {
   if (symbolP && !LOCAL_SYMBOL_CHECK (symbolP))
     {
@@ -645,18 +645,23 @@ symbol_clone_if_forward_ref (symbolS *sy
 
       /* Re-using sy_resolving here, as this routine cannot get called from
 	 symbol resolution code.  */
-      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
+      if ((symbolP->bsym->section == expr_section
+	   || (!is_deferred && symbolP->sy_forward_ref))
+	  && !symbolP->sy_resolving)
 	{
 	  symbolP->sy_resolving = 1;
-	  add_symbol = symbol_clone_if_forward_ref (add_symbol, is_forward);
-	  op_symbol = symbol_clone_if_forward_ref (op_symbol, is_forward);
+	  add_symbol = symbol_clone_if_forward_ref (add_symbol, 0, is_forward);
+	  op_symbol = symbol_clone_if_forward_ref (op_symbol, 0, is_forward);
 	  symbolP->sy_resolving = 0;
 	}
 
       if (symbolP->sy_forward_ref
 	  || add_symbol != symbolP->sy_value.X_add_symbol
 	  || op_symbol != symbolP->sy_value.X_op_symbol)
-	symbolP = symbol_clone (symbolP, 0);
+	{
+	  symbolP = symbol_clone (symbolP, 0);
+	  symbolP->sy_resolving = 0;
+	}
 
       symbolP->sy_value.X_add_symbol = add_symbol;
       symbolP->sy_value.X_op_symbol = op_symbol;
Index: binutils-fsf-trunk-quilt/gas/expr.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/expr.c	2010-08-26 23:31:55.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/expr.c	2010-08-27 00:05:40.000000000 +0100
@@ -1366,8 +1366,12 @@ operand (expressionS *expressionP, enum 
   if (expressionP->X_add_symbol)
     symbol_mark_used (expressionP->X_add_symbol);
 
-  expressionP->X_add_symbol = symbol_clone_if_forward_ref (expressionP->X_add_symbol);
-  expressionP->X_op_symbol = symbol_clone_if_forward_ref (expressionP->X_op_symbol);
+  expressionP->X_add_symbol
+    = symbol_clone_if_forward_ref (expressionP->X_add_symbol,
+				   mode == expr_defer);
+  expressionP->X_op_symbol
+    = symbol_clone_if_forward_ref (expressionP->X_op_symbol,
+				   mode == expr_defer);
 
   switch (expressionP->X_op)
     {
Index: binutils-fsf-trunk-quilt/gas/symbols.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.h	2010-08-26 23:31:55.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.h	2010-08-27 00:05:40.000000000 +0100
@@ -51,8 +51,8 @@ symbolS *symbol_create (const char *name
 			fragS * frag);
 symbolS *symbol_clone (symbolS *, int);
 #undef symbol_clone_if_forward_ref
-symbolS *symbol_clone_if_forward_ref (symbolS *, int);
-#define symbol_clone_if_forward_ref(s) symbol_clone_if_forward_ref (s, 0)
+symbolS *symbol_clone_if_forward_ref (symbolS *, int, int);
+#define symbol_clone_if_forward_ref(s, d) symbol_clone_if_forward_ref (s, d, 0)
 symbolS *symbol_temp_new (segT, valueT, fragS *);
 symbolS *symbol_temp_new_now (void);
 symbolS *symbol_temp_make (void);

binutils-gas-dot.diff
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c	2010-08-27 02:24:02.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c	2010-08-27 03:02:23.000000000 +0100
@@ -659,8 +659,29 @@ symbol_clone_if_forward_ref (symbolS *sy
 	  || add_symbol != symbolP->sy_value.X_add_symbol
 	  || op_symbol != symbolP->sy_value.X_op_symbol)
 	{
+	  symbolS *temp_symbol;
+	  int is_temp_add;
+	  int is_temp_op;
+
 	  symbolP = symbol_clone (symbolP, 0);
 	  symbolP->sy_resolving = 0;
+	  if (!is_deferred)
+	    {
+	      is_temp_add = (add_symbol
+			     && SEG_NORMAL (add_symbol->bsym->section)
+			     && TC_FAKE_LABEL (S_GET_NAME (add_symbol)));
+	      is_temp_op = (op_symbol
+			    && SEG_NORMAL (op_symbol->bsym->section)
+			    && TC_FAKE_LABEL (S_GET_NAME (op_symbol)));
+	      if (is_temp_add || is_temp_op)
+		{
+		  temp_symbol = symbol_temp_new_now ();
+		  if (is_temp_add)
+		    add_symbol = temp_symbol;
+		  if (is_temp_op)
+		    op_symbol = temp_symbol;
+		}
+	    }
 	}
 
       symbolP->sy_value.X_add_symbol = add_symbol;
Index: binutils-fsf-trunk-quilt/gas/write.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/write.c	2010-08-27 02:16:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/write.c	2010-08-27 02:24:05.000000000 +0100
@@ -102,10 +102,6 @@
 #define MD_PCREL_FROM_SECTION(FIX, SEC) md_pcrel_from (FIX)
 #endif
 
-#ifndef TC_FAKE_LABEL
-#define TC_FAKE_LABEL(NAME) (strcmp ((NAME), FAKE_LABEL_NAME) == 0)
-#endif
-
 /* Positive values of TC_FX_SIZE_SLACK allow a target to define
    fixups that far past the end of a frag.  Having such fixups
    is of course most most likely a bug in setting fx_size correctly.
Index: binutils-fsf-trunk-quilt/gas/write.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/write.h	2010-08-27 02:16:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/write.h	2010-08-27 02:24:05.000000000 +0100
@@ -29,6 +29,10 @@
 #define FAKE_LABEL_NAME "L0\001"
 #endif
 
+#ifndef TC_FAKE_LABEL
+#define TC_FAKE_LABEL(NAME) (strcmp ((NAME), FAKE_LABEL_NAME) == 0)
+#endif
+
 #include "bit_fix.h"
 
 /*

Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol

[Richard Sandiford]

> @@ -645,18 +645,23 @@ symbol_clone_if_forward_ref (symbolS *sy
>  
>        /* Re-using sy_resolving here, as this routine cannot get called from
>  	 symbol resolution code.  */
> -      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
> +      if ((symbolP->bsym->section == expr_section
> +	   || (!is_deferred && symbolP->sy_forward_ref))
> +	  && !symbolP->sy_resolving)

Is the idea that if is_deferred is true, this function will be called
again each time symbolP is used in another (non-deferred) expression,
at which point the necessary cloning will happen?  If so, why do we need
to clone _any_ symbols in the deferred case?  I.e. for:

       .eqv     s2,s1
       .eqv     s3,s2
       .eqv     s4,s3
       ...

it seems that you're cloning:

 - s2, but not s1, for s3
 - s3, but not s2, for s4

and so on.  Couldn't we simply avoid calling symbol_clone_if_forward_ref
in the deferred case, and leave all cloning to the point of use?

The answer may well be "no". :-)  I'm just trying to understand the
reasoning.  Whatever the answer is, I think it needs a comment.

The first patch looks generally good otherwise, so hopefully any
changes are just a mopping-up exercise.  I'm still uneasy about the
second patch though.  Can you justify why the new test for "." is safe?

An alternative might be to model "." as a single forward-reference
symbol whose value is redefined before each instruction.  I think
that's what its semantics actually are.  There would then be a
single global symbol that represents ".", and that is marked as a
forward reference.  symbol_clone_if_forward_ref would return
symbol_temp_new_now for that symbol, rather than go through
the usual cloning process.

Richard

[PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference

[Maciej W. Rozycki]

Hi,

 Equated symbols (defined with .eqv) aka forward references are defined to 
reevaluate the expression they are defined to whenever they are referred 
to.  This does not happen for the special "dot" symbol -- the value 
calculated the first time the equated symbol has been used is then used 
over and over again.

 Additionally symbol_clone_if_forward_ref() would only be called 
recursively for internal symbols referring to the special expression 
section.  It should happen for all symbols.

 Overall with symbol definitions like this:

	.data
	.eqv	fnord, .
	.eqv	foobar, fnord + 4
	.eqv	foobaz, foobar - 16
	.word	0
	.word	fnord
	.word	fnord
	.word	foobaz
	.word	foobaz

the "dot" symbol would only be calculated once, with the second .word 
directive, i.e. both the second and the third word emitted would have the 
same value (address of the second word).

 This would also cause "foobaz" to be calculated (as a forward reference) 
with the third .eqv directive and then cloned each time when used with 
.word.  Howeved "foobar" would only be calculated with the second and the 
third .eqv directive, but not with the .word directives, thus fixed at 
the value of the first rather than each use.

 In the end data emitted would be:

0, 0, 0, -12, -12

as if .set was used, rather than:

0, 4, 8, 0, 4

as expected.

 To address the concerns you had with version 1.5 of this change:

> >        /* Re-using sy_resolving here, as this routine cannot get called from
> >  	 symbol resolution code.  */
> > -      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
> > +      if ((symbolP->bsym->section == expr_section
> > +	   || (!is_deferred && symbolP->sy_forward_ref))
> > +	  && !symbolP->sy_resolving)
> 
> Is the idea that if is_deferred is true, this function will be called
> again each time symbolP is used in another (non-deferred) expression,
> at which point the necessary cloning will happen?  If so, why do we need
> to clone _any_ symbols in the deferred case?  I.e. for:
> 
>        .eqv     s2,s1
>        .eqv     s3,s2
>        .eqv     s4,s3
>        ...
> 
> it seems that you're cloning:
> 
>  - s2, but not s1, for s3
>  - s3, but not s2, for s4
> 
> and so on.  Couldn't we simply avoid calling symbol_clone_if_forward_ref
> in the deferred case, and leave all cloning to the point of use?
> 
> The answer may well be "no". :-)  I'm just trying to understand the
> reasoning.  Whatever the answer is, I think it needs a comment.

 The answer is "yes" as far as I can tell and I have now implemented it.  
I have a feeling it was just an oversight by the original implementer.  
The new approach looks most natural to me -- thanks for the hint.

 In addition to the testsuites as in the introductory mail, this change 
(when applied together with its other half to address the "dot" special 
symbol) works correctly with all the snippets we quoted on the way.

2010-10-29  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* symbols.c (symbol_clone_if_forward_ref): Don't limit cloning
	to expr_section symbols; clone all equated symbols.  Clear
	sy_resolving of the cloned copy.
	* expr.c (operand): Only clone equated symbols on a final 
	(i.e. non-equated) reference.

 OK to apply?

  Maciej

binutils-gas-eqv.diff
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c	2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c	2010-10-29 09:07:10.000000000 +0100
@@ -645,7 +645,8 @@ symbol_clone_if_forward_ref (symbolS *sy
 
       /* Re-using sy_resolving here, as this routine cannot get called from
 	 symbol resolution code.  */
-      if (symbolP->bsym->section == expr_section && !symbolP->sy_resolving)
+      if ((symbolP->bsym->section == expr_section || symbolP->sy_forward_ref)
+	  && !symbolP->sy_resolving)
 	{
 	  symbolP->sy_resolving = 1;
 	  add_symbol = symbol_clone_if_forward_ref (add_symbol, is_forward);
@@ -656,7 +657,10 @@ symbol_clone_if_forward_ref (symbolS *sy
       if (symbolP->sy_forward_ref
 	  || add_symbol != symbolP->sy_value.X_add_symbol
 	  || op_symbol != symbolP->sy_value.X_op_symbol)
-	symbolP = symbol_clone (symbolP, 0);
+	{
+	  symbolP = symbol_clone (symbolP, 0);
+	  symbolP->sy_resolving = 0;
+	}
 
       symbolP->sy_value.X_add_symbol = add_symbol;
       symbolP->sy_value.X_op_symbol = op_symbol;
Index: binutils-fsf-trunk-quilt/gas/expr.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/expr.c	2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/expr.c	2010-10-29 09:07:10.000000000 +0100
@@ -1373,8 +1373,13 @@ operand (expressionS *expressionP, enum 
   if (expressionP->X_add_symbol)
     symbol_mark_used (expressionP->X_add_symbol);
 
-  expressionP->X_add_symbol = symbol_clone_if_forward_ref (expressionP->X_add_symbol);
-  expressionP->X_op_symbol = symbol_clone_if_forward_ref (expressionP->X_op_symbol);
+  if (mode != expr_defer)
+    {
+      expressionP->X_add_symbol
+	= symbol_clone_if_forward_ref (expressionP->X_add_symbol);
+      expressionP->X_op_symbol
+	= symbol_clone_if_forward_ref (expressionP->X_op_symbol);
+    }
 
   switch (expressionP->X_op)
     {

Re: [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 2010-10-29  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	gas/
> 	* symbols.c (symbol_clone_if_forward_ref): Don't limit cloning
> 	to expr_section symbols; clone all equated symbols.  Clear
> 	sy_resolving of the cloned copy.
> 	* expr.c (operand): Only clone equated symbols on a final 
> 	(i.e. non-equated) reference.
>
>  OK to apply?

OK once 2.21 has branched, thanks.

Richard

Re: [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference

[Maciej W. Rozycki]

On Sat, 30 Oct 2010, Richard Sandiford wrote:

> > 	gas/
> > 	* symbols.c (symbol_clone_if_forward_ref): Don't limit cloning
> > 	to expr_section symbols; clone all equated symbols.  Clear
> > 	sy_resolving of the cloned copy.
> > 	* expr.c (operand): Only clone equated symbols on a final 
> > 	(i.e. non-equated) reference.
> >
> >  OK to apply?
> 
> OK once 2.21 has branched, thanks.

 Applied now, thanks.

  Maciej

[PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

Hi,

 Equated symbols (defined with .eqv) aka forward references are defined to 
reevaluate the expression they are defined to whenever they are referred 
to.  This does not happen for the special "dot" symbol -- the value 
calculated the first time the equated symbol has been used is then used 
over and over again.

 The reason is each time a reference to the "dot" symbol is made a special 
fake label is created.  This label is then recorded in the chain of 
symbols the equated symbol is defined to and never reevaluated.  The 
solution is to create a new fake label each time the equated symbol is 
used.

 Overall with symbol definitions like this:

	.data
	.eqv	fnord, .
	.eqv	foobar, fnord + 4
	.eqv	foobaz, foobar - 16
	.word	0
	.word	fnord
	.word	fnord
	.word	foobaz
	.word	foobaz

the "dot" symbol would only be calculated once, with the second .word 
directive, i.e. both the second and the third word emitted would have the 
same value (address of the second word).

 This would also cause "foobaz" to be calculated (as a forward reference) 
with the third .eqv directive and then cloned each time when used with 
.word.  Howeved "foobar" would only be calculated with the second and the 
third .eqv directive, but not with the .word directives, thus fixed at 
the value of the first rather than each use.

 In the end data emitted would be:

0, 0, 0, -12, -12

as if .set was used, rather than:

0, 4, 8, 0, 4

as expected.

 To address the concerns you had with version 1.5 of this change:

> The first patch looks generally good otherwise, so hopefully any
> changes are just a mopping-up exercise.  I'm still uneasy about the
> second patch though.  Can you justify why the new test for "." is safe?
> 
> An alternative might be to model "." as a single forward-reference
> symbol whose value is redefined before each instruction.  I think
> that's what its semantics actually are.  There would then be a
> single global symbol that represents ".", and that is marked as a
> forward reference.  symbol_clone_if_forward_ref would return
> symbol_temp_new_now for that symbol, rather than go through
> the usual cloning process.

 That sounded like a brilliant idea to me, so I have implemented it.  I 
have created a special dot_symbol tracking symbol for this purpose.  It 
has to be an external symbol so that GAS doesn't attempt to turn it 
into the so called converted symbol, so it has to be initialised pretty 
late in the game, when output BFD has already been opened, making 
symbol_begin() an unsuitable place to do that.  I have thus created 
dot_symbol_init() to do all the necessary bits and even gave it the name 
of "." that might surprise everyone.  The name should never be reached 
though, because "." is trapped as a special case throughout GAS.

 Of all the other changes I think only bits in read_a_source_file() 
require further explanation.  As I discovered there are places in GAS that 
refer to a symbol's segment early on, long before it is resolved, for the 
purpose of validation relative expressions (generally address 
subtraction).  Rather than just setting the segment only I reused the 
existing symbol_set_value_now() helper that completely updates the symbol, 
making dot_symbol truly track the current location.

 A nice effect of this arrangement (or actually the second statement of 
this form that immediately follows the parser loop, unless .end has been 
used) is that at the end of assembly dot_symbol points to just at the end 
of generated output.  As all equated symbols are actually recorded in the 
generated object's symbol table just as ordinary ones, their values are 
calculated according to the expression they have been equated to at the 
final symbol resolution stage.  For symbols that have been equated to an 
expression involving the special dot symbol this means the final recorded 
value will be based on the final value of the location counter -- just as 
you'd expect by the semantics of the equation operation. :)

 Where applicable I have placed the relevant calls before calls to target 
hooks so that they are presented with a consistent world.

2010-10-29  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	* symbols.h (dot_symbol): New declaration.
	(dot_symbol_init): New prototype.
	* symbols.c (dot_symbol): New variable.
	(dot_symbol_init): New function.
	(symbol_clone_if_forward_ref): Create a new temporary symbol
	when trying to clone dot_symbol.
	* expr.c (current_location): Refer to dot_symbol instead of
	making a new temporary symbol.
	* read.c (read_a_source_file): Update dot_symbol as we go.
	* as.c (main): Call dot_symbol_init.

 OK to apply?

  Maciej

binutils-gas-dot.diff
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c	2010-10-29 09:07:10.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.c	2010-10-29 09:07:10.000000000 +0100
@@ -48,6 +48,7 @@ static struct hash_control *local_hash;
 symbolS *symbol_rootP;
 symbolS *symbol_lastP;
 symbolS abs_symbol;
+symbolS dot_symbol;
 
 #ifdef DEBUG_SYMS
 #define debug_verify_symchain verify_symbol_chain
@@ -658,8 +659,13 @@ symbol_clone_if_forward_ref (symbolS *sy
 	  || add_symbol != symbolP->sy_value.X_add_symbol
 	  || op_symbol != symbolP->sy_value.X_op_symbol)
 	{
-	  symbolP = symbol_clone (symbolP, 0);
-	  symbolP->sy_resolving = 0;
+	  if (symbolP != &dot_symbol)
+	    {
+	      symbolP = symbol_clone (symbolP, 0);
+	      symbolP->sy_resolving = 0;
+	    }
+	  else
+	    symbolP = symbol_temp_new_now ();
 	}
 
       symbolP->sy_value.X_add_symbol = add_symbol;
@@ -2749,6 +2755,17 @@ symbol_begin (void)
   if (LOCAL_LABELS_FB)
     fb_label_init ();
 }
+
+void
+dot_symbol_init (void)
+{
+  dot_symbol.bsym = bfd_make_empty_symbol (stdoutput);
+  if (dot_symbol.bsym == NULL)
+    as_fatal ("bfd_make_empty_symbol: %s", bfd_errmsg (bfd_get_error ()));
+  S_SET_NAME (&dot_symbol, ".");
+  S_SET_FORWARD_REF (&dot_symbol);
+  dot_symbol.sy_value.X_op = O_constant;
+}
 \f
 int indent_level;
 
Index: binutils-fsf-trunk-quilt/gas/expr.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/expr.c	2010-10-29 09:07:10.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/expr.c	2010-10-29 09:07:10.000000000 +0100
@@ -705,7 +705,7 @@ current_location (expressionS *expressio
   else
     {
       expressionp->X_op = O_symbol;
-      expressionp->X_add_symbol = symbol_temp_new_now ();
+      expressionp->X_add_symbol = &dot_symbol;
       expressionp->X_add_number = 0;
     }
 }
Index: binutils-fsf-trunk-quilt/gas/symbols.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.h	2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/symbols.h	2010-10-29 09:07:10.000000000 +0100
@@ -28,6 +28,7 @@ extern symbolS *symbol_rootP;	/* all the
 extern symbolS *symbol_lastP;	/* last struct symbol we made, or NULL */
 
 extern symbolS abs_symbol;
+extern symbolS dot_symbol;
 
 extern int symbol_table_frozen;
 
@@ -60,6 +61,7 @@ symbolS *symbol_temp_make (void);
 symbolS *colon (const char *sym_name);
 void local_colon (int n);
 void symbol_begin (void);
+void dot_symbol_init (void);
 void symbol_print_statistics (FILE *);
 void symbol_table_insert (symbolS * symbolP);
 valueT resolve_symbol_value (symbolS *);
Index: binutils-fsf-trunk-quilt/gas/read.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/read.c	2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/read.c	2010-10-29 09:07:10.000000000 +0100
@@ -629,6 +629,7 @@ read_a_source_file (char *name)
 	  was_new_line = is_end_of_line[(unsigned char) input_line_pointer[-1]];
 	  if (was_new_line)
 	    {
+	      symbol_set_value_now (&dot_symbol);
 #ifdef md_start_line_hook
 	      md_start_line_hook ();
 #endif
@@ -1128,6 +1129,7 @@ read_a_source_file (char *name)
       md_after_pass_hook ();
 #endif
     }
+  symbol_set_value_now (&dot_symbol);
 
  quit:
 
Index: binutils-fsf-trunk-quilt/gas/as.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/as.c	2010-10-29 09:07:06.000000000 +0100
+++ binutils-fsf-trunk-quilt/gas/as.c	2010-10-29 09:07:10.000000000 +0100
@@ -1181,6 +1181,8 @@ main (int argc, char ** argv)
   output_file_create (out_file_name);
   gas_assert (stdoutput != 0);
 
+  dot_symbol_init ();
+
 #ifdef tc_init_after_args
   tc_init_after_args ();
 #endif

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 2010-10-29  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	gas/
> 	* symbols.h (dot_symbol): New declaration.
> 	(dot_symbol_init): New prototype.
> 	* symbols.c (dot_symbol): New variable.
> 	(dot_symbol_init): New function.
> 	(symbol_clone_if_forward_ref): Create a new temporary symbol
> 	when trying to clone dot_symbol.
> 	* expr.c (current_location): Refer to dot_symbol instead of
> 	making a new temporary symbol.
> 	* read.c (read_a_source_file): Update dot_symbol as we go.
> 	* as.c (main): Call dot_symbol_init.

Looks good.  However, 

gcc (Debian 4.4.4-8) 4.4.5 20100728 (prerelease)

complains about so-called aliasing violations on these calls:

> +  S_SET_NAME (&dot_symbol, ".");
> +  S_SET_FORWARD_REF (&dot_symbol);

It's inlining the calls and warning about the local case, which obviously
doesn't apply here.  I know it's not usually good practice to recode to
avoid bogus warnings, but let's use:

  dot_symbol.bsym->name = ".";
  dot_symbol.sy_forward_ref = 1;

anyway.  Could you also add a gcc_assert to symbol_clone to make sure
that we don't accidentally clone dot_symbol in other cases?

OK with those changes once 2.21 has branched, thanks.

Richard

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

On Sat, 30 Oct 2010, Richard Sandiford wrote:

> Looks good.  However, 
> 
> gcc (Debian 4.4.4-8) 4.4.5 20100728 (prerelease)
> 
> complains about so-called aliasing violations on these calls:
> 
> > +  S_SET_NAME (&dot_symbol, ".");
> > +  S_SET_FORWARD_REF (&dot_symbol);
> 
> It's inlining the calls and warning about the local case, which obviously
> doesn't apply here.  I know it's not usually good practice to recode to
> avoid bogus warnings, but let's use:
> 
>   dot_symbol.bsym->name = ".";
>   dot_symbol.sy_forward_ref = 1;
> 
> anyway.

 Hmm, TBH I didn't like the way these structs are type-punned from the 
very first moment I realised what was going on here, but GCC 4.3.2 doesn't 
seem as picky, so I let it be.  The complaint about the risk of alias 
violations with these structs is IMO legitimate; if not here, then 
elsewhere.  And since this is 2.22 material anyway, how about we do this 
properly and convert symbolS to make it a union of the two cases: local 
and external?  I mean:

union symbol
{
  struct local_symbol l;
  struct external_symbol e;
};
typedef union symbol symbolS;

where "struct local_symbol" would remain unchanged and the current "struct 
symbol" would become "struct external_symbol".

 I realise syntactically that's going to be an extensive change, but 
mostly if not entirely mechanical and will improve the overall quality of 
code involved.  And if anytime, freshly after a release branch has been 
forked off is I think the best moment to make such changes.  What do you 
think?

> Could you also add a gcc_assert to symbol_clone to make sure that we 
> don't accidentally clone dot_symbol in other cases?

 Sure.

> OK with those changes once 2.21 has branched, thanks.

 OK, fair enough, all of this is indeed subtle.  I'll wait and commit 
these changes in due course.  Thanks for your review.

  Maciej

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> On Sat, 30 Oct 2010, Richard Sandiford wrote:
>> Looks good.  However, 
>> 
>> gcc (Debian 4.4.4-8) 4.4.5 20100728 (prerelease)
>> 
>> complains about so-called aliasing violations on these calls:
>> 
>> > +  S_SET_NAME (&dot_symbol, ".");
>> > +  S_SET_FORWARD_REF (&dot_symbol);
>> 
>> It's inlining the calls and warning about the local case, which obviously
>> doesn't apply here.  I know it's not usually good practice to recode to
>> avoid bogus warnings, but let's use:
>> 
>>   dot_symbol.bsym->name = ".";
>>   dot_symbol.sy_forward_ref = 1;
>> 
>> anyway.
>
>  Hmm, TBH I didn't like the way these structs are type-punned from the 
> very first moment I realised what was going on here, but GCC 4.3.2 doesn't 
> seem as picky, so I let it be.  The complaint about the risk of alias 
> violations with these structs is IMO legitimate; if not here, then 
> elsewhere.  And since this is 2.22 material anyway, how about we do this 
> properly and convert symbolS to make it a union of the two cases: local 
> and external?  I mean:
>
> union symbol
> {
>   struct local_symbol l;
>   struct external_symbol e;
> };
> typedef union symbol symbolS;
>
> where "struct local_symbol" would remain unchanged and the current "struct 
> symbol" would become "struct external_symbol".
>
>  I realise syntactically that's going to be an extensive change, but 
> mostly if not entirely mechanical and will improve the overall quality of 
> code involved.  And if anytime, freshly after a release branch has been 
> forked off is I think the best moment to make such changes.  What do you 
> think?

Sounds good, but please don't let it get in the way of applying this patch
(when the time comes).  The change above seems entirely in keeping with
the way that abs_symbol is initialised.

Richard

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

On Mon, 1 Nov 2010, Richard Sandiford wrote:

> Sounds good, but please don't let it get in the way of applying this patch
> (when the time comes).  The change above seems entirely in keeping with
> the way that abs_symbol is initialised.

 This is the version I applied.

2010-12-01  Maciej W. Rozycki  <macro@codesourcery.com>

	* symbols.h (dot_symbol): New declaration.
	(dot_symbol_init): New prototype.
	* symbols.c (dot_symbol): New variable.
	(symbol_clone): Assert it's not dot_symbol being cloned.
	(dot_symbol_init): New function.
	(symbol_clone_if_forward_ref): Create a new temporary symbol
	when trying to clone dot_symbol.
	* expr.c (current_location): Refer to dot_symbol instead of
	making a new temporary symbol.
	* read.c (read_a_source_file): Update dot_symbol as we go.
	* as.c (main): Call dot_symbol_init.

 Thanks,

  Maciej

binutils-gas-dot.diff
Index: binutils-fsf-trunk-quilt/gas/symbols.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.c	2010-12-01 21:05:48.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/symbols.c	2010-12-01 21:05:48.000000000 +0000
@@ -48,6 +48,7 @@ static struct hash_control *local_hash;
 symbolS *symbol_rootP;
 symbolS *symbol_lastP;
 symbolS abs_symbol;
+symbolS dot_symbol;
 
 #ifdef DEBUG_SYMS
 #define debug_verify_symchain verify_symbol_chain
@@ -557,6 +558,9 @@ symbol_clone (symbolS *orgsymP, int repl
   symbolS *newsymP;
   asymbol *bsymorg, *bsymnew;
 
+  /* Make sure we never clone the dot special symbol.  */
+  gas_assert (orgsymP != &dot_symbol);
+
   /* Running local_symbol_convert on a clone that's not the one currently
      in local_hash would incorrectly replace the hash entry.  Thus the
      symbol must be converted here.  Note that the rest of the function
@@ -658,8 +662,13 @@ symbol_clone_if_forward_ref (symbolS *sy
 	  || add_symbol != symbolP->sy_value.X_add_symbol
 	  || op_symbol != symbolP->sy_value.X_op_symbol)
 	{
-	  symbolP = symbol_clone (symbolP, 0);
-	  symbolP->sy_resolving = 0;
+	  if (symbolP != &dot_symbol)
+	    {
+	      symbolP = symbol_clone (symbolP, 0);
+	      symbolP->sy_resolving = 0;
+	    }
+	  else
+	    symbolP = symbol_temp_new_now ();
 	}
 
       symbolP->sy_value.X_add_symbol = add_symbol;
@@ -2749,6 +2758,17 @@ symbol_begin (void)
   if (LOCAL_LABELS_FB)
     fb_label_init ();
 }
+
+void
+dot_symbol_init (void)
+{
+  dot_symbol.bsym = bfd_make_empty_symbol (stdoutput);
+  if (dot_symbol.bsym == NULL)
+    as_fatal ("bfd_make_empty_symbol: %s", bfd_errmsg (bfd_get_error ()));
+  dot_symbol.bsym->name = ".";
+  dot_symbol.sy_forward_ref = 1;
+  dot_symbol.sy_value.X_op = O_constant;
+}
 \f
 int indent_level;
 
Index: binutils-fsf-trunk-quilt/gas/expr.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/expr.c	2010-12-01 21:05:48.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/expr.c	2010-12-01 21:05:48.000000000 +0000
@@ -705,7 +705,7 @@ current_location (expressionS *expressio
   else
     {
       expressionp->X_op = O_symbol;
-      expressionp->X_add_symbol = symbol_temp_new_now ();
+      expressionp->X_add_symbol = &dot_symbol;
       expressionp->X_add_number = 0;
     }
 }
Index: binutils-fsf-trunk-quilt/gas/symbols.h
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/symbols.h	2010-12-01 21:05:41.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/symbols.h	2010-12-01 21:05:48.000000000 +0000
@@ -28,6 +28,7 @@ extern symbolS *symbol_rootP;	/* all the
 extern symbolS *symbol_lastP;	/* last struct symbol we made, or NULL */
 
 extern symbolS abs_symbol;
+extern symbolS dot_symbol;
 
 extern int symbol_table_frozen;
 
@@ -60,6 +61,7 @@ symbolS *symbol_temp_make (void);
 symbolS *colon (const char *sym_name);
 void local_colon (int n);
 void symbol_begin (void);
+void dot_symbol_init (void);
 void symbol_print_statistics (FILE *);
 void symbol_table_insert (symbolS * symbolP);
 valueT resolve_symbol_value (symbolS *);
Index: binutils-fsf-trunk-quilt/gas/read.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/read.c	2010-12-01 21:05:41.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/read.c	2010-12-01 21:05:48.000000000 +0000
@@ -629,6 +629,7 @@ read_a_source_file (char *name)
 	  was_new_line = is_end_of_line[(unsigned char) input_line_pointer[-1]];
 	  if (was_new_line)
 	    {
+	      symbol_set_value_now (&dot_symbol);
 #ifdef md_start_line_hook
 	      md_start_line_hook ();
 #endif
@@ -1128,6 +1129,7 @@ read_a_source_file (char *name)
       md_after_pass_hook ();
 #endif
     }
+  symbol_set_value_now (&dot_symbol);
 
  quit:
 
Index: binutils-fsf-trunk-quilt/gas/as.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/as.c	2010-12-01 21:05:41.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/as.c	2010-12-01 21:05:48.000000000 +0000
@@ -1181,6 +1181,8 @@ main (int argc, char ** argv)
   output_file_create (out_file_name);
   gas_assert (stdoutput != 0);
 
+  dot_symbol_init ();
+
 #ifdef tc_init_after_args
   tc_init_after_args ();
 #endif

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Jie Zhang]

On 12/02/2010 05:34 AM, Maciej W. Rozycki wrote:
> On Mon, 1 Nov 2010, Richard Sandiford wrote:
>
>> Sounds good, but please don't let it get in the way of applying this patch
>> (when the time comes).  The change above seems entirely in keeping with
>> the way that abs_symbol is initialised.
>
>   This is the version I applied.
>
> 2010-12-01  Maciej W. Rozycki<macro@codesourcery.com>
>
> 	* symbols.h (dot_symbol): New declaration.
> 	(dot_symbol_init): New prototype.
> 	* symbols.c (dot_symbol): New variable.
> 	(symbol_clone): Assert it's not dot_symbol being cloned.
> 	(dot_symbol_init): New function.
> 	(symbol_clone_if_forward_ref): Create a new temporary symbol
> 	when trying to clone dot_symbol.
> 	* expr.c (current_location): Refer to dot_symbol instead of
> 	making a new temporary symbol.
> 	* read.c (read_a_source_file): Update dot_symbol as we go.
> 	* as.c (main): Call dot_symbol_init.
>
This caused

http://sourceware.org/bugzilla/show_bug.cgi?id=12282


Regards,
-- 
Jie Zhang

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Richard Earnshaw]

On Wed, 2010-12-01 at 21:34 +0000, Maciej W. Rozycki wrote:
> On Mon, 1 Nov 2010, Richard Sandiford wrote:
> 
> > Sounds good, but please don't let it get in the way of applying this patch
> > (when the time comes).  The change above seems entirely in keeping with
> > the way that abs_symbol is initialised.
> 
>  This is the version I applied.
> 
> 2010-12-01  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	* symbols.h (dot_symbol): New declaration.
> 	(dot_symbol_init): New prototype.
> 	* symbols.c (dot_symbol): New variable.
> 	(symbol_clone): Assert it's not dot_symbol being cloned.
> 	(dot_symbol_init): New function.
> 	(symbol_clone_if_forward_ref): Create a new temporary symbol
> 	when trying to clone dot_symbol.
> 	* expr.c (current_location): Refer to dot_symbol instead of
> 	making a new temporary symbol.
> 	* read.c (read_a_source_file): Update dot_symbol as we go.
> 	* as.c (main): Call dot_symbol_init.

So I'm not entirely sure that this patch is responsible, but it does
look a likely candidate...

	.section	.text._ZnajPv,"axG",%progbits,_ZnajPv,comdat
	.fnstart
	.fnend

/arm/scratch/rearnsha/gnu/gcc-results/trunk/gcc/../gas/as-new
-o /tmp/x.o test.s
test.s: Assembler messages:
test.s:3: Error: redefined symbol cannot be used on reloc
/arm/scratch/rearnsha/gnu/gcc-results/trunk/gcc/../gas/as-new: /tmp/x.o:
symbol `.' required but not present
test.s:3: Fatal error: can't close /tmp/x.o: No symbols

This is on arm-eabi.

R

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

Hi Richard,

> > > Sounds good, but please don't let it get in the way of applying this patch
> > > (when the time comes).  The change above seems entirely in keeping with
> > > the way that abs_symbol is initialised.
> > 
> >  This is the version I applied.
> > 
> > 2010-12-01  Maciej W. Rozycki  <macro@codesourcery.com>
> > 
> > 	* symbols.h (dot_symbol): New declaration.
> > 	(dot_symbol_init): New prototype.
> > 	* symbols.c (dot_symbol): New variable.
> > 	(symbol_clone): Assert it's not dot_symbol being cloned.
> > 	(dot_symbol_init): New function.
> > 	(symbol_clone_if_forward_ref): Create a new temporary symbol
> > 	when trying to clone dot_symbol.
> > 	* expr.c (current_location): Refer to dot_symbol instead of
> > 	making a new temporary symbol.
> > 	* read.c (read_a_source_file): Update dot_symbol as we go.
> > 	* as.c (main): Call dot_symbol_init.
> 
> So I'm not entirely sure that this patch is responsible, but it does
> look a likely candidate...
> 
> 	.section	.text._ZnajPv,"axG",%progbits,_ZnajPv,comdat
> 	.fnstart
> 	.fnend
> 
> /arm/scratch/rearnsha/gnu/gcc-results/trunk/gcc/../gas/as-new
> -o /tmp/x.o test.s
> test.s: Assembler messages:
> test.s:3: Error: redefined symbol cannot be used on reloc
> /arm/scratch/rearnsha/gnu/gcc-results/trunk/gcc/../gas/as-new: /tmp/x.o:
> symbol `.' required but not present
> test.s:3: Fatal error: can't close /tmp/x.o: No symbols
> 
> This is on arm-eabi.

 Thanks for reducing the test case.  Below I'm including a change that is 
supposed to fix it.  The original change indeed is what triggered the 
problem, but from the code I have modified I infer it is more related to 
the issue with equated symbols not being handled correctly that I have 
fixed as well.  This function counts as a symbol reference and as such 
should resolve any equated symbols by making a clone.  Chances are code 
could have been crafted that would trigger a problem here even before my 
fixes, but I found no justification to spend time investigating that.

 The change below fixes the problem with your test case as well as one 
attached to PR gas/12282.  I have regression-tested it with the 
arm-none-eabi and mips-sde-elf targets.

2010-12-02  Maciej W. Rozycki  <macro@codesourcery.com>

	PR gas/12282
	* expr.c (make_expr_symbol): Make a clone if handling an
	equated symbol.

 OK to apply?

  Maciej

binutils-gas-dot-fix.diff
Index: binutils-fsf-trunk-quilt/gas/expr.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/expr.c	2010-12-02 22:10:55.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/expr.c	2010-12-02 22:11:31.000000000 +0000
@@ -76,7 +76,7 @@ make_expr_symbol (expressionS *expressio
 
   if (expressionP->X_op == O_symbol
       && expressionP->X_add_number == 0)
-    return expressionP->X_add_symbol;
+    return symbol_clone_if_forward_ref (expressionP->X_add_symbol);
 
   if (expressionP->X_op == O_big)
     {

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  Thanks for reducing the test case.  Below I'm including a change that is 
> supposed to fix it.  The original change indeed is what triggered the 
> problem, but from the code I have modified I infer it is more related to 
> the issue with equated symbols not being handled correctly that I have 
> fixed as well.  This function counts as a symbol reference and as such 
> should resolve any equated symbols by making a clone.  Chances are code 
> could have been crafted that would trigger a problem here even before my 
> fixes, but I found no justification to spend time investigating that.
>
>  The change below fixes the problem with your test case as well as one 
> attached to PR gas/12282.  I have regression-tested it with the 
> arm-none-eabi and mips-sde-elf targets.
>
> 2010-12-02  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	PR gas/12282
> 	* expr.c (make_expr_symbol): Make a clone if handling an
> 	equated symbol.

I'm not convinced this is correct when operand() calls make_expr_symbol()
in expr_defer mode.  Could you provide a bit more justification?

Richard

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Maciej W. Rozycki]

On Fri, 3 Dec 2010, Richard Sandiford wrote:

> >  Thanks for reducing the test case.  Below I'm including a change that is 
> > supposed to fix it.  The original change indeed is what triggered the 
> > problem, but from the code I have modified I infer it is more related to 
> > the issue with equated symbols not being handled correctly that I have 
> > fixed as well.  This function counts as a symbol reference and as such 
> > should resolve any equated symbols by making a clone.  Chances are code 
> > could have been crafted that would trigger a problem here even before my 
> > fixes, but I found no justification to spend time investigating that.
> >
> >  The change below fixes the problem with your test case as well as one 
> > attached to PR gas/12282.  I have regression-tested it with the 
> > arm-none-eabi and mips-sde-elf targets.
> >
> > 2010-12-02  Maciej W. Rozycki  <macro@codesourcery.com>
> >
> > 	PR gas/12282
> > 	* expr.c (make_expr_symbol): Make a clone if handling an
> > 	equated symbol.
> 
> I'm not convinced this is correct when operand() calls make_expr_symbol()
> in expr_defer mode.  Could you provide a bit more justification?

 Hmm, good point, thanks, and expr() too.  I'm not sure why I missed it.

 I have given it some more thought and come to the conclusion it has to be 
decided by the caller of make_expr_symbol() whether the symbol returned 
should be equated or cloned.  Assuming optimistically all the callers 
other than operand(), expr() and expr_build_dot() considered here do the 
right thing, i.e. either use make_expr_symbol() on a non-symbol or cope 
with forward references I propose to fix expr_build_dot() only, like this.  
If more places are eventually required, then a wrapper function or macro 
can be created to combine make_expr_symbol() and 
symbol_clone_if_forward_ref().

  The change below fixes the problem with the two test cases mentioned 
earlier.  I have regression-tested it with the arm-none-eabi and 
mips-sde-elf targets.

2010-12-03  Maciej W. Rozycki  <macro@codesourcery.com>

 	PR gas/12282
 	* expr.c (expr_build_dot): Make a clone of the symbol to return if 
	needed.

 OK to apply?

  Maciej

binutils-gas-dot-fix.diff
Index: binutils-fsf-trunk-quilt/gas/expr.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/expr.c	2010-12-03 14:15:18.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/expr.c	2010-12-03 15:13:54.000000000 +0000
@@ -172,7 +172,7 @@ expr_build_dot (void)
   expressionS e;
 
   current_location (&e);
-  return make_expr_symbol (&e);
+  return symbol_clone_if_forward_ref (make_expr_symbol (&e));
 }
 \f
 /* Build any floating-point literal here.

Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  I have given it some more thought and come to the conclusion it has to be 
> decided by the caller of make_expr_symbol() whether the symbol returned 
> should be equated or cloned.  Assuming optimistically all the callers 
> other than operand(), expr() and expr_build_dot() considered here do the 
> right thing, i.e. either use make_expr_symbol() on a non-symbol or cope 
> with forward references I propose to fix expr_build_dot() only, like this.  
> If more places are eventually required, then a wrapper function or macro 
> can be created to combine make_expr_symbol() and 
> symbol_clone_if_forward_ref().

Yeah, sounds like a good plan.

>   The change below fixes the problem with the two test cases mentioned 
> earlier.  I have regression-tested it with the arm-none-eabi and 
> mips-sde-elf targets.
>
> 2010-12-03  Maciej W. Rozycki  <macro@codesourcery.com>
>
>  	PR gas/12282
>  	* expr.c (expr_build_dot): Make a clone of the symbol to return if 
> 	needed.

OK, thanks.

Richard