From: "Maciej W. Rozycki" <macro@codesourcery.com>
To: Richard Sandiford <rdsandiford@googlemail.com>
Cc: binutils@sourceware.org, Catherine Moore <clm@codesourcery.com>,
gnu-mips-sgxx@codesourcery.com
Subject: Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol
Date: Fri, 27 Aug 2010 12:11:00 -0000 [thread overview]
Message-ID: <alpine.DEB.1.10.1008270057100.4576@tp.orcam.me.uk> (raw)
In-Reply-To: <87wrscdo34.fsf@firetop.home>
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"
/*
next prev parent reply other threads:[~2010-08-27 9:28 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-07-26 10:47 Maciej W. Rozycki
2010-07-31 8:59 ` Richard Sandiford
2010-08-27 12:11 ` Maciej W. Rozycki [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.1.10.1008270057100.4576@tp.orcam.me.uk \
--to=macro@codesourcery.com \
--cc=binutils@sourceware.org \
--cc=clm@codesourcery.com \
--cc=gnu-mips-sgxx@codesourcery.com \
--cc=rdsandiford@googlemail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).