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

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

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"
 
 /*