[PATCH 0/6] x86: further value overflow diagnostic adjustments

[PATCH 0/6] x86: further value overflow diagnostic adjustments

[Jan Beulich]

... in patches 1 and 3, plus again some tidying fallout.

1: off-by-1 in offset_in_range()
2: make offset_in_range()'s warning contents useful (again)
3: harmonize disp with imm handling
4: slightly simplify offset_in_range()
5: simplify .dispNN setting
6: bring "gas --help" output for --32 etc in sync with reality

Jan

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

[Jan Beulich]

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.

--- 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"

[PATCH 2/6] x86: make offset_in_range()'s warning contents useful (again)

[Jan Beulich]

In case there is something which gets shortened (perhaps only on a BFD64
build targeting a 32-bit binary), seeing the full original value is
often helpful to understand what's actually going wrong. Of course for
non-64-bit binaries we better wouldn't be seeing such warnings at all,
as they're often indicative of a behavioral difference between BFD64 and
!BFD64 builds.

Prior to "gas: drop sprint_value()", which introduced the use of
bfd_sprintf_vma(), the output had other shortcomings.

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

	* config/tc-i386.c (offset_in_range): Replace uses of
	bfd_sprintf_vma.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2564,13 +2564,9 @@ offset_in_range (offsetT val, int size)
     }
 
   if ((val & ~mask) != 0 && (-val & ~mask) != 0)
-    {
-      char buf1[40], buf2[40];
+    as_warn (_("%"BFD_VMA_FMT"x shortened to %"BFD_VMA_FMT"x"),
+             val, val & mask);
 
-      bfd_sprintf_vma (stdoutput, buf1, val);
-      bfd_sprintf_vma (stdoutput, buf2, val & mask);
-      as_warn (_("%s shortened to %s"), buf1, buf2);
-    }
   return val & mask;
 }
 

[PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

Certain disp values may trigger "... shortened to ..." warnings when
equivalent imm ones don't. In some of the cases there are also
differences (for non-64-bit code) between BFD64 and !BFD64 builds. The
resulting encodings (i.e. use [or not] of the shorter disp8 / imm8
forms) are also different in some cases. Make this handling consistent.

Note that using equivalent 16-bit mode displacements / immediates
continues to expose entirely different behavior (see the disp-imm-16
testcase added by an earlier patch). This may want to be the subject of
further changes, but it'll then quickly become obvious that e.g. keying
use of extend_to_32bit_address() to non-64-bit mode isn't appropriate
either: Once we allow wrapping operands, we would better do so
consistently, in which case all of this would need to become dependent
upon address or operand size instead of mode.

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

	* config/tc-i386.c (optimize_disp): Generalize disp32 part of
	the BFD64-only logic to also apply to non-64-bit code.
	(i386_finalize_displacement): Use extend_to_32bit_address for
	non-64-bit code. Drop now redundant O_constant checks.
	* testsuite/gas/i386/disp-imm-32.s,
	testsuite/gas/i386/disp-imm-32.d: New.
	* testsuite/gas/i386/i386.exp: Run new test.

---
It may remain a point for discussion whether immediates/displacements
which are obviously exceeding 32 bit (like present in the new test case)
wouldn't better trigger minimally a warning, to at least sort of match
!BFD64's "missing or invalid {displacement,immediate} expression" errors
in such cases. But telling apart expressions which have overflowed (in
32 bits) from ones where larger-than-32-bit constants were present may
end up being difficult.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5905,26 +5905,24 @@ optimize_disp (void)
 	      }
 
 #ifdef BFD64
-	    if (flag_code == CODE_64BIT)
+	    /* Optimize 64-bit displacement to 32-bit for 64-bit BFD.  */
+	    if ((i.types[op].bitfield.disp32
+		 || (flag_code == CODE_64BIT
+		     && want_disp32 (current_templates->start)))
+		&& fits_in_unsigned_long (op_disp))
 	      {
-		/* Optimize 64-bit displacement to 32-bit for 64-bit BFD.  */
-		if ((i.types[op].bitfield.disp32
-		     || want_disp32 (current_templates->start))
-		    && fits_in_unsigned_long (op_disp))
-		  {
-		    /* If this operand is at most 32 bits, convert
-		       to a signed 32 bit number and don't use 64bit
-		       displacement.  */
-		    op_disp = (op_disp ^ ((offsetT) 1 << 31)) - ((addressT) 1 << 31);
-		    i.types[op].bitfield.disp64 = 0;
-		    i.types[op].bitfield.disp32 = 1;
-		  }
+		/* If this operand is at most 32 bits, convert
+		   to a signed 32 bit number and don't use 64bit
+		   displacement.  */
+		op_disp = (op_disp ^ ((offsetT) 1 << 31)) - ((addressT) 1 << 31);
+		i.types[op].bitfield.disp64 = 0;
+		i.types[op].bitfield.disp32 = 1;
+	      }
 
