[PATCH] gas: fix bogus error on .org involving expression

[PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2972 bytes --]

For years I have been carrying this change, and it was long forgotten
what it was originally meant to deal with, hence I've never submitted
it. Until I came across an issue with Linux kernel like alternative
instruction patching, where the space needed to hold the replacement
instruction was allocated using .org. Things built fine for me (since
I had the patch in place), and things also built fine on 2.20. But
assemblers from at least 2.22 onwards produce an undefined symbol
warning for the "orig" label in the new test case, followed by an
error complaining that .org would be moving backwards (which is a
logical consequence of the undefined symbol getting replaced by plain
zero).

gas/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr.c (expr): Set retval to expr_section for expressions
	involving symbols, which cannot be resolved right away.

gas/testsuite/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr-org.s, expr.org.d: New.
	* gas.exp: Run new test.

--- 2016-03-02/gas/expr.c
+++ 2016-03-02/gas/expr.c
@@ -1998,7 +1998,11 @@ expr (int rankarg,		/* Larger # is highe
 		  retval = absolute_section;
 		  rightseg = absolute_section;
 		}
+	      else
+		retval = expr_section;
 	    }
+	  else
+	    retval = expr_section;
 	}
       else
 	{
@@ -2010,17 +2014,18 @@ expr (int rankarg,		/* Larger # is highe
 	  resultP->X_add_number = 0;
 	  resultP->X_unsigned = 1;
 	  resultP->X_extrabit = 0;
+	  retval = expr_section;
 	}
 
       if (retval != rightseg)
 	{
-	  if (retval == undefined_section)
+	  if (retval == expr_section)
 	    ;
-	  else if (rightseg == undefined_section)
+	  else if (rightseg == expr_section)
 	    retval = rightseg;
-	  else if (retval == expr_section)
+	  else if (retval == undefined_section)
 	    ;
-	  else if (rightseg == expr_section)
+	  else if (rightseg == undefined_section)
 	    retval = rightseg;
 	  else if (retval == reg_section)
 	    ;
--- 2016-03-02/gas/testsuite/gas/all/expr-org.d
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.d
@@ -0,0 +1,11 @@
+#objdump: -s -j .data -j .alt
+#name: .org with expression (with forward reference)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 [0f][0f]ffff[0f][0f] 01 [ .]*
+
+Contents of section \.alt:
+ 0000 00000000 [ .]*
+#pass
--- 2016-03-02/gas/testsuite/gas/all/expr-org.s
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.s
@@ -0,0 +1,10 @@
+	.data
+orig:
+	.byte	0
+	.org	orig + (alt_end - alt_begin), 0xff
+	.pushsection .alt, "a", @progbits
+alt_begin:
+	.long	0
+alt_end:
+	.popsection
+	.byte	1
--- 2016-03-02/gas/testsuite/gas/all/gas.exp
+++ 2016-03-02/gas/testsuite/gas/all/gas.exp
@@ -371,6 +371,7 @@ if {  ([istarget "i*86-*-*pe*"] && ![ist
 if { ![istarget "bfin-*-*"] && ![istarget "nds32*-*-*"] } then {
     run_dump_test assign
 }
+run_dump_test expr-org
 run_dump_test sleb128
 run_dump_test sleb128-2
 run_dump_test sleb128-3




[-- Attachment #2: binutils-master-master-org-expression.patch --]
[-- Type: text/plain, Size: 3019 bytes --]

gas: fix bogus error on .org involving expression

For years I have been carrying this change, and it was long forgotten
what it was originally meant to deal with, hence I've never submitted
it. Until I came across an issue with Linux kernel like alternative
instruction patching, where the space needed to hold the replacement
instruction was allocated using .org. Things built fine for me (since
I had the patch in place), and things also built fine on 2.20. But
assemblers from at least 2.22 onwards produce an undefined symbol
warning for the "orig" label in the new test case, followed by an
error complaining that .org would be moving backwards (which is a
logical consequence of the undefined symbol getting replaced by plain
zero).

gas/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr.c (expr): Set retval to expr_section for expressions
	involving symbols, which cannot be resolved right away.

gas/testsuite/
2016-03-02  Jan Beulich  <jbeulich@suse.com>

	* expr-org.s, expr.org.d: New.
	* gas.exp: Run new test.

--- 2016-03-02/gas/expr.c
+++ 2016-03-02/gas/expr.c
@@ -1998,7 +1998,11 @@ expr (int rankarg,		/* Larger # is highe
 		  retval = absolute_section;
 		  rightseg = absolute_section;
 		}
+	      else
+		retval = expr_section;
 	    }
+	  else
+	    retval = expr_section;
 	}
       else
 	{
@@ -2010,17 +2014,18 @@ expr (int rankarg,		/* Larger # is highe
 	  resultP->X_add_number = 0;
 	  resultP->X_unsigned = 1;
 	  resultP->X_extrabit = 0;
+	  retval = expr_section;
 	}
 
       if (retval != rightseg)
 	{
-	  if (retval == undefined_section)
+	  if (retval == expr_section)
 	    ;
-	  else if (rightseg == undefined_section)
+	  else if (rightseg == expr_section)
 	    retval = rightseg;
-	  else if (retval == expr_section)
+	  else if (retval == undefined_section)
 	    ;
-	  else if (rightseg == expr_section)
+	  else if (rightseg == undefined_section)
 	    retval = rightseg;
 	  else if (retval == reg_section)
 	    ;
--- 2016-03-02/gas/testsuite/gas/all/expr-org.d
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.d
@@ -0,0 +1,11 @@
+#objdump: -s -j .data -j .alt
+#name: .org with expression (with forward reference)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 [0f][0f]ffff[0f][0f] 01 [ .]*
+
+Contents of section \.alt:
+ 0000 00000000 [ .]*
+#pass
--- 2016-03-02/gas/testsuite/gas/all/expr-org.s
+++ 2016-03-02/gas/testsuite/gas/all/expr-org.s
@@ -0,0 +1,10 @@
+	.data
+orig:
+	.byte	0
+	.org	orig + (alt_end - alt_begin), 0xff
+	.pushsection .alt, "a", @progbits
+alt_begin:
+	.long	0
+alt_end:
+	.popsection
+	.byte	1
--- 2016-03-02/gas/testsuite/gas/all/gas.exp
+++ 2016-03-02/gas/testsuite/gas/all/gas.exp
@@ -371,6 +371,7 @@ if {  ([istarget "i*86-*-*pe*"] && ![ist
 if { ![istarget "bfin-*-*"] && ![istarget "nds32*-*-*"] } then {
     run_dump_test assign
 }
+run_dump_test expr-org
 run_dump_test sleb128
 run_dump_test sleb128-2
 run_dump_test sleb128-3

Ping: [PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

Ping???

>>> On 02.03.16 at 12:29, <JBeulich@suse.com> wrote:
> For years I have been carrying this change, and it was long forgotten
> what it was originally meant to deal with, hence I've never submitted
> it. Until I came across an issue with Linux kernel like alternative
> instruction patching, where the space needed to hold the replacement
> instruction was allocated using .org. Things built fine for me (since
> I had the patch in place), and things also built fine on 2.20. But
> assemblers from at least 2.22 onwards produce an undefined symbol
> warning for the "orig" label in the new test case, followed by an
> error complaining that .org would be moving backwards (which is a
> logical consequence of the undefined symbol getting replaced by plain
> zero).
> 
> gas/
> 2016-03-02  Jan Beulich  <jbeulich@suse.com>
> 
> 	* expr.c (expr): Set retval to expr_section for expressions
> 	involving symbols, which cannot be resolved right away.
> 
> gas/testsuite/
> 2016-03-02  Jan Beulich  <jbeulich@suse.com>
> 
> 	* expr-org.s, expr.org.d: New.
> 	* gas.exp: Run new test.
> 
> --- 2016-03-02/gas/expr.c
> +++ 2016-03-02/gas/expr.c
> @@ -1998,7 +1998,11 @@ expr (int rankarg,		/* Larger # is highe
>  		  retval = absolute_section;
>  		  rightseg = absolute_section;
>  		}
> +	      else
> +		retval = expr_section;
>  	    }
> +	  else
> +	    retval = expr_section;
>  	}
>        else
>  	{
> @@ -2010,17 +2014,18 @@ expr (int rankarg,		/* Larger # is highe
>  	  resultP->X_add_number = 0;
>  	  resultP->X_unsigned = 1;
>  	  resultP->X_extrabit = 0;
> +	  retval = expr_section;
>  	}
>  
>        if (retval != rightseg)
>  	{
> -	  if (retval == undefined_section)
> +	  if (retval == expr_section)
>  	    ;
> -	  else if (rightseg == undefined_section)
> +	  else if (rightseg == expr_section)
>  	    retval = rightseg;
> -	  else if (retval == expr_section)
> +	  else if (retval == undefined_section)
>  	    ;
> -	  else if (rightseg == expr_section)
> +	  else if (rightseg == undefined_section)
>  	    retval = rightseg;
>  	  else if (retval == reg_section)
>  	    ;
> --- 2016-03-02/gas/testsuite/gas/all/expr-org.d
> +++ 2016-03-02/gas/testsuite/gas/all/expr-org.d
> @@ -0,0 +1,11 @@
> +#objdump: -s -j .data -j .alt
> +#name: .org with expression (with forward reference)
> +
> +.*: .*
> +
> +Contents of section \.data:
> + 0000 [0f][0f]ffff[0f][0f] 01 [ .]*
> +
> +Contents of section \.alt:
> + 0000 00000000 [ .]*
> +#pass
> --- 2016-03-02/gas/testsuite/gas/all/expr-org.s
> +++ 2016-03-02/gas/testsuite/gas/all/expr-org.s
> @@ -0,0 +1,10 @@
> +	.data
> +orig:
> +	.byte	0
> +	.org	orig + (alt_end - alt_begin), 0xff
> +	.pushsection .alt, "a", @progbits
> +alt_begin:
> +	.long	0
> +alt_end:
> +	.popsection
> +	.byte	1
> --- 2016-03-02/gas/testsuite/gas/all/gas.exp
> +++ 2016-03-02/gas/testsuite/gas/all/gas.exp
> @@ -371,6 +371,7 @@ if {  ([istarget "i*86-*-*pe*"] && ![ist
>  if { ![istarget "bfin-*-*"] && ![istarget "nds32*-*-*"] } then {
>      run_dump_test assign
>  }
> +run_dump_test expr-org
>  run_dump_test sleb128
>  run_dump_test sleb128-2
>  run_dump_test sleb128-3

Re: Ping: [PATCH] gas: fix bogus error on .org involving expression

[Alan Modra]

On Wed, Mar 16, 2016 at 10:40:15AM -0600, Jan Beulich wrote:
> Ping???
> >>> On 02.03.16 at 12:29, <JBeulich@suse.com> wrote:
> > gas/
> > 2016-03-02  Jan Beulich  <jbeulich@suse.com>
> > 
> > 	* expr.c (expr): Set retval to expr_section for expressions
> > 	involving symbols, which cannot be resolved right away.

OK.

> > gas/testsuite/
> > 2016-03-02  Jan Beulich  <jbeulich@suse.com>
> > 
> > 	* expr-org.s, expr.org.d: New.
> > 	* gas.exp: Run new test.

Have you run the new test over a good sample of binutils targets?
I suspect that the new test won't work on some, tic4x and tic54x, for
example.  OK with changes to exclude such targets.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Ping: [PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

>>> On 17.03.16 at 00:46, <amodra@gmail.com> wrote:
> On Wed, Mar 16, 2016 at 10:40:15AM -0600, Jan Beulich wrote:
>> Ping???
>> >>> On 02.03.16 at 12:29, <JBeulich@suse.com> wrote:
>> > gas/
>> > 2016-03-02  Jan Beulich  <jbeulich@suse.com>
>> > 
>> > 	* expr.c (expr): Set retval to expr_section for expressions
>> > 	involving symbols, which cannot be resolved right away.
> 
> OK.
> 
>> > gas/testsuite/
>> > 2016-03-02  Jan Beulich  <jbeulich@suse.com>
>> > 
>> > 	* expr-org.s, expr.org.d: New.
>> > 	* gas.exp: Run new test.
> 
> Have you run the new test over a good sample of binutils targets?

Honestly - no, I didn't, as I didn't expect .org to behave
differently for different targets (to me that would be pretty
surprising).

> I suspect that the new test won't work on some, tic4x and tic54x, for
> example.  OK with changes to exclude such targets.

Will do.

Jan

Re: Ping: [PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2746 bytes --]

>>> On 17.03.16 at 00:46, <amodra@gmail.com> wrote:
> On Wed, Mar 16, 2016 at 10:40:15AM -0600, Jan Beulich wrote:
>> Ping???
>> >>> On 02.03.16 at 12:29, <JBeulich@suse.com> wrote:
>> > gas/
>> > 2016-03-02  Jan Beulich  <jbeulich@suse.com>
>> > 
>> > 	* expr.c (expr): Set retval to expr_section for expressions
>> > 	involving symbols, which cannot be resolved right away.
> 
> OK.
> 
>> > gas/testsuite/
>> > 2016-03-02  Jan Beulich  <jbeulich@suse.com>
>> > 
>> > 	* expr-org.s, expr.org.d: New.
>> > 	* gas.exp: Run new test.
> 
> Have you run the new test over a good sample of binutils targets?
> I suspect that the new test won't work on some, tic4x and tic54x, for
> example.  OK with changes to exclude such targets.

Indeed there were quite a few. Namely, due to the use
of .pushsection and .popsection all non-ELF targets need to be
excluded. mep-*-elf was the only other one needing exclusion,
since the complex relocation support it enables appears to result
in expressions not getting evaluated in time for .org to get its
operand fully resolved. To be honest, I'm not really bothered
enough to dig into finding (and fixing) the exact reason for this.

Beyond those, however, this also revealed a few other failures:

1) i586-aout had several regressions due to the unimplemented
segment checks in i386_finalize_immediate() and
i386_finalize_displacement() not accounting for expr_section.
This will therefore require an update to the patch itself, not just
the testsuite (which I think is warranted even if the test now is
being limited to ELF targets).

2) Along the same lines HPPA's "Bad segment in expression."
warning generation logic didn't account for expr_section.

3) xtensa-elf setting TC_FINALIZE_SYMS_BEFORE_SIZE_SEG to 0
causes the "invalid operands (.text.unlikely and .text.startup
sections) for `-'" to not appear for elf/bad-group.s. Prior to the
change this was detected as "operation combines symbols in
different segments" from expr(), but the code change results in
this error getting eliminated. It's not clear to me how to make
sure the "finalize_syms" dependent "invalid operands" error from
resolve_symbol_value() gets properly raised for such targets
(nds32 appears to be the only other one, which my set of tested
targets didn't include).

4) MMIX'es pr12815-* tests both fail (they no longer produce the
expected or any other error). Since I have no idea what exactly
those tests test, I also have no idea how to deal with this.

I'm attaching the patch taking care of everything except 3) and
4); please advise what to do about those two remaining issues
(I'm also Cc-ing the respective target code maintainers for that
purpose).

Jan


[-- Attachment #2: binutils-master-master-org-expression.patch --]
[-- Type: text/plain, Size: 4199 bytes --]

gas: fix bogus error on .org involving expression

For years I have been carrying this change, and it was long forgotten
what it was originally meant to deal with, hence I've never submitted
it. Until I came across an issue with Linux kernel like alternative
instruction patching, where the space needed to hold the replacement
instruction was allocated using .org. Things built fine for me (since
I had the patch in place), and things also built fine on 2.20. But
assemblers from at least 2.22 onwards produce an undefined symbol
warning for the "orig" label in the new test case, followed by an
error complaining that .org would be moving backwards (which is a
logical consequence of the undefined symbol getting replaced by plain
zero).

gas/
2016-03-22  Jan Beulich  <jbeulich@suse.com>

	* config/tc-hppa.c (get_expression): Also allow expr_section.
	* config/tc-i386.c (i386_finalize_immediate): Likewise.
	(i386_finalize_displacement): Likewise.
	* expr.c (expr): Set retval to expr_section for expressions
	involving symbols, which cannot be resolved right away.

gas/testsuite/
2016-03-22  Jan Beulich  <jbeulich@suse.com>

	* expr-org.s, expr.org.d: New.
	* gas.exp: Run new test.

--- 2016-03-22/gas/config/tc-hppa.c
+++ 2016-03-22/gas/config/tc-hppa.c
@@ -1282,6 +1282,7 @@ get_expression (char *str)
   input_line_pointer = str;
   seg = expression (&the_insn.exp);
   if (!(seg == absolute_section
+	|| seg == expr_section
 	|| seg == undefined_section
 	|| SEG_NORMAL (seg)))
     {
--- 2016-03-22/gas/config/tc-i386.c
+++ 2016-03-22/gas/config/tc-i386.c
@@ -8043,6 +8043,7 @@ i386_finalize_immediate (segT exp_seg AT
 	   && exp_seg != text_section
 	   && exp_seg != data_section
 	   && exp_seg != bss_section
+	   && exp_seg != expr_section
 	   && exp_seg != undefined_section
 	   && !bfd_is_com_section (exp_seg))
     {
@@ -8336,6 +8337,7 @@ i386_finalize_displacement (segT exp_seg
 	   && exp_seg != text_section
 	   && exp_seg != data_section
 	   && exp_seg != bss_section
+	   && exp_seg != expr_section
 	   && exp_seg != undefined_section
 	   && !bfd_is_com_section (exp_seg))
     {
--- 2016-03-22/gas/expr.c
+++ 2016-03-22/gas/expr.c
@@ -1998,7 +1998,11 @@ expr (int rankarg,		/* Larger # is highe
 		  retval = absolute_section;
 		  rightseg = absolute_section;
 		}
+	      else
+		retval = expr_section;
 	    }
+	  else
+	    retval = expr_section;
 	}
       else
 	{
@@ -2010,17 +2014,18 @@ expr (int rankarg,		/* Larger # is highe
 	  resultP->X_add_number = 0;
 	  resultP->X_unsigned = 1;
 	  resultP->X_extrabit = 0;
+	  retval = expr_section;
 	}
 
       if (retval != rightseg)
 	{
-	  if (retval == undefined_section)
+	  if (retval == expr_section)
 	    ;
-	  else if (rightseg == undefined_section)
+	  else if (rightseg == expr_section)
 	    retval = rightseg;
-	  else if (retval == expr_section)
+	  else if (retval == undefined_section)
 	    ;
-	  else if (rightseg == expr_section)
+	  else if (rightseg == undefined_section)
 	    retval = rightseg;
 	  else if (retval == reg_section)
 	    ;
--- 2016-03-22/gas/testsuite/gas/all/expr-org.d
+++ 2016-03-22/gas/testsuite/gas/all/expr-org.d
@@ -0,0 +1,11 @@
+#objdump: -s -j .data -j .alt
+#name: .org with expression (with forward reference)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 [0f][0f]ffff[0f][0f] 01[0 .]*
+
+Contents of section \.alt:
+ 0000 00000000 [ .]*
+#pass
--- 2016-03-22/gas/testsuite/gas/all/expr-org.s
+++ 2016-03-22/gas/testsuite/gas/all/expr-org.s
@@ -0,0 +1,10 @@
+	.data
+orig:
+	.byte	0
+	.org	orig + (alt_end - alt_begin), 0xff
+	.pushsection .alt, "a", %progbits
+alt_begin:
+	.long	0
+alt_end:
+	.popsection
+	.byte	1
--- 2016-03-22/gas/testsuite/gas/all/gas.exp
+++ 2016-03-22/gas/testsuite/gas/all/gas.exp
@@ -432,6 +432,11 @@ gas_test_error "weakref4.s" "" "is alrea
 run_dump_test string
 if [is_elf_format] {
     run_dump_test none
+
+# Targets enabling OBJ_COMPLEX_RELC don't seem to be compatible with this.
+    if { ![istarget "mep-*-*"] } {
+	run_dump_test expr-org
+    }
 }
 
 run_dump_test quoted-sym-names

Re: Ping: [PATCH] gas: fix bogus error on .org involving expression

[Hans-Peter Nilsson]

On Tue, 22 Mar 2016, Jan Beulich wrote:
> Beyond those, however, this also revealed a few other failures:

> 4) MMIX'es pr12815-* tests both fail (they no longer produce the
> expected or any other error). Since I have no idea what exactly
> those tests test, I also have no idea how to deal with this.

You refer to *linker tests* checking for "a meaningful error
message rather than SEGV" from the *linker*.  I suspect your
assembler patches affects things it shouldn't, i.e. the it
affects assembler output, not only fixing error cases.

brgds, H-P

Re: Ping: [PATCH] gas: fix bogus error on .org involving expression

[Jan Beulich]

>>> On 22.03.16 at 13:26, <hp@bitrange.com> wrote:
> On Tue, 22 Mar 2016, Jan Beulich wrote:
>> Beyond those, however, this also revealed a few other failures:
> 
>> 4) MMIX'es pr12815-* tests both fail (they no longer produce the
>> expected or any other error). Since I have no idea what exactly
>> those tests test, I also have no idea how to deal with this.
> 
> You refer to *linker tests* checking for "a meaningful error
> message rather than SEGV" from the *linker*.  I suspect your
> assembler patches affects things it shouldn't, i.e. the it
> affects assembler output, not only fixing error cases.

While that's certainly true, it's entirely unclear to me why that would
be. Looking a little more closely, both generated object files have
empty .text and .data sections (yet only the .data section of the
second one was empty before). Assuming that what I see in the
sources are opcodes, I have absolutely no idea why that would
result in no code getting generated at all, but there also not being
any error. This, to me at least, quite clearly hints at a bug elsewhere
which this change uncovers. I could guess (and perhaps work out
by trial and error) whether one of the five uses of undefined_section
in gas/config/tc-mmix.c needs replacing or extending by considering
expr_section, but that's not the kind of work I'd like to do on code I
have no knowledge about at all.

Of course it's generally questionable whether the significantly
lower amount of expr_section uses in gas/config/tc-*.c compared
to undefined_section doesn't indicate further lurking issues. So
unless I can get some help here, I'm almost willing to give up on
this and leave .org broken (I've already managed to find an
acceptable replacement for the use case with which I ran into the
problem originally).

Jan

Re: Ping: [PATCH] gas: fix bogus error on .org involving expression

[augustine.sterling]

On Tue, Mar 22, 2016 at 2:19 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 3) xtensa-elf setting TC_FINALIZE_SYMS_BEFORE_SIZE_SEG to 0
> causes the "invalid operands (.text.unlikely and .text.startup
> sections) for `-'" to not appear for elf/bad-group.s. Prior to the
> change this was detected as "operation combines symbols in
> different segments" from expr(), but the code change results in
> this error getting eliminated. It's not clear to me how to make
> sure the "finalize_syms" dependent "invalid operands" error from
> resolve_symbol_value() gets properly raised for such targets
> (nds32 appears to be the only other one, which my set of tested
> targets didn't include).

In the Xtensa port, the reason for setting
TC_FINALIZE_SYMS_BEFORE_SIZE_SEG to zero is lost to the mists of time.
Removing that setting creates no new failures in our testing. After
that, applying the patch in question doesn't create the new failure
you list above.

I'll commit a patch to stop setting it, and the Xtensa port has no
objection to this patch, and you shouldn't block on it.

Sterling
(thanks to David Weatherford for additional testing.)