[PATCH 0/2] x86: .insn and diagnostic fixes

[PATCH 0/2] x86: .insn and diagnostic fixes

[Jan Beulich]

Having spotted that {rex} is ignored with .insn, fixing this has
uncovered further issues.

1: properly respect rex/{rex}
2: refuse "high" 8-bit regs with .insn and VEX/XOP/EVEX encodings

Jan

[PATCH 1/2] x86: properly respect rex/{rex}

[Jan Beulich]

This addresses two issues: For one, a user specified "rex" did not cause
the diagnostic to trigger when there was no other need for a REX prefix;
instead, another than the specified insn+operands was encoded. And then
(which is what this started from) .insn didn't respect {rex} (and was
otherwise similarly flawed as ordinary insns).

The latter requires splitting out code from md_assemble(), for it to
become re-usable. Besides the addition to address the first issue, that
code then also needs generalizing to account for immediate operands, as
with .insn we can't make assumptions anymore on all respective templates
having at most two operands (we still can build upon there being at most
two non-immediate operands, though). While moving the code also simplify
the first if(), by folding redundant checks.

In the new testcase also test a few more things which afaics weren't
tested till now.
---
It's not at all obvious to me why the existing "rex" testcase is
excluded for so many targets. It can't really be the use of / as prefix
separator, as that could easily be covered by passing --divide to gas.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -4123,6 +4123,74 @@ build_evex_prefix (void)
     i.vex.bytes[3] |= i.mask.reg->reg_num;
 }
 
+static void establish_rex (void)
+{
+  /* Note that legacy encodings have at most 2 non-immediate operands.  */
+  unsigned int first = i.imm_operands;
+  unsigned int last = i.operands > first ? i.operands - first - 1 : first;
+
+  /* Respect a user-specified REX prefix.  */
+  i.rex |= i.prefix[REX_PREFIX] & REX_OPCODE;
+
+  /* For 8 bit registers we need an empty rex prefix.  Also if the
+     instruction already has a prefix, we need to convert old
+     registers to new ones.  */
+
+  if ((i.types[first].bitfield.class == Reg && i.types[first].bitfield.byte
+       && ((i.op[first].regs->reg_flags & RegRex64) != 0 || i.rex != 0))
+      || (i.types[last].bitfield.class == Reg && i.types[last].bitfield.byte
+	  && ((i.op[last].regs->reg_flags & RegRex64) != 0 || i.rex != 0)))
+    {
+      unsigned int x;
+
+      i.rex |= REX_OPCODE;
+      for (x = first; x <= last; x++)
+	{
+	  /* Look for 8 bit operand that uses old registers.  */
+	  if (i.types[x].bitfield.class == Reg && i.types[x].bitfield.byte
+	      && (i.op[x].regs->reg_flags & RegRex64) == 0)
+	    {
+	      gas_assert (!(i.op[x].regs->reg_flags & RegRex));
+	      /* In case it is "hi" register, give up.  */
+	      if (i.op[x].regs->reg_num > 3)
+		as_bad (_("can't encode register '%s%s' in an "
+			  "instruction requiring REX prefix"),
+			register_prefix, i.op[x].regs->reg_name);
+
+	      /* Otherwise it is equivalent to the extended register.
+		 Since the encoding doesn't change this is merely
+		 cosmetic cleanup for debug output.  */
+	      i.op[x].regs += 8;
+	    }
+	}
+    }
+
+  if (i.rex == 0 && i.rex_encoding)
+    {
+      /* Check if we can add a REX_OPCODE byte.  Look for 8 bit operand
+	 that uses legacy register.  If it is "hi" register, don't add
+	 the REX_OPCODE byte.  */
+      unsigned int x;
+
+      for (x = first; x <= last; x++)
+	if (i.types[x].bitfield.class == Reg
+	    && i.types[x].bitfield.byte
+	    && (i.op[x].regs->reg_flags & RegRex64) == 0
+	    && i.op[x].regs->reg_num > 3)
+	  {
+	    gas_assert (!(i.op[x].regs->reg_flags & RegRex));
+	    i.rex_encoding = false;
+	    break;
+	  }
+
+      if (i.rex_encoding)
+	i.rex = REX_OPCODE;
+    }
+
+  if (i.rex != 0)
+    add_prefix (REX_OPCODE | i.rex);
+}
+
 static void
 process_immext (void)
 {
@@ -5620,66 +5688,7 @@ md_assemble (char *line)
       i.op[0].disps->X_op = O_symbol;
     }
 
-  /* For 8 bit registers we need an empty rex prefix.  Also if the
-     instruction already has a prefix, we need to convert old
-     registers to new ones.  */
-
-  if ((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
-       && (i.op[0].regs->reg_flags & RegRex64) != 0)
-      || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
-	  && (i.op[1].regs->reg_flags & RegRex64) != 0)
-      || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
-	   || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
-	  && i.rex != 0))
-    {
-      int x;
-
-      i.rex |= REX_OPCODE;
-      for (x = 0; x < 2; x++)
-	{
-	  /* Look for 8 bit operand that uses old registers.  */
-	  if (i.types[x].bitfield.class == Reg && i.types[x].bitfield.byte
-	      && (i.op[x].regs->reg_flags & RegRex64) == 0)
-	    {
-	      gas_assert (!(i.op[x].regs->reg_flags & RegRex));
-	      /* In case it is "hi" register, give up.  */
-	      if (i.op[x].regs->reg_num > 3)
-		as_bad (_("can't encode register '%s%s' in an "
-			  "instruction requiring REX prefix."),
-			register_prefix, i.op[x].regs->reg_name);
-
-	      /* Otherwise it is equivalent to the extended register.
-		 Since the encoding doesn't change this is merely
-		 cosmetic cleanup for debug output.  */
-
-	      i.op[x].regs = i.op[x].regs + 8;
-	    }
-	}
-    }
-
-  if (i.rex == 0 && i.rex_encoding)
-    {
-      /* Check if we can add a REX_OPCODE byte.  Look for 8 bit operand
-	 that uses legacy register.  If it is "hi" register, don't add
-	 the REX_OPCODE byte.  */
-      int x;
-      for (x = 0; x < 2; x++)
-	if (i.types[x].bitfield.class == Reg
-	    && i.types[x].bitfield.byte
-	    && (i.op[x].regs->reg_flags & RegRex64) == 0
-	    && i.op[x].regs->reg_num > 3)
-	  {
-	    gas_assert (!(i.op[x].regs->reg_flags & RegRex));
-	    i.rex_encoding = false;
-	    break;
-	  }
-
-      if (i.rex_encoding)
-	i.rex = REX_OPCODE;
-    }
-
-  if (i.rex != 0)
-    add_prefix (REX_OPCODE | i.rex);
+  establish_rex ();
 
   insert_lfence_before (last_insn);
 
@@ -11697,8 +11706,8 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
       build_evex_prefix ();
       i.rex &= REX_OPCODE;
     }
-  else if (i.rex != 0)
-    add_prefix (REX_OPCODE | i.rex);
+  else
+    establish_rex ();
 
   last_insn = &seg_info(now_seg)->tc_segment_info_data.last_insn;
   output_insn (last_insn);
--- /dev/null
+++ b/gas/testsuite/gas/i386/rex-bad.l
@@ -0,0 +1,10 @@
+.*: Assembler messages:
+.*:4: Error: same .*
+.*:5: Error: same .*
+.*:6: Error: same .*
+.*:7: Error: same .*
+.*:9: Error: .* REX .*
+.*:10: Error: .* REX .*
+.*:12: Error: .* REX .*
+.*:13: Error: .* REX .*
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/rex-bad.s
@@ -0,0 +1,13 @@
+	.text
+
+_start:
+	rex.w add (%rax,%rax), %rax
+	rex.r add (%rax,%rax), %r8
+	rex.b add (%r8,%rax), %eax
+	rex.x add (%rax,%r8), %eax
+
+	rex mov %al, %ch
+	rex mov %ah, %cl
+
+	.insn rex 0x88, %al, %ch
+	.insn rex 0x88, %ah, %cl
--- a/gas/testsuite/gas/i386/x86-64-pseudos.d
+++ b/gas/testsuite/gas/i386/x86-64-pseudos.d
@@ -470,4 +470,5 @@ Disassembly of section .text:
  +[a-f0-9]+:	41 8a 45 00          	mov    0x0\(%r13\),%al
  +[a-f0-9]+:	67 41 8a 45 00       	mov    0x0\(%r13d\),%al
  +[a-f0-9]+:	67 41 8a 85 00 00 00 00 	mov    0x0\(%r13d\),%al
+ +[a-f0-9]+:	40 8a c1             	rex mov %cl,%al
 #pass
--- a/gas/testsuite/gas/i386/x86-64-pseudos.s
+++ b/gas/testsuite/gas/i386/x86-64-pseudos.s
@@ -438,3 +438,5 @@ _start:
 	mov al, BYTE PTR [r13]
 	{disp8} mov al, BYTE PTR [r13d]
 	{disp32} mov al, BYTE PTR [r13d]
+
+	.insn {rex} 0x8a, al, cl
--- a/gas/testsuite/gas/i386/x86-64.exp
+++ b/gas/testsuite/gas/i386/x86-64.exp
@@ -596,6 +596,7 @@ if { ![istarget "*-*-aix*"]
      && ![istarget "*-*-solaris*"]
      && ![istarget "*-*-sysv*"] } then {
     run_dump_test "rex"
+    run_list_test "rex-bad"
 }
 
 # ELF specific tests

[PATCH 2/2] x86-64: refuse "high" 8-bit regs with .insn and VEX/XOP/EVEX encodings

[Jan Beulich]

Much like REX, those encodings - if permitting 8-bit regs at all, i.e.
only starting with APX - permit use of "new" 8-bit registers only. %ah,
%ch, %dh, and %bh cannot be encoded and hence should be rejected.

Permit their use outside of 64-bit code though, as "new" registers
simply don't exist there.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -11484,6 +11484,16 @@ s_insn (int dummy ATTRIBUTE_UNUSED)
 
       for (j = i.imm_operands; j < i.operands; ++j)
 	{
+	  /* Look for 8-bit operands that use old registers.  */
+	  if (i.vec_encoding != vex_encoding_default
+	      && flag_code == CODE_64BIT
+	      && i.types[j].bitfield.class == Reg
+	      && i.types[j].bitfield.byte
+	      && !(i.op[j].regs->reg_flags & RegRex64)
+	      && i.op[j].regs->reg_num > 3)
+	    as_bad (_("can't encode register '%s%s' with VEX/XOP/EVEX"),
+		    register_prefix, i.op[j].regs->reg_name);
+
 	  i.types[j].bitfield.instance = InstanceNone;
 
 	  if (operand_type_check (i.types[j], disp))