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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

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)
     {