Your GAS change caused a testsuite regression for cris-elf

Your GAS change caused a testsuite regression for cris-elf

[Hans-Peter Nilsson]

(Sorry for this duplicate, Alan, I see I missed CC:ing binutils@.)

Well, actually the test is a.out+stabs-specific (the ELF variant
in undef3.d
doesn't fail):
...
Running /h/hp/binutils/cvs_latest/src/ld/testsuite/ld-cris/cris.exp ...
FAIL: ld-cris/undef2
...

ld.log:
./ld-new  -L/h/hp/binutils/cvs_latest/src/ld/testsuite/ld-cris  -mcrisaout -o tmpdir/dump tmpdir/dump0.o tmpdir/dump1.o
failed with: <tmpdir/dump1.o:tmpdir/dump1.o:96: undefined reference to `globsym1'>, expected: <.o:/blah/foo.c:96: undefined reference to `globsym1'$>
tmpdir/dump1.o:tmpdir/dump1.o:96: undefined reference to `globsym1'
failed with: <tmpdir/dump1.o:tmpdir/dump1.o:96: undefined reference to `globsym1'>, expected: <.o:/blah/foo.c:96: undefined reference to `globsym1'$>
FAIL: ld-cris/undef2

Initially, it looked like a typo somewhere.  Looking at the
changes in the time-frame back to the previous change, I had
trouble believing that this change (src/gas):

2005-09-20  Alan Modra  <amodra@bigpond.net.au>

	* read.c (pseudo_set): Set segment of expression syms to expr_section.

actually caused the regression, but it does, or at least causes
it to be exposed.  I checked by testing with and without (cvs
diff -D yesterday -l | patch -R) that change.  I guess something
strange and wonderful is going on.  Luckily for me, I can just
point at the regression and say "waaah!".

brgds, H-P

Re: Your GAS change caused a testsuite regression for cris-elf

[Alan Modra]

On Tue, Sep 20, 2005 at 02:26:07PM +0200, Hans-Peter Nilsson wrote:
> 2005-09-20  Alan Modra  <amodra@bigpond.net.au>
> 
> 	* read.c (pseudo_set): Set segment of expression syms to expr_section.
> 
> actually caused the regression, but it does, or at least causes

I think the following obvious fix should cure the problem, but I'll wait
until the morning to commit (and this time after I've tested lots of
targets).

	* read.c (pseudo_set): Don't set undefined symbols to expr_section.

Index: gas/read.c
===================================================================
RCS file: /cvs/src/src/gas/read.c,v
retrieving revision 1.107
diff -u -p -r1.107 read.c
--- gas/read.c	20 Sep 2005 03:06:13 -0000	1.107
+++ gas/read.c	20 Sep 2005 15:02:59 -0000
@@ -3259,7 +3259,10 @@ pseudo_set (symbolS *symbolP)
 	  copy_symbol_attributes (symbolP, s);
 	  break;
 	}
-      /* Fall thru */
+      S_SET_SEGMENT (symbolP, undefined_section);
+      symbol_set_value_expression (symbolP, &exp);
+      set_zero_frag (symbolP);
+      break;
 
     default:
       /* The value is some complex expression.  */

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: Your GAS change caused a testsuite regression for cris-elf

[Alan Modra]

On Wed, Sep 21, 2005 at 12:35:23AM +0930, Alan Modra wrote:
> On Tue, Sep 20, 2005 at 02:26:07PM +0200, Hans-Peter Nilsson wrote:
> > 2005-09-20  Alan Modra  <amodra@bigpond.net.au>
> > 
> > 	* read.c (pseudo_set): Set segment of expression syms to expr_section.
> > 
> > actually caused the regression, but it does, or at least causes
> 
> I think the following obvious fix should cure the problem, but I'll wait
> until the morning to commit (and this time after I've tested lots of
> targets).
> 
> 	* read.c (pseudo_set): Don't set undefined symbols to expr_section.

It tested out OK.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: Your GAS change caused a testsuite regression for cris-elf

[Jan Beulich]