-		if (fits_in_signed_long (op_disp))
-		  {
-		    i.types[op].bitfield.disp64 = 0;
-		    i.types[op].bitfield.disp32s = 1;
-		  }
+	    if (flag_code == CODE_64BIT && fits_in_signed_long (op_disp))
+	      {
+		i.types[op].bitfield.disp64 = 0;
+		i.types[op].bitfield.disp32s = 1;
 	      }
 #endif
 	    if ((i.types[op].bitfield.disp32
@@ -11019,9 +11017,18 @@ i386_finalize_displacement (segT exp_seg
       ret = 0;
     }
 
+  else if (exp->X_op == O_constant)
+    {
+      /* Sizing gets taken care of by optimize_disp().
+
+	 If not 64bit, sign/zero extend val, to account for wraparound
+	 when !BFD64.  */
+      if (flag_code != CODE_64BIT)
+	exp->X_add_number = extend_to_32bit_address (exp->X_add_number);
+    }
+
 #if (defined (OBJ_AOUT) || defined (OBJ_MAYBE_AOUT))
-  else if (exp->X_op != O_constant
-	   && OUTPUT_FLAVOR == bfd_target_aout_flavour
+  else if (OUTPUT_FLAVOR == bfd_target_aout_flavour
 	   && exp_seg != absolute_section
 	   && exp_seg != text_section
 	   && exp_seg != data_section
@@ -11034,9 +11041,7 @@ i386_finalize_displacement (segT exp_seg
     }
 #endif
 
-  if (current_templates->start->opcode_modifier.jump == JUMP_BYTE
-      /* Constants get taken care of by optimize_disp().  */
-      && exp->X_op != O_constant)
+  else if (current_templates->start->opcode_modifier.jump == JUMP_BYTE)
     i.types[this_operand].bitfield.disp8 = 1;
 
   /* Check if this is a displacement only operand.  */
--- /dev/null
+++ b/gas/testsuite/gas/i386/disp-imm-32.d
@@ -0,0 +1,21 @@
+#objdump: -dw
+#name: i386 displacements / immediates (32-bit)
+
+.*: +file format .*
+
+Disassembly of section .text:
+
+0+ <disp_imm>:
+[ 	]*[a-f0-9]+:	8b 40 01             	mov    0x1\(%eax\),%eax
+[ 	]*[a-f0-9]+:	62 f1 7c 48 28 40 01 	vmovaps 0x40\(%eax\),%zmm0
+[ 	]*[a-f0-9]+:	83 c1 01             	add    \$0x1,%ecx
+[ 	]*[a-f0-9]+:	8b 00                	mov    \(%eax\),%eax
+[ 	]*[a-f0-9]+:	62 f1 7c 48 28 00    	vmovaps \(%eax\),%zmm0
+[ 	]*[a-f0-9]+:	83 c1 00             	add    \$0x0,%ecx
+[ 	]*[a-f0-9]+:	8b 40 ff             	mov    -0x1\(%eax\),%eax
+[ 	]*[a-f0-9]+:	62 f1 7c 48 28 40 ff 	vmovaps -0x40\(%eax\),%zmm0
+[ 	]*[a-f0-9]+:	83 c1 ff             	add    \$0xffffffff,%ecx
+[ 	]*[a-f0-9]+:	8b 40 01             	mov    0x1\(%eax\),%eax
+[ 	]*[a-f0-9]+:	62 f1 7c 48 28 40 01 	vmovaps 0x40\(%eax\),%zmm0
+[ 	]*[a-f0-9]+:	83 c1 01             	add    \$0x1,%ecx
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/disp-imm-32.s
@@ -0,0 +1,17 @@
+	.text
+disp_imm:
+	mov	-0xffffffff(%eax), %eax
+	vmovaps	-0xffffffc0(%eax), %zmm0
+	add	$-0xffffffff, %ecx
+
+	mov	-0xffffffff-1(%eax), %eax
+	vmovaps	-0xffffffc0-0x40(%eax), %zmm0
+	add	$-0xffffffff-1, %ecx
+
+	mov	-0xffffffff-2(%eax), %eax
+	vmovaps	-0xffffffc0-0x80(%eax), %zmm0
+	add	$-0xffffffff-2, %ecx
+
+	mov	-0x1ffffffff(%eax), %eax
+	vmovaps	-0x1ffffffc0(%eax), %zmm0
+	add	$-0x1ffffffff, %ecx
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -88,6 +88,9 @@ if [gas_32_check] then {
     run_dump_test "disp-intel"
     run_dump_test "disp32"
     run_list_test "disp-imm-16"
+    if { [gas_bfd64_check] } {
+	run_dump_test "disp-imm-32"
+    }
     run_dump_test "vmx"
     run_dump_test "vmfunc"
     run_dump_test "smx"

[PATCH 4/6] x86: slightly simplify offset_in_range()

[Jan Beulich]

Applying a mask with all bits set (or its inverse, with hence all bits
clear) won't alter the result (or won't trigger the warning). Re-arrange
the code to eliminate two more of the somewhat odd (2 << width_minus_1)
constructs.

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

	* config/tc-i386.c (offset_in_range): Bail early when mask would
	cover all bits anyway.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2556,10 +2556,10 @@ offset_in_range (offsetT val, int size)
     {
     case 1: mask = ((addressT) 1 <<  8) - 1; break;
     case 2: mask = ((addressT) 1 << 16) - 1; break;
-    case 4: mask = ((addressT) 2 << 31) - 1; break;
 #ifdef BFD64
-    case 8: mask = ((addressT) 2 << 63) - 1; break;
+    case 4: mask = ((addressT) 1 << 32) - 1; break;
 #endif
+    case sizeof (val): return val;
     default: abort ();
     }
 

[PATCH 5/6] x86: simplify .dispNN setting

[Jan Beulich]

First of all eliminate the disp{16,32,32s} static variables, which are
used solely for setting a temporary variable in build_modrm_byte(). The
effect there can be had without use of such a temporary and without
operand_type_or(), by just setting the single bit each that needs
setting.

Then use operand_type_and_not(..., anydisp) when all dispNN bits want
clearing together.

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

	* config/tc-i386.c (disp16, disp32, disp32s): Delete.
	(optimize_disp, i386_finalize_displacement): Use
	operand_type_and_not.
	(build_modrm_byte): Likewise. Eliminate local variable newdisp.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2089,9 +2089,6 @@ operand_type_xor (i386_operand_type x, i
   return x;
 }
 
-static const i386_operand_type disp16 = OPERAND_TYPE_DISP16;
-static const i386_operand_type disp32 = OPERAND_TYPE_DISP32;
-static const i386_operand_type disp32s = OPERAND_TYPE_DISP32S;
 static const i386_operand_type disp16_32 = OPERAND_TYPE_DISP16_32;
 static const i386_operand_type anydisp = OPERAND_TYPE_ANYDISP;
 static const i386_operand_type anyimm = OPERAND_TYPE_ANYIMM;
@@ -5884,12 +5881,8 @@ optimize_disp (void)
 
 	    if (!op_disp && i.types[op].bitfield.baseindex)
 	      {
-		i.types[op].bitfield.disp8 = 0;
-		i.types[op].bitfield.disp16 = 0;
-		i.types[op].bitfield.disp32 = 0;
-		i.types[op].bitfield.disp32s = 0;
-		i.types[op].bitfield.disp64 = 0;
-		i.op[op].disps = 0;
+		i.types[op] = operand_type_and_not (i.types[op], anydisp);
+		i.op[op].disps = NULL;
 		i.disp_operands--;
 		continue;
 	      }
@@ -5938,11 +5931,7 @@ optimize_disp (void)
 	  {
 	    fix_new_exp (frag_now, frag_more (0) - frag_now->fr_literal, 0,
 			 i.op[op].disps, 0, i.reloc[op]);
-	    i.types[op].bitfield.disp8 = 0;
-	    i.types[op].bitfield.disp16 = 0;
-	    i.types[op].bitfield.disp32 = 0;
-	    i.types[op].bitfield.disp32s = 0;
-	    i.types[op].bitfield.disp64 = 0;
+	    i.types[op] = operand_type_and_not (i.types[op], anydisp);
 	  }
  	else
 	  /* We only support 64bit displacement on constants.  */
@@ -8261,20 +8250,11 @@ build_modrm_byte (void)
 		{
 		  i.sib.base = NO_BASE_REGISTER;
 		  i.sib.scale = i.log2_scale_factor;
-		  i.types[op].bitfield.disp8 = 0;
-		  i.types[op].bitfield.disp16 = 0;
-		  i.types[op].bitfield.disp64 = 0;
+		  i.types[op] = operand_type_and_not (i.types[op], anydisp);
 		  if (want_disp32 (&i.tm))
-		    {
-		      /* Must be 32 bit */
-		      i.types[op].bitfield.disp32 = 1;
-		      i.types[op].bitfield.disp32s = 0;
-		    }
+		    i.types[op].bitfield.disp32 = 1;
 		  else
-		    {
-		      i.types[op].bitfield.disp32 = 0;
-		      i.types[op].bitfield.disp32s = 1;
-		    }
+		    i.types[op].bitfield.disp32s = 1;
 		}
 
 	      /* Since the mandatory SIB always has index register, so
@@ -8300,12 +8280,11 @@ build_modrm_byte (void)
 		fake_zero_displacement = 1;
 	      if (i.index_reg == 0)
 		{
-		  i386_operand_type newdisp;
-
 		  /* Both check for VSIB and mandatory non-vector SIB. */
 		  gas_assert (!i.tm.opcode_modifier.sib
 			      || i.tm.opcode_modifier.sib == SIBMEM);
 		  /* Operand is just <disp>  */
+		  i.types[op] = operand_type_and_not (i.types[op], anydisp);
 		  if (flag_code == CODE_64BIT)
 		    {
 		      /* 64bit mode overwrites the 32bit absolute
@@ -8315,21 +8294,22 @@ build_modrm_byte (void)
 		      i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;
 		      i.sib.base = NO_BASE_REGISTER;
 		      i.sib.index = NO_INDEX_REGISTER;
-		      newdisp = (want_disp32(&i.tm) ? disp32 : disp32s);
+		      if (want_disp32 (&i.tm))
+			i.types[op].bitfield.disp32 = 1;
+		      else
+			i.types[op].bitfield.disp32s = 1;
 		    }
 		  else if ((flag_code == CODE_16BIT)
 			   ^ (i.prefix[ADDR_PREFIX] != 0))
 		    {
 		      i.rm.regmem = NO_BASE_REGISTER_16;
-		      newdisp = disp16;
+		      i.types[op].bitfield.disp16 = 1;
 		    }
 		  else
 		    {
 		      i.rm.regmem = NO_BASE_REGISTER;
-		      newdisp = disp32;
+		      i.types[op].bitfield.disp32 = 1;
 		    }
-		  i.types[op] = operand_type_and_not (i.types[op], anydisp);
-		  i.types[op] = operand_type_or (i.types[op], newdisp);
 		}
 	      else if (!i.tm.opcode_modifier.sib)
 		{
@@ -8341,20 +8321,11 @@ build_modrm_byte (void)
 		  i.sib.base = NO_BASE_REGISTER;
 		  i.sib.scale = i.log2_scale_factor;
 		  i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;
-		  i.types[op].bitfield.disp8 = 0;
-		  i.types[op].bitfield.disp16 = 0;
-		  i.types[op].bitfield.disp64 = 0;
+		  i.types[op] = operand_type_and_not (i.types[op], anydisp);
 		  if (want_disp32 (&i.tm))
-		    {
-		      /* Must be 32 bit */
-		      i.types[op].bitfield.disp32 = 1;
-		      i.types[op].bitfield.disp32s = 0;
-		    }
+		    i.types[op].bitfield.disp32 = 1;
 		  else
-		    {
-		      i.types[op].bitfield.disp32 = 0;
-		      i.types[op].bitfield.disp32s = 1;
-		    }
+		    i.types[op].bitfield.disp32s = 1;
 		  if ((i.index_reg->reg_flags & RegRex) != 0)
 		    i.rex |= REX_X;
 		}
@@ -11045,12 +11016,7 @@ i386_finalize_displacement (segT exp_seg
     i.types[this_operand].bitfield.disp8 = 1;
 
   /* Check if this is a displacement only operand.  */
-  bigdisp = i.types[this_operand];
-  bigdisp.bitfield.disp8 = 0;
-  bigdisp.bitfield.disp16 = 0;
-  bigdisp.bitfield.disp32 = 0;
-  bigdisp.bitfield.disp32s = 0;
-  bigdisp.bitfield.disp64 = 0;
+  bigdisp = operand_type_and_not (i.types[this_operand], anydisp);
   if (operand_type_all_zero (&bigdisp))
     i.types[this_operand] = operand_type_and (i.types[this_operand],
 					      types);

[PATCH 6/6] x86: bring "gas --help" output for --32 etc in sync with reality

[Jan Beulich]

The testsuite uses the output to determine whether BFD64 is in effect.

--x32 is supported for ELF only; don't advertise it for PE/COFF. --64 is
also supported for Mach-O; advertise it. Adjust the testsuite's BFD64
check accordingly.

Also replace "code" by "object", since it's the object format that the
options primarily control. It's also _initial_ code bitness, but this
can be changed by directives.

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

	* config/tc-i386.c (md_show_usage): Split ELF and PE/COFF parts
	of object format controlling option. Add Mach-O to the latter.
	* testsuite/gas/i386/i386.exp (gas_bfd64_check): Adjust
	accordingly.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -13719,10 +13719,14 @@ md_show_usage (FILE *stream)
   fprintf (stream, _("\
   -s                      ignored\n"));
 #endif
-#if defined BFD64 && (defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF) \
-		      || defined (TE_PE) || defined (TE_PEP))
+#ifdef BFD64
+# if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
   fprintf (stream, _("\
-  --32/--64/--x32         generate 32bit/64bit/x32 code\n"));
+  --32/--64/--x32         generate 32bit/64bit/x32 object\n"));
+# elif defined (TE_PE) || defined (TE_PEP) || defined (OBJ_MACH_O)
+  fprintf (stream, _("\
+  --32/--64               generate 32bit/64bit object\n"));
+# endif
 #endif
 #ifdef SVR4_COMMENT_CHARS
   fprintf (stream, _("\
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -41,7 +41,7 @@ proc gas_bfd64_check { } {
     global AS
 
     set status [gas_host_run "$AS --help" ""]
-    return [regexp "32bit/64bit/x32" [lindex $status 1]];
+    return [regexp "32bit/64bit" [lindex $status 1]];
 }
 
 if [gas_32_check] then {

Re: [PATCH 0/6] x86: further value overflow diagnostic adjustments

[H.J. Lu]

On Monday, June 14, 2021, Jan Beulich <jbeulich@suse.com> wrote:

> ... in patches 1 and 3, plus again some tidying fallout.
>
> 1: off-by-1 in offset_in_range()
> 2: make offset_in_range()'s warning contents useful (again)
> 3: harmonize disp with imm handling
> 4: slightly simplify offset_in_range()
> 5: simplify .dispNN setting
> 6: bring "gas --help" output for --32 etc in sync with reality
>
> Jan
>
>
Ok for all.

Thanks.

H.J.


-- 
H.J.

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

[H.J. Lu]

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.

Re: [PATCH 2/6] x86: make offset_in_range()'s warning contents useful (again)

[H.J. Lu]

On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> In case there is something which gets shortened (perhaps only on a BFD64
> build targeting a 32-bit binary), seeing the full original value is
> often helpful to understand what's actually going wrong. Of course for
> non-64-bit binaries we better wouldn't be seeing such warnings at all,
> as they're often indicative of a behavioral difference between BFD64 and
> !BFD64 builds.
>
> Prior to "gas: drop sprint_value()", which introduced the use of
> bfd_sprintf_vma(), the output had other shortcomings.
>
> gas/
> 2021-06-XX  Jan Beulich  <jbeulich@suse.com>
>
>         * config/tc-i386.c (offset_in_range): Replace uses of
>         bfd_sprintf_vma.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2564,13 +2564,9 @@ offset_in_range (offsetT val, int size)
>      }
>
>    if ((val & ~mask) != 0 && (-val & ~mask) != 0)
> -    {
> -      char buf1[40], buf2[40];
> +    as_warn (_("%"BFD_VMA_FMT"x shortened to %"BFD_VMA_FMT"x"),
> +             val, val & mask);
>
> -      bfd_sprintf_vma (stdoutput, buf1, val);
> -      bfd_sprintf_vma (stdoutput, buf2, val & mask);
> -      as_warn (_("%s shortened to %s"), buf1, buf2);
> -    }
>    return val & mask;
>  }
>
>

OK.

Thanks.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Certain disp values may trigger "... shortened to ..." warnings when
> equivalent imm ones don't. In some of the cases there are also
> differences (for non-64-bit code) between BFD64 and !BFD64 builds. The
> resulting encodings (i.e. use [or not] of the shorter disp8 / imm8
> forms) are also different in some cases. Make this handling consistent.
>
> Note that using equivalent 16-bit mode displacements / immediates
> continues to expose entirely different behavior (see the disp-imm-16
> testcase added by an earlier patch). This may want to be the subject of
> further changes, but it'll then quickly become obvious that e.g. keying
> use of extend_to_32bit_address() to non-64-bit mode isn't appropriate
> either: Once we allow wrapping operands, we would better do so
> consistently, in which case all of this would need to become dependent
> upon address or operand size instead of mode.
>
> gas/
> 2021-06-XX  Jan Beulich  <jbeulich@suse.com>
>
>         * config/tc-i386.c (optimize_disp): Generalize disp32 part of
>         the BFD64-only logic to also apply to non-64-bit code.
>         (i386_finalize_displacement): Use extend_to_32bit_address for
>         non-64-bit code. Drop now redundant O_constant checks.
>         * testsuite/gas/i386/disp-imm-32.s,
>         testsuite/gas/i386/disp-imm-32.d: New.
>         * testsuite/gas/i386/i386.exp: Run new test.
>
> ---
> It may remain a point for discussion whether immediates/displacements
> which are obviously exceeding 32 bit (like present in the new test case)
> wouldn't better trigger minimally a warning, to at least sort of match
> !BFD64's "missing or invalid {displacement,immediate} expression" errors
> in such cases. But telling apart expressions which have overflowed (in
> 32 bits) from ones where larger-than-32-bit constants were present may
> end up being difficult.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5905,26 +5905,24 @@ optimize_disp (void)
>               }
>
>  #ifdef BFD64
> -           if (flag_code == CODE_64BIT)
> +           /* Optimize 64-bit displacement to 32-bit for 64-bit BFD.  */
> +           if ((i.types[op].bitfield.disp32
> +                || (flag_code == CODE_64BIT
> +                    && want_disp32 (current_templates->start)))
> +               && fits_in_unsigned_long (op_disp))
>               {
> -               /* Optimize 64-bit displacement to 32-bit for 64-bit BFD.  */
> -               if ((i.types[op].bitfield.disp32
> -                    || want_disp32 (current_templates->start))
> -                   && fits_in_unsigned_long (op_disp))
> -                 {
> -                   /* If this operand is at most 32 bits, convert
> -                      to a signed 32 bit number and don't use 64bit
> -                      displacement.  */
> -                   op_disp = (op_disp ^ ((offsetT) 1 << 31)) - ((addressT) 1 << 31);
> -                   i.types[op].bitfield.disp64 = 0;
> -                   i.types[op].bitfield.disp32 = 1;
> -                 }
> +               /* If this operand is at most 32 bits, convert
> +                  to a signed 32 bit number and don't use 64bit
> +                  displacement.  */
> +               op_disp = (op_disp ^ ((offsetT) 1 << 31)) - ((addressT) 1 << 31);
> +               i.types[op].bitfield.disp64 = 0;
> +               i.types[op].bitfield.disp32 = 1;
> +             }
>
> -               if (fits_in_signed_long (op_disp))
> -                 {
> -                   i.types[op].bitfield.disp64 = 0;
> -                   i.types[op].bitfield.disp32s = 1;
> -                 }
> +           if (flag_code == CODE_64BIT && fits_in_signed_long (op_disp))
> +             {
> +               i.types[op].bitfield.disp64 = 0;
> +               i.types[op].bitfield.disp32s = 1;
>               }
>  #endif
>             if ((i.types[op].bitfield.disp32
> @@ -11019,9 +11017,18 @@ i386_finalize_displacement (segT exp_seg
>        ret = 0;
>      }
>
> +  else if (exp->X_op == O_constant)
> +    {
> +      /* Sizing gets taken care of by optimize_disp().
> +
> +        If not 64bit, sign/zero extend val, to account for wraparound
> +        when !BFD64.  */
> +      if (flag_code != CODE_64BIT)
> +       exp->X_add_number = extend_to_32bit_address (exp->X_add_number);
> +    }
> +
>  #if (defined (OBJ_AOUT) || defined (OBJ_MAYBE_AOUT))
> -  else if (exp->X_op != O_constant
> -          && OUTPUT_FLAVOR == bfd_target_aout_flavour
> +  else if (OUTPUT_FLAVOR == bfd_target_aout_flavour
>            && exp_seg != absolute_section
>            && exp_seg != text_section
>            && exp_seg != data_section
> @@ -11034,9 +11041,7 @@ i386_finalize_displacement (segT exp_seg
>      }
>  #endif
>
> -  if (current_templates->start->opcode_modifier.jump == JUMP_BYTE
> -      /* Constants get taken care of by optimize_disp().  */
> -      && exp->X_op != O_constant)
> +  else if (current_templates->start->opcode_modifier.jump == JUMP_BYTE)
>      i.types[this_operand].bitfield.disp8 = 1;
>
>    /* Check if this is a displacement only operand.  */
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/disp-imm-32.d
> @@ -0,0 +1,21 @@
> +#objdump: -dw
> +#name: i386 displacements / immediates (32-bit)
> +
> +.*: +file format .*
> +
> +Disassembly of section .text:
> +
> +0+ <disp_imm>:
> +[      ]*[a-f0-9]+:    8b 40 01                mov    0x1\(%eax\),%eax
> +[      ]*[a-f0-9]+:    62 f1 7c 48 28 40 01    vmovaps 0x40\(%eax\),%zmm0
> +[      ]*[a-f0-9]+:    83 c1 01                add    \$0x1,%ecx
> +[      ]*[a-f0-9]+:    8b 00                   mov    \(%eax\),%eax
> +[      ]*[a-f0-9]+:    62 f1 7c 48 28 00       vmovaps \(%eax\),%zmm0
> +[      ]*[a-f0-9]+:    83 c1 00                add    \$0x0,%ecx
> +[      ]*[a-f0-9]+:    8b 40 ff                mov    -0x1\(%eax\),%eax
> +[      ]*[a-f0-9]+:    62 f1 7c 48 28 40 ff    vmovaps -0x40\(%eax\),%zmm0
> +[      ]*[a-f0-9]+:    83 c1 ff                add    \$0xffffffff,%ecx
> +[      ]*[a-f0-9]+:    8b 40 01                mov    0x1\(%eax\),%eax
> +[      ]*[a-f0-9]+:    62 f1 7c 48 28 40 01    vmovaps 0x40\(%eax\),%zmm0
> +[      ]*[a-f0-9]+:    83 c1 01                add    \$0x1,%ecx
> +#pass
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> @@ -0,0 +1,17 @@
> +       .text
> +disp_imm:
> +       mov     -0xffffffff(%eax), %eax

I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
We allow addresses to wraparound. I don't see a need for
displacements to wraparound.

> +       vmovaps -0xffffffc0(%eax), %zmm0
> +       add     $-0xffffffff, %ecx
> +
> +       mov     -0xffffffff-1(%eax), %eax
> +       vmovaps -0xffffffc0-0x40(%eax), %zmm0
> +       add     $-0xffffffff-1, %ecx
> +
> +       mov     -0xffffffff-2(%eax), %eax
> +       vmovaps -0xffffffc0-0x80(%eax), %zmm0
> +       add     $-0xffffffff-2, %ecx
> +
> +       mov     -0x1ffffffff(%eax), %eax
> +       vmovaps -0x1ffffffc0(%eax), %zmm0
> +       add     $-0x1ffffffff, %ecx
> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -88,6 +88,9 @@ if [gas_32_check] then {
>      run_dump_test "disp-intel"
>      run_dump_test "disp32"
>      run_list_test "disp-imm-16"
> +    if { [gas_bfd64_check] } {
> +       run_dump_test "disp-imm-32"
> +    }
>      run_dump_test "vmx"
>      run_dump_test "vmfunc"
>      run_dump_test "smx"
>


-- 
H.J.

Re: [PATCH 4/6] x86: slightly simplify offset_in_range()

[H.J. Lu]

On Mon, Jun 14, 2021 at 3:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Applying a mask with all bits set (or its inverse, with hence all bits
> clear) won't alter the result (or won't trigger the warning). Re-arrange
> the code to eliminate two more of the somewhat odd (2 << width_minus_1)
> constructs.
>
> gas/
> 2021-06-XX  Jan Beulich  <jbeulich@suse.com>
>
>         * config/tc-i386.c (offset_in_range): Bail early when mask would
>         cover all bits anyway.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2556,10 +2556,10 @@ offset_in_range (offsetT val, int size)
>      {
>      case 1: mask = ((addressT) 1 <<  8) - 1; break;
>      case 2: mask = ((addressT) 1 << 16) - 1; break;
> -    case 4: mask = ((addressT) 2 << 31) - 1; break;
>  #ifdef BFD64
> -    case 8: mask = ((addressT) 2 << 63) - 1; break;
> +    case 4: mask = ((addressT) 1 << 32) - 1; break;
>  #endif
> +    case sizeof (val): return val;
>      default: abort ();
>      }
>
>

OK.

Thanks.

-- 
H.J.

Re: [PATCH 5/6] x86: simplify .dispNN setting

[H.J. Lu]

On Mon, Jun 14, 2021 at 3:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> First of all eliminate the disp{16,32,32s} static variables, which are
> used solely for setting a temporary variable in build_modrm_byte(). The
> effect there can be had without use of such a temporary and without
> operand_type_or(), by just setting the single bit each that needs
> setting.
>
> Then use operand_type_and_not(..., anydisp) when all dispNN bits want
> clearing together.
>
> gas/
> 2021-06-XX  Jan Beulich  <jbeulich@suse.com>
>
>         * config/tc-i386.c (disp16, disp32, disp32s): Delete.
>         (optimize_disp, i386_finalize_displacement): Use
>         operand_type_and_not.
>         (build_modrm_byte): Likewise. Eliminate local variable newdisp.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -2089,9 +2089,6 @@ operand_type_xor (i386_operand_type x, i
>    return x;
>  }
>
> -static const i386_operand_type disp16 = OPERAND_TYPE_DISP16;
> -static const i386_operand_type disp32 = OPERAND_TYPE_DISP32;
> -static const i386_operand_type disp32s = OPERAND_TYPE_DISP32S;
>  static const i386_operand_type disp16_32 = OPERAND_TYPE_DISP16_32;
>  static const i386_operand_type anydisp = OPERAND_TYPE_ANYDISP;
>  static const i386_operand_type anyimm = OPERAND_TYPE_ANYIMM;
> @@ -5884,12 +5881,8 @@ optimize_disp (void)
>
>             if (!op_disp && i.types[op].bitfield.baseindex)
>               {
> -               i.types[op].bitfield.disp8 = 0;
> -               i.types[op].bitfield.disp16 = 0;
> -               i.types[op].bitfield.disp32 = 0;
> -               i.types[op].bitfield.disp32s = 0;
> -               i.types[op].bitfield.disp64 = 0;
> -               i.op[op].disps = 0;
> +               i.types[op] = operand_type_and_not (i.types[op], anydisp);
> +               i.op[op].disps = NULL;
>                 i.disp_operands--;
>                 continue;
>               }
> @@ -5938,11 +5931,7 @@ optimize_disp (void)
>           {
>             fix_new_exp (frag_now, frag_more (0) - frag_now->fr_literal, 0,
>                          i.op[op].disps, 0, i.reloc[op]);
> -           i.types[op].bitfield.disp8 = 0;
> -           i.types[op].bitfield.disp16 = 0;
> -           i.types[op].bitfield.disp32 = 0;
> -           i.types[op].bitfield.disp32s = 0;
> -           i.types[op].bitfield.disp64 = 0;
> +           i.types[op] = operand_type_and_not (i.types[op], anydisp);
>           }
>         else
>           /* We only support 64bit displacement on constants.  */
> @@ -8261,20 +8250,11 @@ build_modrm_byte (void)
>                 {
>                   i.sib.base = NO_BASE_REGISTER;
>                   i.sib.scale = i.log2_scale_factor;
> -                 i.types[op].bitfield.disp8 = 0;
> -                 i.types[op].bitfield.disp16 = 0;
> -                 i.types[op].bitfield.disp64 = 0;
> +                 i.types[op] = operand_type_and_not (i.types[op], anydisp);
>                   if (want_disp32 (&i.tm))
> -                   {
> -                     /* Must be 32 bit */
> -                     i.types[op].bitfield.disp32 = 1;
> -                     i.types[op].bitfield.disp32s = 0;
> -                   }
> +                   i.types[op].bitfield.disp32 = 1;
>                   else
> -                   {
> -                     i.types[op].bitfield.disp32 = 0;
> -                     i.types[op].bitfield.disp32s = 1;
> -                   }
> +                   i.types[op].bitfield.disp32s = 1;
>                 }
>
>               /* Since the mandatory SIB always has index register, so
> @@ -8300,12 +8280,11 @@ build_modrm_byte (void)
>                 fake_zero_displacement = 1;
>               if (i.index_reg == 0)
>                 {
> -                 i386_operand_type newdisp;
> -
>                   /* Both check for VSIB and mandatory non-vector SIB. */
>                   gas_assert (!i.tm.opcode_modifier.sib
>                               || i.tm.opcode_modifier.sib == SIBMEM);
>                   /* Operand is just <disp>  */
> +                 i.types[op] = operand_type_and_not (i.types[op], anydisp);
>                   if (flag_code == CODE_64BIT)
>                     {
>                       /* 64bit mode overwrites the 32bit absolute
> @@ -8315,21 +8294,22 @@ build_modrm_byte (void)
>                       i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;
>                       i.sib.base = NO_BASE_REGISTER;
>                       i.sib.index = NO_INDEX_REGISTER;
> -                     newdisp = (want_disp32(&i.tm) ? disp32 : disp32s);
> +                     if (want_disp32 (&i.tm))
> +                       i.types[op].bitfield.disp32 = 1;
> +                     else
> +                       i.types[op].bitfield.disp32s = 1;
>                     }
>                   else if ((flag_code == CODE_16BIT)
>                            ^ (i.prefix[ADDR_PREFIX] != 0))
>                     {
>                       i.rm.regmem = NO_BASE_REGISTER_16;
> -                     newdisp = disp16;
> +                     i.types[op].bitfield.disp16 = 1;
>                     }
>                   else
>                     {
>                       i.rm.regmem = NO_BASE_REGISTER;
> -                     newdisp = disp32;
> +                     i.types[op].bitfield.disp32 = 1;
>                     }
> -                 i.types[op] = operand_type_and_not (i.types[op], anydisp);
> -                 i.types[op] = operand_type_or (i.types[op], newdisp);
>                 }
>               else if (!i.tm.opcode_modifier.sib)
>                 {
> @@ -8341,20 +8321,11 @@ build_modrm_byte (void)
>                   i.sib.base = NO_BASE_REGISTER;
>                   i.sib.scale = i.log2_scale_factor;
>                   i.rm.regmem = ESCAPE_TO_TWO_BYTE_ADDRESSING;
> -                 i.types[op].bitfield.disp8 = 0;
> -                 i.types[op].bitfield.disp16 = 0;
> -                 i.types[op].bitfield.disp64 = 0;
> +                 i.types[op] = operand_type_and_not (i.types[op], anydisp);
>                   if (want_disp32 (&i.tm))
> -                   {
> -                     /* Must be 32 bit */
> -                     i.types[op].bitfield.disp32 = 1;
> -                     i.types[op].bitfield.disp32s = 0;
> -                   }
> +                   i.types[op].bitfield.disp32 = 1;
>                   else
> -                   {
> -                     i.types[op].bitfield.disp32 = 0;
> -                     i.types[op].bitfield.disp32s = 1;
> -                   }
> +                   i.types[op].bitfield.disp32s = 1;
>                   if ((i.index_reg->reg_flags & RegRex) != 0)
>                     i.rex |= REX_X;
>                 }
> @@ -11045,12 +11016,7 @@ i386_finalize_displacement (segT exp_seg
>      i.types[this_operand].bitfield.disp8 = 1;
>
>    /* Check if this is a displacement only operand.  */
> -  bigdisp = i.types[this_operand];
> -  bigdisp.bitfield.disp8 = 0;
> -  bigdisp.bitfield.disp16 = 0;
> -  bigdisp.bitfield.disp32 = 0;
> -  bigdisp.bitfield.disp32s = 0;
> -  bigdisp.bitfield.disp64 = 0;
> +  bigdisp = operand_type_and_not (i.types[this_operand], anydisp);
>    if (operand_type_all_zero (&bigdisp))
>      i.types[this_operand] = operand_type_and (i.types[this_operand],
>                                               types);
>

OK.

Thanks.

-- 
H.J.

Re: [PATCH 6/6] x86: bring "gas --help" output for --32 etc in sync with reality

[H.J. Lu]

On Mon, Jun 14, 2021 at 3:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> The testsuite uses the output to determine whether BFD64 is in effect.
>
> --x32 is supported for ELF only; don't advertise it for PE/COFF. --64 is
> also supported for Mach-O; advertise it. Adjust the testsuite's BFD64
> check accordingly.
>
> Also replace "code" by "object", since it's the object format that the
> options primarily control. It's also _initial_ code bitness, but this
> can be changed by directives.
>
> gas/
> 2021-06-XX  Jan Beulich  <jbeulich@suse.com>
>
>         * config/tc-i386.c (md_show_usage): Split ELF and PE/COFF parts
>         of object format controlling option. Add Mach-O to the latter.
>         * testsuite/gas/i386/i386.exp (gas_bfd64_check): Adjust
>         accordingly.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -13719,10 +13719,14 @@ md_show_usage (FILE *stream)
>    fprintf (stream, _("\
>    -s                      ignored\n"));
>  #endif
> -#if defined BFD64 && (defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF) \
> -                     || defined (TE_PE) || defined (TE_PEP))
> +#ifdef BFD64
> +# if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>    fprintf (stream, _("\
> -  --32/--64/--x32         generate 32bit/64bit/x32 code\n"));
> +  --32/--64/--x32         generate 32bit/64bit/x32 object\n"));
> +# elif defined (TE_PE) || defined (TE_PEP) || defined (OBJ_MACH_O)
> +  fprintf (stream, _("\
> +  --32/--64               generate 32bit/64bit object\n"));
> +# endif
>  #endif
>  #ifdef SVR4_COMMENT_CHARS
>    fprintf (stream, _("\
> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -41,7 +41,7 @@ proc gas_bfd64_check { } {
>      global AS
>
>      set status [gas_host_run "$AS --help" ""]
> -    return [regexp "32bit/64bit/x32" [lindex $status 1]];
> +    return [regexp "32bit/64bit" [lindex $status 1]];
>  }
>
>  if [gas_32_check] then {
>

OK.

Thanks.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 17.06.2021 16:46, H.J. Lu wrote:
> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>> --- /dev/null
>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
>> @@ -0,0 +1,17 @@
>> +       .text
>> +disp_imm:
>> +       mov     -0xffffffff(%eax), %eax
> 
> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
> We allow addresses to wraparound. I don't see a need for
> displacements to wraparound.

This then is entirely unexpected to the programmer. In fact the
same (abstracted away behind some defines or equates) constant
could be used for both purposes (and should be usable both ways,
imo).

Jan

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 17.06.2021 16:46, H.J. Lu wrote:
> > On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> --- /dev/null
> >> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> >> @@ -0,0 +1,17 @@
> >> +       .text
> >> +disp_imm:
> >> +       mov     -0xffffffff(%eax), %eax
> >
> > I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
> > We allow addresses to wraparound. I don't see a need for
> > displacements to wraparound.
>
> This then is entirely unexpected to the programmer. In fact the
> same (abstracted away behind some defines or equates) constant
> could be used for both purposes (and should be usable both ways,
> imo).
>

Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
wraparound (DISP) + BASE + INDEX * SCALE.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 17.06.2021 18:00, H.J. Lu wrote:
> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 17.06.2021 16:46, H.J. Lu wrote:
>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
>>>> @@ -0,0 +1,17 @@
>>>> +       .text
>>>> +disp_imm:
>>>> +       mov     -0xffffffff(%eax), %eax
>>>
>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
>>> We allow addresses to wraparound. I don't see a need for
>>> displacements to wraparound.
>>
>> This then is entirely unexpected to the programmer. In fact the
>> same (abstracted away behind some defines or equates) constant
>> could be used for both purposes (and should be usable both ways,
>> imo).
> 
> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
> wraparound (DISP) + BASE + INDEX * SCALE.

But this is true regardless of how small (or big) the displacement.
Without knowing the register values, you can't know at what
displacement values wraparound occurs. Also, unless I'm mistaken,
wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).

Jan

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 17.06.2021 18:00, H.J. Lu wrote:
> > On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 17.06.2021 16:46, H.J. Lu wrote:
> >>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> --- /dev/null
> >>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> >>>> @@ -0,0 +1,17 @@
> >>>> +       .text
> >>>> +disp_imm:
> >>>> +       mov     -0xffffffff(%eax), %eax
> >>>
> >>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
> >>> We allow addresses to wraparound. I don't see a need for
> >>> displacements to wraparound.
> >>
> >> This then is entirely unexpected to the programmer. In fact the
> >> same (abstracted away behind some defines or equates) constant
> >> could be used for both purposes (and should be usable both ways,
> >> imo).
> >
> > Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
> > on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
> > wraparound (DISP) + BASE + INDEX * SCALE.
>
> But this is true regardless of how small (or big) the displacement.
> Without knowing the register values, you can't know at what
> displacement values wraparound occurs. Also, unless I'm mistaken,
> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).

Hardware does wraparound (DISP + BASE + INDEX * SCALE).
Assembler and linker should only wraparound on the final address.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 17.06.2021 18:12, H.J. Lu wrote:
> On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 17.06.2021 18:00, H.J. Lu wrote:
>>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 17.06.2021 16:46, H.J. Lu wrote:
>>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
>>>>>> @@ -0,0 +1,17 @@
>>>>>> +       .text
>>>>>> +disp_imm:
>>>>>> +       mov     -0xffffffff(%eax), %eax
>>>>>
>>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
>>>>> We allow addresses to wraparound. I don't see a need for
>>>>> displacements to wraparound.
>>>>
>>>> This then is entirely unexpected to the programmer. In fact the
>>>> same (abstracted away behind some defines or equates) constant
>>>> could be used for both purposes (and should be usable both ways,
>>>> imo).
>>>
>>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
>>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
>>> wraparound (DISP) + BASE + INDEX * SCALE.
>>
>> But this is true regardless of how small (or big) the displacement.
>> Without knowing the register values, you can't know at what
>> displacement values wraparound occurs. Also, unless I'm mistaken,
>> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
> 
> Hardware does wraparound (DISP + BASE + INDEX * SCALE).
> Assembler and linker should only wraparound on the final address.

I'm afraid this last sentence makes no sense to me: The assembler
(or linker) can't know the final address. Instead, both immediates
and displacements should allow for anything the programmer might
sensibly use. If 0xffffffff as a displacement is fine (meaning
-1 really), -0xffffffff (meaning 1) ought to be, too. Or else
where do you draw the boundary of which displacements are
"legitimate" and which are not?

Jan

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

[Jan Beulich]

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

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Fri, Jun 18, 2021 at 2:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 17.06.2021 18:12, H.J. Lu wrote:
> > On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 17.06.2021 18:00, H.J. Lu wrote:
> >>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 17.06.2021 16:46, H.J. Lu wrote:
> >>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>> --- /dev/null
> >>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> >>>>>> @@ -0,0 +1,17 @@
> >>>>>> +       .text
> >>>>>> +disp_imm:
> >>>>>> +       mov     -0xffffffff(%eax), %eax
> >>>>>
> >>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
> >>>>> We allow addresses to wraparound. I don't see a need for
> >>>>> displacements to wraparound.
> >>>>
> >>>> This then is entirely unexpected to the programmer. In fact the
> >>>> same (abstracted away behind some defines or equates) constant
> >>>> could be used for both purposes (and should be usable both ways,
> >>>> imo).
> >>>
> >>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
> >>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
> >>> wraparound (DISP) + BASE + INDEX * SCALE.
> >>
> >> But this is true regardless of how small (or big) the displacement.
> >> Without knowing the register values, you can't know at what
> >> displacement values wraparound occurs. Also, unless I'm mistaken,
> >> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
> >
> > Hardware does wraparound (DISP + BASE + INDEX * SCALE).
> > Assembler and linker should only wraparound on the final address.
>
> I'm afraid this last sentence makes no sense to me: The assembler
> (or linker) can't know the final address. Instead, both immediates
> and displacements should allow for anything the programmer might
> sensibly use. If 0xffffffff as a displacement is fine (meaning
> -1 really), -0xffffffff (meaning 1) ought to be, too. Or else
> where do you draw the boundary of which displacements are
> "legitimate" and which are not?
>

If the program wants 1, use 1.  DISP treatment should stay as is.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 18.06.2021 16:12, H.J. Lu wrote:
> On Fri, Jun 18, 2021 at 2:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 17.06.2021 18:12, H.J. Lu wrote:
>>> On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 17.06.2021 18:00, H.J. Lu wrote:
>>>>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 17.06.2021 16:46, H.J. Lu wrote:
>>>>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
>>>>>>>> @@ -0,0 +1,17 @@
>>>>>>>> +       .text
>>>>>>>> +disp_imm:
>>>>>>>> +       mov     -0xffffffff(%eax), %eax
>>>>>>>
>>>>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
>>>>>>> We allow addresses to wraparound. I don't see a need for
>>>>>>> displacements to wraparound.
>>>>>>
>>>>>> This then is entirely unexpected to the programmer. In fact the
>>>>>> same (abstracted away behind some defines or equates) constant
>>>>>> could be used for both purposes (and should be usable both ways,
>>>>>> imo).
>>>>>
>>>>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
>>>>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
>>>>> wraparound (DISP) + BASE + INDEX * SCALE.
>>>>
>>>> But this is true regardless of how small (or big) the displacement.
>>>> Without knowing the register values, you can't know at what
>>>> displacement values wraparound occurs. Also, unless I'm mistaken,
>>>> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
>>>
>>> Hardware does wraparound (DISP + BASE + INDEX * SCALE).
>>> Assembler and linker should only wraparound on the final address.
>>
>> I'm afraid this last sentence makes no sense to me: The assembler
>> (or linker) can't know the final address. Instead, both immediates
>> and displacements should allow for anything the programmer might
>> sensibly use. If 0xffffffff as a displacement is fine (meaning
>> -1 really), -0xffffffff (meaning 1) ought to be, too. Or else
>> where do you draw the boundary of which displacements are
>> "legitimate" and which are not?
> 
> If the program wants 1, use 1.

You realize that for the purpose of the test case the -0xffffffff is
an over-simplification of what might appear in "real" code, don't
you? It also feels as if you didn't really read my earlier remark:

"In fact the same (abstracted away behind some defines or equates)
 constant could be used for both purposes (and should be usable
 both ways, imo)."

If what you say were true for disp, why would it not also apply to
imm? In the end, an imm loaded into a register may very well be
used to access memory, i.e. possibly fulfills the purpose of a disp.
We shouldn't force people to write quirky code just because gas
accepts certain things for imm which it doesn't accept for disp.

Jan

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Fri, Jun 18, 2021 at 7:52 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.06.2021 16:12, H.J. Lu wrote:
> > On Fri, Jun 18, 2021 at 2:03 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 17.06.2021 18:12, H.J. Lu wrote:
> >>> On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 17.06.2021 18:00, H.J. Lu wrote:
> >>>>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 17.06.2021 16:46, H.J. Lu wrote:
> >>>>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> >>>>>>>> @@ -0,0 +1,17 @@
> >>>>>>>> +       .text
> >>>>>>>> +disp_imm:
> >>>>>>>> +       mov     -0xffffffff(%eax), %eax
> >>>>>>>
> >>>>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
> >>>>>>> We allow addresses to wraparound. I don't see a need for
> >>>>>>> displacements to wraparound.
> >>>>>>
> >>>>>> This then is entirely unexpected to the programmer. In fact the
> >>>>>> same (abstracted away behind some defines or equates) constant
> >>>>>> could be used for both purposes (and should be usable both ways,
> >>>>>> imo).
> >>>>>
> >>>>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
> >>>>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
> >>>>> wraparound (DISP) + BASE + INDEX * SCALE.
> >>>>
> >>>> But this is true regardless of how small (or big) the displacement.
> >>>> Without knowing the register values, you can't know at what
> >>>> displacement values wraparound occurs. Also, unless I'm mistaken,
> >>>> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
> >>>
> >>> Hardware does wraparound (DISP + BASE + INDEX * SCALE).
> >>> Assembler and linker should only wraparound on the final address.
> >>
> >> I'm afraid this last sentence makes no sense to me: The assembler
> >> (or linker) can't know the final address. Instead, both immediates
> >> and displacements should allow for anything the programmer might
> >> sensibly use. If 0xffffffff as a displacement is fine (meaning
> >> -1 really), -0xffffffff (meaning 1) ought to be, too. Or else
> >> where do you draw the boundary of which displacements are
> >> "legitimate" and which are not?
> >
> > If the program wants 1, use 1.
>
> You realize that for the purpose of the test case the -0xffffffff is
> an over-simplification of what might appear in "real" code, don't
> you? It also feels as if you didn't really read my earlier remark:

I don't think treating -0xffffffff as 1 here helps anyone.

> "In fact the same (abstracted away behind some defines or equates)
>  constant could be used for both purposes (and should be usable
>  both ways, imo)."
>
> If what you say were true for disp, why would it not also apply to
> imm? In the end, an imm loaded into a register may very well be
> used to access memory, i.e. possibly fulfills the purpose of a disp.
> We shouldn't force people to write quirky code just because gas
> accepts certain things for imm which it doesn't accept for disp.
>

Loading imm into register is similar to wraparound in C.  DISP
is different.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 18.06.2021 17:41, H.J. Lu wrote:
> On Fri, Jun 18, 2021 at 7:52 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 18.06.2021 16:12, H.J. Lu wrote:
>>> On Fri, Jun 18, 2021 at 2:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 17.06.2021 18:12, H.J. Lu wrote:
>>>>> On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 17.06.2021 18:00, H.J. Lu wrote:
>>>>>>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 17.06.2021 16:46, H.J. Lu wrote:
>>>>>>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
>>>>>>>>>> @@ -0,0 +1,17 @@
>>>>>>>>>> +       .text
>>>>>>>>>> +disp_imm:
>>>>>>>>>> +       mov     -0xffffffff(%eax), %eax
>>>>>>>>>
>>>>>>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
>>>>>>>>> We allow addresses to wraparound. I don't see a need for
>>>>>>>>> displacements to wraparound.
>>>>>>>>
>>>>>>>> This then is entirely unexpected to the programmer. In fact the
>>>>>>>> same (abstracted away behind some defines or equates) constant
>>>>>>>> could be used for both purposes (and should be usable both ways,
>>>>>>>> imo).
>>>>>>>
>>>>>>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
>>>>>>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
>>>>>>> wraparound (DISP) + BASE + INDEX * SCALE.
>>>>>>
>>>>>> But this is true regardless of how small (or big) the displacement.
>>>>>> Without knowing the register values, you can't know at what
>>>>>> displacement values wraparound occurs. Also, unless I'm mistaken,
>>>>>> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
>>>>>
>>>>> Hardware does wraparound (DISP + BASE + INDEX * SCALE).
>>>>> Assembler and linker should only wraparound on the final address.
>>>>
>>>> I'm afraid this last sentence makes no sense to me: The assembler
>>>> (or linker) can't know the final address. Instead, both immediates
>>>> and displacements should allow for anything the programmer might
>>>> sensibly use. If 0xffffffff as a displacement is fine (meaning
>>>> -1 really), -0xffffffff (meaning 1) ought to be, too. Or else
>>>> where do you draw the boundary of which displacements are
>>>> "legitimate" and which are not?
>>>
>>> If the program wants 1, use 1.
>>
>> You realize that for the purpose of the test case the -0xffffffff is
>> an over-simplification of what might appear in "real" code, don't
>> you? It also feels as if you didn't really read my earlier remark:
> 
> I don't think treating -0xffffffff as 1 here helps anyone.

I'm afraid I was utterly mislead by your commentary here. There's no
change from this patch in the numeric meaning of -0xffffffff. The
change here is what warnings result and what encodings get used. As
the description says:

'Certain disp values may trigger "... shortened to ..." warnings when
 equivalent imm ones don't. In some of the cases there are also
 differences (for non-64-bit code) between BFD64 and !BFD64 builds. The
 resulting encodings (i.e. use [or not] of the shorter disp8 / imm8
 forms) are also different in some cases. Make this handling consistent.'

Please may I ask that you take the new testcase and observe the
difference in behavior with / without BFD64 prior to this patch, and
the now consistent one?

Jan

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Sun, Jun 20, 2021 at 11:36 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.06.2021 17:41, H.J. Lu wrote:
> > On Fri, Jun 18, 2021 at 7:52 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 18.06.2021 16:12, H.J. Lu wrote:
> >>> On Fri, Jun 18, 2021 at 2:03 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 17.06.2021 18:12, H.J. Lu wrote:
> >>>>> On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 17.06.2021 18:00, H.J. Lu wrote:
> >>>>>>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>
> >>>>>>>> On 17.06.2021 16:46, H.J. Lu wrote:
> >>>>>>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>> --- /dev/null
> >>>>>>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> >>>>>>>>>> @@ -0,0 +1,17 @@
> >>>>>>>>>> +       .text
> >>>>>>>>>> +disp_imm:
> >>>>>>>>>> +       mov     -0xffffffff(%eax), %eax
> >>>>>>>>>
> >>>>>>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
> >>>>>>>>> We allow addresses to wraparound. I don't see a need for
> >>>>>>>>> displacements to wraparound.
> >>>>>>>>
> >>>>>>>> This then is entirely unexpected to the programmer. In fact the
> >>>>>>>> same (abstracted away behind some defines or equates) constant
> >>>>>>>> could be used for both purposes (and should be usable both ways,
> >>>>>>>> imo).
> >>>>>>>
> >>>>>>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
> >>>>>>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
> >>>>>>> wraparound (DISP) + BASE + INDEX * SCALE.
> >>>>>>
> >>>>>> But this is true regardless of how small (or big) the displacement.
> >>>>>> Without knowing the register values, you can't know at what
> >>>>>> displacement values wraparound occurs. Also, unless I'm mistaken,
> >>>>>> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
> >>>>>
> >>>>> Hardware does wraparound (DISP + BASE + INDEX * SCALE).
> >>>>> Assembler and linker should only wraparound on the final address.
> >>>>
> >>>> I'm afraid this last sentence makes no sense to me: The assembler
> >>>> (or linker) can't know the final address. Instead, both immediates
> >>>> and displacements should allow for anything the programmer might
> >>>> sensibly use. If 0xffffffff as a displacement is fine (meaning
> >>>> -1 really), -0xffffffff (meaning 1) ought to be, too. Or else
> >>>> where do you draw the boundary of which displacements are
> >>>> "legitimate" and which are not?
> >>>
> >>> If the program wants 1, use 1.
> >>
> >> You realize that for the purpose of the test case the -0xffffffff is
> >> an over-simplification of what might appear in "real" code, don't
> >> you? It also feels as if you didn't really read my earlier remark:
> >
> > I don't think treating -0xffffffff as 1 here helps anyone.
>
> I'm afraid I was utterly mislead by your commentary here. There's no
> change from this patch in the numeric meaning of -0xffffffff. The
> change here is what warnings result and what encodings get used. As
> the description says:
>
> 'Certain disp values may trigger "... shortened to ..." warnings when
>  equivalent imm ones don't. In some of the cases there are also
>  differences (for non-64-bit code) between BFD64 and !BFD64 builds. The
>  resulting encodings (i.e. use [or not] of the shorter disp8 / imm8
>  forms) are also different in some cases. Make this handling consistent.'
>
> Please may I ask that you take the new testcase and observe the
> difference in behavior with / without BFD64 prior to this patch, and
> the now consistent one?

Make disp consistent is good.  But wraparound disp is not.   Please
find another way to make disp consistent.

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 21.06.2021 15:26, H.J. Lu wrote:
> On Sun, Jun 20, 2021 at 11:36 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 18.06.2021 17:41, H.J. Lu wrote:
>>> On Fri, Jun 18, 2021 at 7:52 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 18.06.2021 16:12, H.J. Lu wrote:
>>>>> On Fri, Jun 18, 2021 at 2:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 17.06.2021 18:12, H.J. Lu wrote:
>>>>>>> On Thu, Jun 17, 2021 at 9:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 17.06.2021 18:00, H.J. Lu wrote:
>>>>>>>>> On Thu, Jun 17, 2021 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 17.06.2021 16:46, H.J. Lu wrote:
>>>>>>>>>>> On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
>>>>>>>>>>>> @@ -0,0 +1,17 @@
>>>>>>>>>>>> +       .text
>>>>>>>>>>>> +disp_imm:
>>>>>>>>>>>> +       mov     -0xffffffff(%eax), %eax
>>>>>>>>>>>
>>>>>>>>>>> I don't think we should treat  -0xffffffff(%eax) as 1(%eax).
>>>>>>>>>>> We allow addresses to wraparound. I don't see a need for
>>>>>>>>>>> displacements to wraparound.
>>>>>>>>>>
>>>>>>>>>> This then is entirely unexpected to the programmer. In fact the
>>>>>>>>>> same (abstracted away behind some defines or equates) constant
>>>>>>>>>> could be used for both purposes (and should be usable both ways,
>>>>>>>>>> imo).
>>>>>>>>>
>>>>>>>>> Since hardware wraparound on DISP + BASE + INDEX * SCALE, not
>>>>>>>>> on DISP, it is wrong to change DISP + BASE + INDEX * SCALE to
>>>>>>>>> wraparound (DISP) + BASE + INDEX * SCALE.
>>>>>>>>
>>>>>>>> But this is true regardless of how small (or big) the displacement.
>>>>>>>> Without knowing the register values, you can't know at what
>>>>>>>> displacement values wraparound occurs. Also, unless I'm mistaken,
>>>>>>>> wrapround(a + b) == wrapround(wrapround(a) + wrapround(b)).
>>>>>>>
>>>>>>> Hardware does wraparound (DISP + BASE + INDEX * SCALE).
>>>>>>> Assembler and linker should only wraparound on the final address.
>>>>>>
>>>>>> I'm afraid this last sentence makes no sense to me: The assembler
>>>>>> (or linker) can't know the final address. Instead, both immediates
>>>>>> and displacements should allow for anything the programmer might
>>>>>> sensibly use. If 0xffffffff as a displacement is fine (meaning
>>>>>> -1 really), -0xffffffff (meaning 1) ought to be, too. Or else
>>>>>> where do you draw the boundary of which displacements are
>>>>>> "legitimate" and which are not?
>>>>>
>>>>> If the program wants 1, use 1.
>>>>
>>>> You realize that for the purpose of the test case the -0xffffffff is
>>>> an over-simplification of what might appear in "real" code, don't
>>>> you? It also feels as if you didn't really read my earlier remark:
>>>
>>> I don't think treating -0xffffffff as 1 here helps anyone.
>>
>> I'm afraid I was utterly mislead by your commentary here. There's no
>> change from this patch in the numeric meaning of -0xffffffff. The
>> change here is what warnings result and what encodings get used. As
>> the description says:
>>
>> 'Certain disp values may trigger "... shortened to ..." warnings when
>>  equivalent imm ones don't. In some of the cases there are also
>>  differences (for non-64-bit code) between BFD64 and !BFD64 builds. The
>>  resulting encodings (i.e. use [or not] of the shorter disp8 / imm8
>>  forms) are also different in some cases. Make this handling consistent.'
>>
>> Please may I ask that you take the new testcase and observe the
>> difference in behavior with / without BFD64 prior to this patch, and
>> the now consistent one?
> 
> Make disp consistent is good.  But wraparound disp is not.

You still didn't understand, it seems: This wrapping was there before.
All I did is make it consistently silent, and generate consistent code.

>   Please find another way to make disp consistent.

I don't see any.

Jan

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Michael Matz]

Hello,

On Fri, 18 Jun 2021, H.J. Lu via Binutils wrote:

> > You realize that for the purpose of the test case the -0xffffffff is
> > an over-simplification of what might appear in "real" code, don't
> > you? It also feels as if you didn't really read my earlier remark:
> 
> I don't think treating -0xffffffff as 1 here helps anyone.

The assembler should not be too much in the business of constraining users 
with well-intended but badly implemented rules.  The -0xffffffff isn't 
actually a literal, it's an (assembler) operation applied to a literal.  
The wraparound that does or doesn't happen is _internal_ to the assembler, 
the hardware doesn't enter the picture.  -0xffffffff only means '1' 
because the assembler internally represents this as 32bit entity, again 
without the hardware mattering.  (And if it were using a different-sized 
internal field it would or wouldn't warn on different input ranges, and if 
that size then depends on how BFD is configured that would be very bad).

Further it's quite probable that the '-' actually comes from e.g. a macro 
expansion, it's for instance feasible to write a macro that offsets 
something by a constant in both directions:

#define sub2(reg, B, D) \
  movq D(B), reg \
  addq -D(B), reg

Now it's completely opaque why the user should be warned about this:

  sub2(%rax, %rdi, 0xffffffff)

but not about this:

  sub2(%rax, %rdi, 1)

What you are saying is equivalent to say to also warn about uses of '--1', 
after all the user "should have simply used "1"'.  Extended such makes it 
obvious that the warning is unwarranted, because then there wouldn't even 
be a work-around for the above situation anymore, you can't write 
sub2(%rax,%rdi,0xffffffff) and you can't write sub2(%rax,%rdi,-1).

I would perhaps agree that if a user literally would write '-0xffffffff' 
except in a testcase that a warning might be appropriate, in something 
that's designed to teach good style to assembler programmers perhaps.  But 
not in GNU as.  It would be a warning that can't be silenced and happens 
in perfectly valid code, so, no.

All in all: I think the assembler should be entirely silent about any of 
these literals modified by assembler operations.


Ciao,
Michael.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[H.J. Lu]

On Tue, Jun 22, 2021 at 6:22 AM Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Fri, 18 Jun 2021, H.J. Lu via Binutils wrote:
>
> > > You realize that for the purpose of the test case the -0xffffffff is
> > > an over-simplification of what might appear in "real" code, don't
> > > you? It also feels as if you didn't really read my earlier remark:
> >
> > I don't think treating -0xffffffff as 1 here helps anyone.
>
> The assembler should not be too much in the business of constraining users
> with well-intended but badly implemented rules.  The -0xffffffff isn't
> actually a literal, it's an (assembler) operation applied to a literal.
> The wraparound that does or doesn't happen is _internal_ to the assembler,
> the hardware doesn't enter the picture.  -0xffffffff only means '1'
> because the assembler internally represents this as 32bit entity, again
> without the hardware mattering.  (And if it were using a different-sized
> internal field it would or wouldn't warn on different input ranges, and if
> that size then depends on how BFD is configured that would be very bad).
>
> Further it's quite probable that the '-' actually comes from e.g. a macro
> expansion, it's for instance feasible to write a macro that offsets
> something by a constant in both directions:
>
> #define sub2(reg, B, D) \
>   movq D(B), reg \
>   addq -D(B), reg
>
> Now it's completely opaque why the user should be warned about this:
>
>   sub2(%rax, %rdi, 0xffffffff)
>
> but not about this:
>
>   sub2(%rax, %rdi, 1)
>
> What you are saying is equivalent to say to also warn about uses of '--1',
> after all the user "should have simply used "1"'.  Extended such makes it
> obvious that the warning is unwarranted, because then there wouldn't even
> be a work-around for the above situation anymore, you can't write
> sub2(%rax,%rdi,0xffffffff) and you can't write sub2(%rax,%rdi,-1).
>
> I would perhaps agree that if a user literally would write '-0xffffffff'
> except in a testcase that a warning might be appropriate, in something
> that's designed to teach good style to assembler programmers perhaps.  But
> not in GNU as.  It would be a warning that can't be silenced and happens
> in perfectly valid code, so, no.
>
> All in all: I think the assembler should be entirely silent about any of
> these literals modified by assembler operations.

A 32bit displacement is signed with R_X86_64_32S.  For

mov disp(%rax), %eax

should linker issue an error if disp == -0xffffffff?

-- 
H.J.

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Jan Beulich]

On 22.06.2021 16:01, H.J. Lu wrote:
> On Tue, Jun 22, 2021 at 6:22 AM Michael Matz <matz@suse.de> wrote:
>>
>> Hello,
>>
>> On Fri, 18 Jun 2021, H.J. Lu via Binutils wrote:
>>
>>>> You realize that for the purpose of the test case the -0xffffffff is
>>>> an over-simplification of what might appear in "real" code, don't
>>>> you? It also feels as if you didn't really read my earlier remark:
>>>
>>> I don't think treating -0xffffffff as 1 here helps anyone.
>>
>> The assembler should not be too much in the business of constraining users
>> with well-intended but badly implemented rules.  The -0xffffffff isn't
>> actually a literal, it's an (assembler) operation applied to a literal.
>> The wraparound that does or doesn't happen is _internal_ to the assembler,
>> the hardware doesn't enter the picture.  -0xffffffff only means '1'
>> because the assembler internally represents this as 32bit entity, again
>> without the hardware mattering.  (And if it were using a different-sized
>> internal field it would or wouldn't warn on different input ranges, and if
>> that size then depends on how BFD is configured that would be very bad).
>>
>> Further it's quite probable that the '-' actually comes from e.g. a macro
>> expansion, it's for instance feasible to write a macro that offsets
>> something by a constant in both directions:
>>
>> #define sub2(reg, B, D) \
>>   movq D(B), reg \
>>   addq -D(B), reg
>>
>> Now it's completely opaque why the user should be warned about this:
>>
>>   sub2(%rax, %rdi, 0xffffffff)
>>
>> but not about this:
>>
>>   sub2(%rax, %rdi, 1)
>>
>> What you are saying is equivalent to say to also warn about uses of '--1',
>> after all the user "should have simply used "1"'.  Extended such makes it
>> obvious that the warning is unwarranted, because then there wouldn't even
>> be a work-around for the above situation anymore, you can't write
>> sub2(%rax,%rdi,0xffffffff) and you can't write sub2(%rax,%rdi,-1).
>>
>> I would perhaps agree that if a user literally would write '-0xffffffff'
>> except in a testcase that a warning might be appropriate, in something
>> that's designed to teach good style to assembler programmers perhaps.  But
>> not in GNU as.  It would be a warning that can't be silenced and happens
>> in perfectly valid code, so, no.
>>
>> All in all: I think the assembler should be entirely silent about any of
>> these literals modified by assembler operations.
> 
> A 32bit displacement is signed with R_X86_64_32S.  For
> 
> mov disp(%rax), %eax
> 
> should linker issue an error if disp == -0xffffffff?

I think so, yes. In fact I'd hope the assembler would already not allow
this, -0xffffffff actually being 0xffffffff00000001 in 64-bit arithmetic.
What we've been discussing here so far were issues with 32-bit addressing
(and wrapping of involved arithmetic).

Jan

Re: [PATCH 3/6] x86: harmonize disp with imm handling

[Michael Matz]

Hello,

On Tue, 22 Jun 2021, H.J. Lu wrote:

> > All in all: I think the assembler should be entirely silent about any of
> > these literals modified by assembler operations.
> 
> A 32bit displacement is signed with R_X86_64_32S.  For
> 
> mov disp(%rax), %eax
> 
> should linker issue an error if disp == -0xffffffff?

The linker doesn't see '-0xffffffff', it sees a bit pattern that it 
interprets in some way as a single number.  When we write down the bit 
pattern as unsigned hex, it will for instance see (in the above case when 
the width is 32bit and the assembler did the obvious thing) '1' (i.e. 
something that the linker can't differentiate from what was produced by 
the assembler on a simple literal '1' input).  There is no unary minus 
operation on linker input.


Ciao,
Michael.