[1/6] x86: off-by-1 in offset_in_range()

Message ID 7f92142e-e391-e1fe-c741-0d290f71da27@suse.com
State New
Headers show
Series
  • x86: further value overflow diagnostic adjustments
Related show

Commit Message

Luis Machado via Binutils June 14, 2021, 10:24 a.m.
Just like e.g. 0x10000 triggers a warning for size 2, -0x10000 ought to
as well.

Note that some of the encodings produced aren't ones one would expect,
and hence the generated code is not being checked for in the new
testcases.

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

	* config/tc-i386.c (offset_in_range): Adjust conditional.
	* testsuite/gas/i386/disp-imm-16.s,
	testsuite/gas/i386/disp-imm-16.l,
	testsuite/gas/i386/disp-imm-64.s,
	testsuite/gas/i386/disp-imm-64.l: New.
	* testsuite/gas/i386/i386.exp: Run new tests.
---
As for the new 16-bit test, see also the subsequent "x86: harmonize disp
with imm handling" - it's at least debatable whether wrapping (in 16
bits) wouldn't better match 32-bit code's wrapping in 32-bits, as (to
some degree at least, gets put in place there), in which case this
testcase may better not be added here, as it would then "document" wrong
behavior.

Comments

Luis Machado via Binutils June 17, 2021, 2:40 p.m. | #1
On Mon, Jun 14, 2021 at 3:24 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> Just like e.g. 0x10000 triggers a warning for size 2, -0x10000 ought to

> as well.

>

> Note that some of the encodings produced aren't ones one would expect,

> and hence the generated code is not being checked for in the new

> testcases.

>

> gas/

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

>

>         * config/tc-i386.c (offset_in_range): Adjust conditional.

>         * testsuite/gas/i386/disp-imm-16.s,

>         testsuite/gas/i386/disp-imm-16.l,

>         testsuite/gas/i386/disp-imm-64.s,

>         testsuite/gas/i386/disp-imm-64.l: New.

>         * testsuite/gas/i386/i386.exp: Run new tests.

> ---

> As for the new 16-bit test, see also the subsequent "x86: harmonize disp

> with imm handling" - it's at least debatable whether wrapping (in 16

> bits) wouldn't better match 32-bit code's wrapping in 32-bits, as (to

> some degree at least, gets put in place there), in which case this

> testcase may better not be added here, as it would then "document" wrong

> behavior.



OK.

Thanks.

-- 
H.J.
Luis Machado via Binutils June 18, 2021, 10:48 a.m. | #2
On 17.06.2021 16:40, H.J. Lu wrote:
> On Mon, Jun 14, 2021 at 3:24 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> Just like e.g. 0x10000 triggers a warning for size 2, -0x10000 ought to

>> as well.

>>

>> Note that some of the encodings produced aren't ones one would expect,

>> and hence the generated code is not being checked for in the new

>> testcases.

>>

>> gas/

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

>>

>>         * config/tc-i386.c (offset_in_range): Adjust conditional.

>>         * testsuite/gas/i386/disp-imm-16.s,

>>         testsuite/gas/i386/disp-imm-16.l,

>>         testsuite/gas/i386/disp-imm-64.s,

>>         testsuite/gas/i386/disp-imm-64.l: New.

>>         * testsuite/gas/i386/i386.exp: Run new tests.

>> ---

>> As for the new 16-bit test, see also the subsequent "x86: harmonize disp

>> with imm handling" - it's at least debatable whether wrapping (in 16

>> bits) wouldn't better match 32-bit code's wrapping in 32-bits, as (to

>> some degree at least, gets put in place there), in which case this

>> testcase may better not be added here, as it would then "document" wrong

>> behavior.

> 

> 

> OK.


Thanks, but did you perhaps miss that you had given your okay for the
entire series already on the 14th?

Jan

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2563,7 +2563,7 @@  offset_in_range (offsetT val, int size)
     default: abort ();
     }
 
