gas: fix overflow diagnostics

Message ID 44395237-8000-141c-aeff-69a2d2e7b524@suse.com
State New
Headers show
Series
  • gas: fix overflow diagnostics
Related show

Commit Message

Luis Machado via Binutils June 15, 2021, 6:10 a.m.
While the logic in fixup_segment() so far was off by 1 for fixups
dealing with quantities without known signedness, thus failing to report
an overflow when e.g. a byte-sized resulting value is -0x100, the logic
in emit_expr_with_reloc() reported an overflow even on large negative
values when the respective positive ones would not be warned
about, and when fixup_segment() wouldn't do so either. Such diagnostics
all ought to follow a similar pattern of what value range is acceptable.
(If expressions' X_unsigned was reliably set, emit_expr_with_reloc()'s
checking might make sense to tighten based on that flag.)

Note that with commit 80aab57939a0 ("Changes to let cons handle bignums
like general expressions") having added handling of nbytes >
sizeof(valueT) in the O_constant case, converting to O_big, the setting
to zero of "hibit" had become dead. With "hibit" no longer used, this
code now gets dropped altogether. But additionally a respective know()
gets inserted.

gas/
2021-06-XX  Jan Beulich  <jbeulich@suse.com>

	* read.c (emit_expr_with_reloc): Adjust overflow check. Drop
	hibit local variable.
	* write.c (fixup_segment): Differentiate signed and non-signed
	relocs in overflow check.
	* testsuite/gas/all/overflow.s,
	testsuite/gas/all/overflow.l: New.
	* testsuite/gas/all/gas.exp: Run new test.
---
TBD: The values being of type valueT may trigger compiler warnings about
     the negation of unsigned values, but them getting read from an
     expression's X_add_number suggests anyway they may better be
     offsetT. Thoughts, anyone?

Considering x86's further instance of the underlying pattern (see "x86:
off-by-1 in offset_in_range()" I wonder how many more instances of this
exist elsewhere in the code base, without being straightforward to spot.

Comments

Luis Machado via Binutils June 15, 2021, 7:03 a.m. | #1
On Tue, Jun 15, 2021 at 08:10:50AM +0200, Jan Beulich via Binutils wrote:
> 	* read.c (emit_expr_with_reloc): Adjust overflow check. Drop

> 	hibit local variable.

> 	* write.c (fixup_segment): Differentiate signed and non-signed

> 	relocs in overflow check.

> 	* testsuite/gas/all/overflow.s,

> 	testsuite/gas/all/overflow.l: New.

> 	* testsuite/gas/all/gas.exp: Run new test.


OK, but

>        /* JF << of >= number of bits in the object is undefined.  In

>  	 particular SPARC (Sun 4) has problems.  */

>        if (nbytes >= sizeof (valueT))

>  	{

> +	  know (nbytes == sizeof (valueT));


I question the need for this assertion.  It seems to me that nothing
goes wrong here if nbytes is larger, so what assumption is it
protecting?

-- 
Alan Modra
Australia Development Lab, IBM
Luis Machado via Binutils June 15, 2021, 7:33 a.m. | #2
On 15.06.2021 09:03, Alan Modra wrote:
> On Tue, Jun 15, 2021 at 08:10:50AM +0200, Jan Beulich via Binutils wrote:

>> 	* read.c (emit_expr_with_reloc): Adjust overflow check. Drop

>> 	hibit local variable.

>> 	* write.c (fixup_segment): Differentiate signed and non-signed

>> 	relocs in overflow check.

>> 	* testsuite/gas/all/overflow.s,

>> 	testsuite/gas/all/overflow.l: New.

>> 	* testsuite/gas/all/gas.exp: Run new test.

> 

> OK, but

> 

>>        /* JF << of >= number of bits in the object is undefined.  In

>>  	 particular SPARC (Sun 4) has problems.  */

>>        if (nbytes >= sizeof (valueT))

>>  	{

>> +	  know (nbytes == sizeof (valueT));

> 

> I question the need for this assertion.  It seems to me that nothing

> goes wrong here if nbytes is larger, so what assumption is it

> protecting?


Well, I put it there to document the state at this point. If the hibit
calculation was still present (or something similar be added down the
road), it makes clear that the nbytes > sizeof() doesn't need handling
here.

I can drop it if you'd prefer it that way. I did add it because I was
expecting the question to come the other way around if it hadn't been
put there.

Jan
Luis Machado via Binutils June 15, 2021, 9:45 a.m. | #3
On Tue, Jun 15, 2021 at 09:33:18AM +0200, Jan Beulich wrote:
> On 15.06.2021 09:03, Alan Modra wrote:

> > I question the need for this assertion.  It seems to me that nothing

> > goes wrong here if nbytes is larger, so what assumption is it

> > protecting?

> 

> Well, I put it there to document the state at this point. If the hibit

> calculation was still present (or something similar be added down the

> road), it makes clear that the nbytes > sizeof() doesn't need handling

> here.

> 

> I can drop it if you'd prefer it that way. I did add it because I was

> expecting the question to come the other way around if it hadn't been

> put there.


Heh, it doesn't really matter.  Either way is OK.

-- 
Alan Modra
Australia Development Lab, IBM

Patch

--- a/gas/read.c
+++ b/gas/read.c
@@ -4505,24 +4505,19 @@  emit_expr_with_reloc (expressionS *exp,
       valueT get;
       valueT use;
       valueT mask;
-      valueT hibit;
       valueT unmask;
 
       /* JF << of >= number of bits in the object is undefined.  In
 	 particular SPARC (Sun 4) has problems.  */
       if (nbytes >= sizeof (valueT))
 	{
+	  know (nbytes == sizeof (valueT));
 	  mask = 0;
-	  if (nbytes > sizeof (valueT))
-	    hibit = 0;
-	  else
-	    hibit = (valueT) 1 << (nbytes * BITS_PER_CHAR - 1);
 	}
       else
 	{
 	  /* Don't store these bits.  */
 	  mask = ~(valueT) 0 << (BITS_PER_CHAR * nbytes);
-	  hibit = (valueT) 1 << (nbytes * BITS_PER_CHAR - 1);
 	}
 
       unmask = ~mask;		/* Do store these bits.  */
@@ -4534,9 +4529,7 @@  emit_expr_with_reloc (expressionS *exp,
 
       get = exp->X_add_number;
       use = get & unmask;
-      if ((get & mask) != 0
-	  && ((get & mask) != mask
-	      || (get & hibit) == 0))
+      if ((get & mask) != 0 && (-get & mask) != 0)
 	{
 	  /* Leading bits contain both 0s & 1s.  */
 	  as_warn (_("value 0x%" BFD_VMA_FMT "x truncated to 0x%" BFD_VMA_FMT "x"),
--- a/gas/testsuite/gas/all/gas.exp
+++ b/gas/testsuite/gas/all/gas.exp
@@ -372,6 +372,27 @@  switch -glob $target_triplet {
     }
 }
 
+# Various targets use too custom handling to be able to sensibly create
+# common expecations for this test.  Also .equ works differently on some
+# targets.
+if {    ![istarget avr-*-*]
+     && ![istarget bfin-*-*]
+     && ![istarget cris*-*-*]
+     && ![istarget dlx-*-*]
+     && ![istarget hppa*-*-*]
+     && ![istarget m68k-*-*]
+     && ![istarget nios2-*-*]
+     && ![istarget pj-*-*]
+     && ![istarget sh*-*-*]
+     && ![istarget *c4x-*-*]
+     && ![istarget *c54x-*-*]
+     && ![istarget *c6x-*-*]
+     && ![istarget z80-*-*] } then {
+    # Some further targets' custom handling fails to recognize the overflows.
+    setup_xfail "crx-*-*" "h8300-*-*" "mcore-*-*" "mn10200-*-*" "mn10300-*-*" "msp430-*-*" "ns32k-*-*"
+    run_list_test "overflow"
+}
+
 if {  ([istarget "i*86-*-*pe*"] && ![istarget "i*86-*-openbsd*"]) \
     || [istarget "i*86-*-cygwin*"] \
     || [istarget "i*86-*-mingw32*"] } {
--- /dev/null
+++ b/gas/testsuite/gas/all/overflow.l
@@ -0,0 +1,9 @@ 
+.*: Assembler messages:
+.*:5: Warning: .* (0x)?100 truncated to (0x)?0
+.*:6: Warning: .* (0x)?101 truncated to (0x)?1
+.*:11: Warning: .* (0x)?f+00 truncated to (0x)?0
+.*:12: Warning: .* (0x)?f+eff truncated to (0x)?ff
+.*:17: Error: .* (256|(0x)?100) too large .*
+.*:18: Error: .* (257|(0x)?101) too large .*
+.*:23: Error: .* (0x)?f+00 too large .*
+.*:24: Error: .* (0x)?f+eff too large .*
--- /dev/null
+++ b/gas/testsuite/gas/all/overflow.s
@@ -0,0 +1,26 @@ 
+	.data
+	.dc.b +0x80
+	.dc.b +0x81
+	.dc.b +0xff
+	.dc.b +0x100
+	.dc.b +0x101
+
+	.dc.b -0x80
+	.dc.b -0x81
+	.dc.b -0xff
+	.dc.b -0x100
+	.dc.b -0x101
+
+	.dc.b zero+0x80
+	.dc.b zero+0x81
+	.dc.b zero+0xff
+	.dc.b zero+0x100
+	.dc.b zero+0x101
+
+	.dc.b zero-0x80
+	.dc.b zero-0x81
+	.dc.b zero-0xff
+	.dc.b zero-0x100
+	.dc.b zero-0x101
+
+	.equ zero, 0
--- a/gas/write.c
+++ b/gas/write.c
@@ -1107,7 +1107,10 @@  fixup_segment (fixS *fixP, segT this_seg
 	      mask = 0;
 	      mask--;		/* Set all bits to one.  */
 	      mask <<= fixP->fx_size * 8 - (fixP->fx_signed ? 1 : 0);
-	      if ((add_number & mask) != 0 && (add_number & mask) != mask)
+	      if ((add_number & mask) != 0
+		  && (fixP->fx_signed
+		      ? (add_number & mask) != mask
+		      : (-add_number & mask) != 0))
 		{
 		  char buf[50], buf2[50];
 		  bfd_sprintf_vma (stdoutput, buf, fragP->fr_address + fixP->fx_where);