* [PATCH 2/4] GAS: Make new fake labels when cloning a symbol
@ 2010-07-26 10:47 Maciej W. Rozycki
2010-07-31 8:59 ` Richard Sandiford
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-07-26 10:47 UTC (permalink / raw)
To: binutils; +Cc: Richard Sandiford, Catherine Moore, gnu-mips-sgxx
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"
/*
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol
2010-07-26 10:47 [PATCH 2/4] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
@ 2010-07-31 8:59 ` Richard Sandiford
2010-08-27 12:11 ` Maciej W. Rozycki
2010-10-29 14:37 ` [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference Maciej W. Rozycki
2010-10-29 14:38 ` [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
2 siblings, 1 reply; 18+ messages in thread
From: Richard Sandiford @ 2010-07-31 8:59 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils, Catherine Moore, gnu-mips-sgxx
"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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol
2010-07-31 8:59 ` Richard Sandiford
@ 2010-08-27 12:11 ` Maciej W. Rozycki
2010-08-30 17:53 ` Richard Sandiford
0 siblings, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-08-27 12:11 UTC (permalink / raw)
To: Richard Sandiford; +Cc: binutils, Catherine Moore, gnu-mips-sgxx
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"
/*
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol
2010-08-27 12:11 ` Maciej W. Rozycki
@ 2010-08-30 17:53 ` Richard Sandiford
0 siblings, 0 replies; 18+ messages in thread
From: Richard Sandiford @ 2010-08-30 17:53 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils, Catherine Moore, gnu-mips-sgxx
> @@ -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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference
2010-07-26 10:47 [PATCH 2/4] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
2010-07-31 8:59 ` Richard Sandiford
@ 2010-10-29 14:37 ` Maciej W. Rozycki
2010-10-30 9:56 ` Richard Sandiford
2010-10-29 14:38 ` [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
2 siblings, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-10-29 14:37 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
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)
{
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-07-26 10:47 [PATCH 2/4] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
2010-07-31 8:59 ` Richard Sandiford
2010-10-29 14:37 ` [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference Maciej W. Rozycki
@ 2010-10-29 14:38 ` Maciej W. Rozycki
2010-10-30 10:01 ` Richard Sandiford
2 siblings, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-10-29 14:38 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference
2010-10-29 14:37 ` [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference Maciej W. Rozycki
@ 2010-10-30 9:56 ` Richard Sandiford
2010-12-01 20:35 ` Maciej W. Rozycki
0 siblings, 1 reply; 18+ messages in thread
From: Richard Sandiford @ 2010-10-30 9:56 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Catherine Moore, binutils
"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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-10-29 14:38 ` [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
@ 2010-10-30 10:01 ` Richard Sandiford
2010-11-01 12:23 ` Maciej W. Rozycki
0 siblings, 1 reply; 18+ messages in thread
From: Richard Sandiford @ 2010-10-30 10:01 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Catherine Moore, binutils
"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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-10-30 10:01 ` Richard Sandiford
@ 2010-11-01 12:23 ` Maciej W. Rozycki
2010-11-01 21:32 ` Richard Sandiford
0 siblings, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-11-01 12:23 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-11-01 12:23 ` Maciej W. Rozycki
@ 2010-11-01 21:32 ` Richard Sandiford
2010-12-01 21:34 ` Maciej W. Rozycki
0 siblings, 1 reply; 18+ messages in thread
From: Richard Sandiford @ 2010-11-01 21:32 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Catherine Moore, binutils
"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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference
2010-10-30 9:56 ` Richard Sandiford
@ 2010-12-01 20:35 ` Maciej W. Rozycki
0 siblings, 0 replies; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-12-01 20:35 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-11-01 21:32 ` Richard Sandiford
@ 2010-12-01 21:34 ` Maciej W. Rozycki
2010-12-02 10:00 ` Jie Zhang
2010-12-02 17:01 ` Richard Earnshaw
0 siblings, 2 replies; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-12-01 21:34 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Catherine Moore, binutils
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-12-01 21:34 ` Maciej W. Rozycki
@ 2010-12-02 10:00 ` Jie Zhang
2010-12-02 17:01 ` Richard Earnshaw
1 sibling, 0 replies; 18+ messages in thread
From: Jie Zhang @ 2010-12-02 10:00 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Richard Sandiford, Catherine Moore, binutils
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-12-01 21:34 ` Maciej W. Rozycki
2010-12-02 10:00 ` Jie Zhang
@ 2010-12-02 17:01 ` Richard Earnshaw
2010-12-02 23:19 ` Maciej W. Rozycki
1 sibling, 1 reply; 18+ messages in thread
From: Richard Earnshaw @ 2010-12-02 17:01 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Richard Sandiford, Catherine Moore, binutils
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-12-02 17:01 ` Richard Earnshaw
@ 2010-12-02 23:19 ` Maciej W. Rozycki
2010-12-03 10:58 ` Richard Sandiford
0 siblings, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-12-02 23:19 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: Richard Sandiford, Jie Zhang, Catherine Moore, binutils
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)
{
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-12-02 23:19 ` Maciej W. Rozycki
@ 2010-12-03 10:58 ` Richard Sandiford
2010-12-03 16:20 ` Maciej W. Rozycki
0 siblings, 1 reply; 18+ messages in thread
From: Richard Sandiford @ 2010-12-03 10:58 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Richard Earnshaw, Jie Zhang, Catherine Moore, binutils
"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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-12-03 10:58 ` Richard Sandiford
@ 2010-12-03 16:20 ` Maciej W. Rozycki
2010-12-03 16:47 ` Richard Sandiford
0 siblings, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2010-12-03 16:20 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Richard Earnshaw, Jie Zhang, Catherine Moore, binutils
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol
2010-12-03 16:20 ` Maciej W. Rozycki
@ 2010-12-03 16:47 ` Richard Sandiford
0 siblings, 0 replies; 18+ messages in thread
From: Richard Sandiford @ 2010-12-03 16:47 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Richard Earnshaw, Jie Zhang, Catherine Moore, binutils
"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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2010-12-03 16:47 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-26 10:47 [PATCH 2/4] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
2010-07-31 8:59 ` Richard Sandiford
2010-08-27 12:11 ` Maciej W. Rozycki
2010-08-30 17:53 ` Richard Sandiford
2010-10-29 14:37 ` [PATCH 2.0/4 v2] GAS: Only clone equated symbols on a final reference Maciej W. Rozycki
2010-10-30 9:56 ` Richard Sandiford
2010-12-01 20:35 ` Maciej W. Rozycki
2010-10-29 14:38 ` [PATCH 2.5/4 v2] GAS: Make new fake labels when cloning a symbol Maciej W. Rozycki
2010-10-30 10:01 ` Richard Sandiford
2010-11-01 12:23 ` Maciej W. Rozycki
2010-11-01 21:32 ` Richard Sandiford
2010-12-01 21:34 ` Maciej W. Rozycki
2010-12-02 10:00 ` Jie Zhang
2010-12-02 17:01 ` Richard Earnshaw
2010-12-02 23:19 ` Maciej W. Rozycki
2010-12-03 10:58 ` Richard Sandiford
2010-12-03 16:20 ` Maciej W. Rozycki
2010-12-03 16:47 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).