-  if ((val & ~mask) != 0 && (val & ~mask) != ~mask)
+  if ((val & ~mask) != 0 && (-val & ~mask) != 0)
     {
       char buf1[40], buf2[40];
 
--- /dev/null
+++ b/gas/testsuite/gas/i386/disp-imm-16.l
@@ -0,0 +1,10 @@ 
+.*: Assembler messages:
+.*:7: Warning: .* shortened to .*
+.*:8: Warning: .* shortened to .*
+.*:9: Warning: .* shortened to .*
+.*:11: Warning: .* shortened to .*
+.*:12: Warning: .* shortened to .*
+.*:13: Warning: .* shortened to .*
+.*:15: Warning: .* shortened to .*
+.*:16: Warning: .* shortened to .*
+.*:17: Warning: .* shortened to .*
--- /dev/null
+++ b/gas/testsuite/gas/i386/disp-imm-16.s
@@ -0,0 +1,17 @@ 
+	.text
+	.code16
+	mov	-0xffff(%bx), %eax
+	vmovaps	-0xffc0(%bx), %zmm0
+	add	$-0xffff, %cx
+
+	mov	-0xffff-1(%bx), %eax
+	vmovaps	-0xffc0-0x40(%bx), %zmm0
+	add	$-0xffff-1, %cx
+
+	mov	-0xffff-2(%bx), %eax
+	vmovaps	-0xffc0-0x80(%bx), %zmm0
+	add	$-0xffff-2, %cx
+
+	mov	-0x1ffff(%bx), %eax
+	vmovaps	-0x1ffc0(%bx), %zmm0
+	add	$-0x1ffff, %cx
--- /dev/null
+++ b/gas/testsuite/gas/i386/disp-imm-64.l
@@ -0,0 +1,22 @@ 
+.*: Assembler messages:
+.*:2: Error: .*
+.*:4: Error: .*
+.*:6: Error: .*
+.*:9: Error: .*
+.*:10: Warning: .* shortened to .*
+.*:11: Error: .*
+.*:12: Warning: .* shortened to .*
+.*:13: Error: .*
+.*:14: Warning: .* shortened to .*
+.*:16: Error: .*
+.*:17: Warning: .* shortened to .*
+.*:18: Error: .*
+.*:19: Warning: .* shortened to .*
+.*:20: Error: .*
+.*:21: Warning: .* shortened to .*
+.*:23: Error: .*
+.*:24: Warning: .* shortened to .*
+.*:25: Error: .*
+.*:26: Warning: .* shortened to .*
+.*:27: Error: .*
+.*:28: Warning: .* shortened to .*
--- /dev/null
+++ b/gas/testsuite/gas/i386/disp-imm-64.s
@@ -0,0 +1,28 @@ 
+	.text
+	mov	-0xffffffff(%rax), %eax
+	mov	-0xffffffff(%eax), %eax
+	vmovaps	-0xffffffc0(%rax), %zmm0
+	vmovaps	-0xffffffc0(%eax), %zmm0
+	add	$-0xffffffff, %rcx
+	add	$-0xffffffff, %ecx
+
+	mov	-0xffffffff-1(%rax), %eax
+	mov	-0xffffffff-1(%eax), %eax
+	vmovaps	-0xffffffc0-0x40(%rax), %zmm0
+	vmovaps	-0xffffffc0-0x40(%eax), %zmm0
+	add	$-0xffffffff-1, %rcx
+	add	$-0xffffffff-1, %ecx
+
+	mov	-0xffffffff-2(%rax), %eax
+	mov	-0xffffffff-2(%eax), %eax
+	vmovaps	-0xffffffc0-0x80(%rax), %zmm0
+	vmovaps	-0xffffffc0-0x80(%eax), %zmm0
+	add	$-0xffffffff-2, %rcx
+	add	$-0xffffffff-2, %ecx
+
+	mov	-0x1ffffffff(%rax), %eax
+	mov	-0x1ffffffff(%eax), %eax
+	vmovaps	-0x1ffffffc0(%rax), %zmm0
+	vmovaps	-0x1ffffffc0(%eax), %zmm0
+	add	$-0x1ffffffff, %rcx
+	add	$-0x1ffffffff, %ecx
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -87,6 +87,7 @@  if [gas_32_check] then {
     run_dump_test "disp"
     run_dump_test "disp-intel"
     run_dump_test "disp32"
+    run_list_test "disp-imm-16"
     run_dump_test "vmx"
     run_dump_test "vmfunc"
     run_dump_test "smx"
@@ -861,6 +862,7 @@  if [gas_64_check] then {
     run_dump_test "x86-64-sib-intel"
     run_dump_test "x86-64-disp"
     run_dump_test "x86-64-disp-intel"
+    run_list_test "disp-imm-64"
     run_dump_test "intel-movs64"
     run_dump_test "intel-cmps64"
     run_dump_test "x86-64-disp32"