From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13906 invoked by alias); 27 Aug 2010 09:28:51 -0000 Received: (qmail 13896 invoked by uid 22791); 27 Aug 2010 09:28:50 -0000 X-SWARE-Spam-Status: No, hits=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,TW_EQ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 27 Aug 2010 09:28:45 +0000 Received: (qmail 24974 invoked from network); 27 Aug 2010 09:28:42 -0000 Received: from unknown (HELO tp.orcam.me.uk) (macro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 27 Aug 2010 09:28:42 -0000 Date: Fri, 27 Aug 2010 12:11:00 -0000 From: "Maciej W. Rozycki" To: Richard Sandiford cc: binutils@sourceware.org, Catherine Moore , gnu-mips-sgxx@codesourcery.com Subject: Re: [PATCH 2/4] GAS: Make new fake labels when cloning a symbol In-Reply-To: <87wrscdo34.fsf@firetop.home> Message-ID: References: <87wrscdo34.fsf@firetop.home> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2010-08/txt/msg00342.txt.bz2 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 : > 0: 00000000 nop > 4: 1000ffff b 4 > 8: 00000000 nop > c: 00000000 nop > 10: 1000ffff b 10 > 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 : > 0: 00000000 nop > 4: 1000ffff b 4 > 8: 00000000 nop > > 0000000c : > c: 00000000 nop > 10: 1000ffff b 10 > 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 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 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" /*