>>> Alan Modra <amodra@bigpond.net.au> 20.09.05 17:05:23 >>>
>On Tue, Sep 20, 2005 at 02:26:07PM +0200, Hans-Peter Nilsson wrote:
>> 2005-09-20  Alan Modra  <amodra@bigpond.net.au>
>> 
>> 	* read.c (pseudo_set): Set segment of expression syms to
expr_section.
>> 
>> actually caused the regression, but it does, or at least causes
>
>I think the following obvious fix should cure the problem, but I'll
wait
>until the morning to commit (and this time after I've tested lots of
>targets).
>
>	* read.c (pseudo_set): Don't set undefined symbols to
expr_section.
>
>Index: gas/read.c
>===================================================================
>RCS file: /cvs/src/src/gas/read.c,v
>retrieving revision 1.107
>diff -u -p -r1.107 read.c
>--- gas/read.c	20 Sep 2005 03:06:13 -0000	1.107
>+++ gas/read.c	20 Sep 2005 15:02:59 -0000
>@@ -3259,7 +3259,10 @@ pseudo_set (symbolS *symbolP)
> 	  copy_symbol_attributes (symbolP, s);
> 	  break;
> 	}
>-      /* Fall thru */
>+      S_SET_SEGMENT (symbolP, undefined_section);
>+      symbol_set_value_expression (symbolP, &exp);
>+      set_zero_frag (symbolP);
>+      break;
> 
>     default:
>       /* The value is some complex expression.  */
>

I don't think this is correct, as it leads to failure recognizing the
redefinition of x in

	.equiv	x, y
	.equiv	x, 1

or, even less logical (because x by the time it gets redefined is fully
resolvable),

	.equiv	x, y
	.equiv	y, 1
	.equiv	x, 1

The problem observed by Hans-Peter probably needs to be fixed in a.out
and/or stabs related code.

Jan

Re: Your GAS change caused a testsuite regression for cris-elf

[Alan Modra]

On Mon, Sep 26, 2005 at 02:54:39PM +0200, Jan Beulich wrote:
> >>> Alan Modra <amodra@bigpond.net.au> 20.09.05 17:05:23 >>>
> >	* read.c (pseudo_set): Don't set undefined symbols to
> expr_section.
> 
> I don't think this is correct, as it leads to failure recognizing the
> redefinition of x in
> 
> 	.equiv	x, y
> 	.equiv	x, 1
> 
> or, even less logical (because x by the time it gets redefined is fully
> resolvable),
> 
> 	.equiv	x, y
> 	.equiv	y, 1
> 	.equiv	x, 1
> 
> The problem observed by Hans-Peter probably needs to be fixed in a.out
> and/or stabs related code.

Given an undefined "y", ".equiv x,y" results in an "x" that is also in
some sense undefined.  I don't want to break places in gas that use a
simple test of the section to see whether a symbol is defined or not.
Fixing a longstanding bug with .equiv by changing the symbol section
will probably break more than just the aout stabs code.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: Your GAS change caused a testsuite regression for cris-elf

[Jan Beulich]

>>> Alan Modra <amodra@bigpond.net.au> 27.09.05 03:19:32 >>>
>On Mon, Sep 26, 2005 at 02:54:39PM +0200, Jan Beulich wrote:
>> >>> Alan Modra <amodra@bigpond.net.au> 20.09.05 17:05:23 >>>
>> >	* read.c (pseudo_set): Don't set undefined symbols to
>> expr_section.
>> 
>> I don't think this is correct, as it leads to failure recognizing
the
>> redefinition of x in
>> 
>> 	.equiv	x, y
>> 	.equiv	x, 1
>> 
>> or, even less logical (because x by the time it gets redefined is
fully
>> resolvable),
>> 
>> 	.equiv	x, y
>> 	.equiv	y, 1
>> 	.equiv	x, 1
>> 
>> The problem observed by Hans-Peter probably needs to be fixed in
a.out
>> and/or stabs related code.
>
>Given an undefined "y", ".equiv x,y" results in an "x" that is also
in
>some sense undefined.  I don't want to break places in gas that use a
>simple test of the section to see whether a symbol is defined or not.
>Fixing a longstanding bug with .equiv by changing the symbol section
>will probably break more than just the aout stabs code.

I'm sorry, but this makes no sense to me. A bug is a bug and should be
fixed. The more that in the second example given y (and thus x) isn't
even 'in some sense undefined'; it's just the case that x never
'learned' that y became defined. Plus it makes it impossible to write
code that supplies a fallback value in case of a symbol being
undefined:

 	.equiv	x, y
 	.equiv	y, 1
	.ifndef	x
 	.equiv	x, 0
	.endif

And finally, in what way is

	.equiv	x, y-z
	.equiv	x, 0

different from the first example above? I.e., why is x not 'in some
sense undefined' here?

Jan

Re: Your GAS change caused a testsuite regression for cris-elf

[Alan Modra]

On Tue, Sep 27, 2005 at 08:52:33AM +0200, Jan Beulich wrote:
> I'm sorry, but this makes no sense to me.

It's simple enough.  I'm not willing to deliberately break the
assembler, as demonstrated by the aout stabs testcase, just to fix
".equiv".  How many other places in the assembler have code that
does something similar to the aout stabs code?

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre