[PATCH 0/6] gas/x86: adjustments to register handling

[PATCH 0/6] gas/x86: adjustments to register handling

[Jan Beulich]

I've noticed a couple of bugs or other anomalies, and the patches
below are the outcome of trying to rectify these.

1: restrict use of register aliases
2: also allow %st(N) in CFI directives
3: enable 2nd CFI test
4: restrict %tr<N> visibility
5: simplify check_byte_reg()
6: also handle %k<N> and %bnd<N> in debugging helpers

Jan

[PATCH 1/6] x86: restrict use of register aliases

[Jan Beulich]

Register aliases (created e.g. via .set) check their target register at
the time of creation of the alias. While this makes sense, it's not
enough: The underlying register must also be "visible" at the time of
use. Wrong use of such aliases would lead to internal errors in e.g.
add_prefix() or build_modrm_byte().

Split the checking part of parse_real_register() into a new helper
function and use it also from the latter part of parse_register() (at
the same time replacing a minor open coded part of it).

Since parse_register() returning NULL already has a meaning, a fake new
"bad register" indicator gets added, which all callers need to check
for.

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

	* config/tc-i386.c (bad_reg): New.
	(check_VecOperations, i386_att_operand, i386_parse_name): Check
	for it.
	(check_register): New, broken out from ...
	(parse_real_register): ... here. Call it.
	(parse_register): Call it, and error upon failure.
	* testsuite/gas/i386/equ-bad.s, testsuite/gas/i386/equ-bad.l,
	testsuite/gas/i386/x86-64-equ-bad.s,
	testsuite/gas/i386/x86-64-equ-bad.l: New.
	* testsuite/gas/i386/i386.exp: Run new tests:

opcodes/
2020-06-XX  Jan Beulich  <jbeulich@suse.com>

	* i386-opc.h (reg_entry): Const-qualify reg_name field.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -210,6 +210,10 @@ static unsigned int x86_used_note = DEFA
 
 static const char *default_arch = DEFAULT_ARCH;
 
+/* parse_register() returns this when a register alias cannot be used.  */
+static const reg_entry bad_reg = { "<bad>", OPERAND_TYPE_NONE, 0, 0,
+				   { Dw2Inval, Dw2Inval } };
+
 /* This struct describes rounding control and SAE in the instruction.  */
 struct RC_Operation
 {
@@ -10176,6 +10180,9 @@ check_VecOperations (char *op_string, ch
 	  /* Check masking operation.  */
 	  else if ((mask = parse_register (op_string, &end_op)) != NULL)
 	    {
+	      if (mask == &bad_reg)
+		return NULL;
+
 	      /* k0 can't be used for write mask.  */
 	      if (mask->reg_type.bitfield.class != RegMask || !mask->reg_num)
 		{
@@ -11035,6 +11042,9 @@ i386_att_operand (char *operand_string)
     {
       i386_operand_type temp;
 
+      if (r == &bad_reg)
+	return 0;
+
       /* Check for a segment override by searching for ':' after a
 	 segment register.  */
       op_string = end_op;
@@ -11211,6 +11221,8 @@ i386_att_operand (char *operand_string)
 
 	      if (i.base_reg)
 		{
+		  if (i.base_reg == &bad_reg)
+		    return 0;
 		  base_string = end_op;
 		  if (is_space_char (*base_string))
 		    ++base_string;
@@ -11226,6 +11238,8 @@ i386_att_operand (char *operand_string)
 		  if ((i.index_reg = parse_register (base_string, &end_op))
 		      != NULL)
 		    {
+		      if (i.index_reg == &bad_reg)
+			return 0;
 		      base_string = end_op;
 		      if (is_space_char (*base_string))
 			++base_string;
@@ -12331,6 +12345,73 @@ output_invalid (int c)
   return output_invalid_buf;
 }
 
+/* Verify that @r can be used in the current context.  */
+
+static bfd_boolean check_register (const reg_entry *r)
+{
+  if (allow_pseudo_reg)
+    return TRUE;
+
+  if (operand_type_all_zero (&r->reg_type))
+    return FALSE;
+
+  if ((r->reg_type.bitfield.dword
+       || (r->reg_type.bitfield.class == SReg && r->reg_num > 3)
+       || r->reg_type.bitfield.class == RegCR
+       || r->reg_type.bitfield.class == RegDR
+       || r->reg_type.bitfield.class == RegTR)
+      && !cpu_arch_flags.bitfield.cpui386)
+    return FALSE;
+
+  if (r->reg_type.bitfield.class == RegMMX && !cpu_arch_flags.bitfield.cpummx)
+    return FALSE;
+
+  if (!cpu_arch_flags.bitfield.cpuavx512f)
+    {
+      if (r->reg_type.bitfield.zmmword
+	  || r->reg_type.bitfield.class == RegMask)
+	return FALSE;
+
+      if (!cpu_arch_flags.bitfield.cpuavx)
+	{
+	  if (r->reg_type.bitfield.ymmword)
+	    return FALSE;
+
+	  if (!cpu_arch_flags.bitfield.cpusse && r->reg_type.bitfield.xmmword)
+	    return FALSE;
+	}
+    }
+
+  if (r->reg_type.bitfield.class == RegBND && !cpu_arch_flags.bitfield.cpumpx)
+    return FALSE;
+
+  /* Don't allow fake index register unless allow_index_reg isn't 0. */
+  if (!allow_index_reg && r->reg_num == RegIZ)
+    return FALSE;
+
+  /* Upper 16 vector registers are only available with VREX in 64bit
+     mode, and require EVEX encoding.  */
+  if (r->reg_flags & RegVRex)
+    {
+      if (!cpu_arch_flags.bitfield.cpuavx512f
+	  || flag_code != CODE_64BIT)
+	return FALSE;
+
+      i.vec_encoding = vex_encoding_evex;
+    }
+
+  if (((r->reg_flags & (RegRex64 | RegRex)) || r->reg_type.bitfield.qword)
+      && (!cpu_arch_flags.bitfield.cpulm || r->reg_type.bitfield.class != RegCR)
+      && flag_code != CODE_64BIT)
+    return FALSE;
+
+  if (r->reg_type.bitfield.class == SReg && r->reg_num == RegFlat
+      && !intel_syntax)
+    return FALSE;
+
+  return TRUE;
+}
+
 /* REG_STRING starts *before* REGISTER_PREFIX.  */
 
 static const reg_entry *
@@ -12400,67 +12481,7 @@ parse_real_register (char *reg_string, c
 	}
     }
 
-  if (r == NULL || allow_pseudo_reg)
-    return r;
-
-  if (operand_type_all_zero (&r->reg_type))
-    return (const reg_entry *) NULL;
-
-  if ((r->reg_type.bitfield.dword
-       || (r->reg_type.bitfield.class == SReg && r->reg_num > 3)
-       || r->reg_type.bitfield.class == RegCR
-       || r->reg_type.bitfield.class == RegDR
-       || r->reg_type.bitfield.class == RegTR)
-      && !cpu_arch_flags.bitfield.cpui386)
-    return (const reg_entry *) NULL;
-
-  if (r->reg_type.bitfield.class == RegMMX && !cpu_arch_flags.bitfield.cpummx)
-    return (const reg_entry *) NULL;
-
-  if (!cpu_arch_flags.bitfield.cpuavx512f)
-    {
-      if (r->reg_type.bitfield.zmmword
-	  || r->reg_type.bitfield.class == RegMask)
-	return (const reg_entry *) NULL;
-
-      if (!cpu_arch_flags.bitfield.cpuavx)
-	{
-	  if (r->reg_type.bitfield.ymmword)
-	    return (const reg_entry *) NULL;
-
-	  if (!cpu_arch_flags.bitfield.cpusse && r->reg_type.bitfield.xmmword)
-	    return (const reg_entry *) NULL;
-	}
-    }
-
-  if (r->reg_type.bitfield.class == RegBND && !cpu_arch_flags.bitfield.cpumpx)
-    return (const reg_entry *) NULL;
-
-  /* Don't allow fake index register unless allow_index_reg isn't 0. */
-  if (!allow_index_reg && r->reg_num == RegIZ)
-    return (const reg_entry *) NULL;
-
-  /* Upper 16 vector registers are only available with VREX in 64bit
-     mode, and require EVEX encoding.  */
-  if (r->reg_flags & RegVRex)
-    {
-      if (!cpu_arch_flags.bitfield.cpuavx512f
-	  || flag_code != CODE_64BIT)
-	return (const reg_entry *) NULL;
-
-      i.vec_encoding = vex_encoding_evex;
-    }
-
-  if (((r->reg_flags & (RegRex64 | RegRex)) || r->reg_type.bitfield.qword)
-      && (!cpu_arch_flags.bitfield.cpulm || r->reg_type.bitfield.class != RegCR)
-      && flag_code != CODE_64BIT)
-    return (const reg_entry *) NULL;
-
-  if (r->reg_type.bitfield.class == SReg && r->reg_num == RegFlat
-      && !intel_syntax)
-    return (const reg_entry *) NULL;
-
-  return r;
+  return r && check_register (r) ? r : NULL;
 }
 
 /* REG_STRING starts *before* REGISTER_PREFIX.  */
@@ -12491,8 +12512,12 @@ parse_register (char *reg_string, char *
 	  know (e->X_add_number >= 0
 		&& (valueT) e->X_add_number < i386_regtab_size);
 	  r = i386_regtab + e->X_add_number;
-	  if ((r->reg_flags & RegVRex))
-	    i.vec_encoding = vex_encoding_evex;
+	  if (!check_register (r))
+	    {
+	      as_bad (_("register '%s%s' cannot be used here"),
+		      register_prefix, r->reg_name);
+	      r = &bad_reg;
+	    }
 	  *end_op = input_line_pointer;
 	}
       *input_line_pointer = c;
@@ -12513,8 +12538,13 @@ i386_parse_name (char *name, expressionS
     {
       *nextcharP = *input_line_pointer;
       *input_line_pointer = 0;
-      e->X_op = O_register;
-      e->X_add_number = r - i386_regtab;
+      if (r != &bad_reg)
+	{
+	  e->X_op = O_register;
+	  e->X_add_number = r - i386_regtab;
+	}
+      else
+	  e->X_op = O_illegal;
       return 1;
     }
   input_line_pointer = end;
--- /dev/null
+++ b/gas/testsuite/gas/i386/equ-bad.l
@@ -0,0 +1,3 @@
+.*: Assembler messages:
+.*:8: Error: .*%ebx.*
+.*:9: Error: .*%ebx.*
--- /dev/null
+++ b/gas/testsuite/gas/i386/equ-bad.s
@@ -0,0 +1,9 @@
+	.text
+	.arch generic32
+equ:
+	.set	xBX, %ebx
+
+	.code16
+	.arch i286
+	inc	xBX
+	incb	(xBX)
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -91,6 +91,7 @@ if [expr ([istarget "i*86-*-*"] ||  [ist
     run_list_test "suffix-bad"
     run_dump_test "immed32"
     run_dump_test "equ"
+    run_list_test "equ-bad"
     run_dump_test "divide"
     run_dump_test "padlock"
     run_dump_test "crx"
@@ -924,6 +925,7 @@ if [expr ([istarget "i*86-*-*"] || [ista
     run_dump_test "x86-64-prefetchwt1-intel"
     run_dump_test "x86-64-se1"
     run_dump_test "x86-64-equ"
+    run_list_test "x86-64-equ-bad"
     run_dump_test "x86-64-avx512f_vl-intel"
     run_dump_test "x86-64-avx512f_vl-opts-intel"
     run_dump_test "x86-64-avx512f_vl-opts"
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-equ-bad.l
@@ -0,0 +1,8 @@
+.*: Assembler messages:
+.*:11: Error: .*'%xmm18'.*
+.*:13: Error: .*'%dil'.*
+.*:14: Error: .*'%rdi'.*
+.*:15: Error: .*'%r8'.*
+.*:16: Error: .*'%r9d'.*
+.*:18: Error: .*'%r8'.*
+.*:19: Error: .*'%r9d'.*
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-equ-bad.s
@@ -0,0 +1,19 @@
+	.text
+	.code64
+equ:
+	.set R18, %xmm18
+	.set lDI, %dil
+	.set xDI, %rdi
+	.set x8, %r8
+	.set x9, %r9d
+
+	.code32
+	vmovaps %xmm0, R18
+
+	inc	lDI
+	incl	(xDI)
+	inc	x8
+	inc	x9
+
+	shlx	x8, x8, x8
+	shlx	x9, x9, x9
--- a/opcodes/i386-opc.h
+++ b/opcodes/i386-opc.h
@@ -906,7 +906,7 @@ extern const insn_template i386_optab[];
 /* these are for register name --> number & type hash lookup */
 typedef struct
 {
-  char *reg_name;
+  const char *reg_name;
   i386_operand_type reg_type;
   unsigned char reg_flags;
 #define RegRex	    0x1  /* Extended register.  */

[PATCH 2/6] x86: also allow %st(N) in CFI directives

[Jan Beulich]

In 0e0eea782025 ("x86: x87-related adjustments") I screwed up CFI
directives with FPU support disabled, by moving the conditional there
across a check of "allow_pseudo_reg". Add the missing check.

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

	* config/tc-i386.c (parse_real_register): Add allow_pseudo_reg
	check to %st(N) parsing logic.
	* testsuite/gas/cfi/cfi-i386.s: Set "generic32" arch.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -12452,7 +12452,8 @@ parse_real_register (char *reg_string, c
     {
       if (!cpu_arch_flags.bitfield.cpu8087
 	  && !cpu_arch_flags.bitfield.cpu287
-	  && !cpu_arch_flags.bitfield.cpu387)
+	  && !cpu_arch_flags.bitfield.cpu387
+	  && !allow_pseudo_reg)
 	return (const reg_entry *) NULL;
 
       if (is_space_char (*s))
--- a/gas/testsuite/gas/cfi/cfi-i386.s
+++ b/gas/testsuite/gas/cfi/cfi-i386.s
@@ -1,4 +1,5 @@
 	.text
+	.arch generic32
 
 #; func_locvars
 #; - function with a space on the stack 

[PATCH 4/6] x86: restrict %tr<N> visibility

[Jan Beulich]

First of all, these registers have never been available on any 64-bit
CPU, and hence should not be recognized in 64-bit mode. But even before
that they had already disappeared - also don't recognize them when 586
or 686 architectures were explicitly set.

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

	* config/tc-i386.c (check_register): Split RegTR handling, to
	fail recognition also in 64-bit mode as well as with i586 or
	i686 explicitly enabled.
	* testsuite/gas/i386/x86_64.s: Add insns referencing tr<N>.
	* testsuite/gas/i386/x86_64-intel.d,
	testsuite/gas/i386/x86_64.d: Adjust expectations.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -12358,11 +12358,17 @@ static bfd_boolean check_register (const
   if ((r->reg_type.bitfield.dword
        || (r->reg_type.bitfield.class == SReg && r->reg_num > 3)
        || r->reg_type.bitfield.class == RegCR
-       || r->reg_type.bitfield.class == RegDR
-       || r->reg_type.bitfield.class == RegTR)
+       || r->reg_type.bitfield.class == RegDR)
       && !cpu_arch_flags.bitfield.cpui386)
     return FALSE;
 
+  if (r->reg_type.bitfield.class == RegTR
+      && (flag_code == CODE_64BIT
+	  || !cpu_arch_flags.bitfield.cpui386
+	  || cpu_arch_isa_flags.bitfield.cpui586
+	  || cpu_arch_isa_flags.bitfield.cpui686))
+    return FALSE;
+
   if (r->reg_type.bitfield.class == RegMMX && !cpu_arch_flags.bitfield.cpummx)
     return FALSE;
 
--- a/gas/testsuite/gas/i386/x86_64-intel.d
+++ b/gas/testsuite/gas/i386/x86_64-intel.d
@@ -256,4 +256,6 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	48 0f b7 00          	movzx  rax,WORD PTR \[rax\]
 [ 	]*[a-f0-9]+:	0f c3 00             	movnti DWORD PTR \[rax\],eax
 [ 	]*[a-f0-9]+:	48 0f c3 00          	movnti QWORD PTR \[rax\],rax
+[ 	]*[a-f0-9]+:	8b 04 25 00 00 00 00 	mov    eax,DWORD PTR (ds:)?0x0
+[ 	]*[a-f0-9]+:	48 89 0c 25 00 00 00 00 	mov    QWORD PTR (ds:)?0x0,rcx
 #pass
--- a/gas/testsuite/gas/i386/x86_64.d
+++ b/gas/testsuite/gas/i386/x86_64.d
@@ -256,4 +256,6 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	48 0f b7 00          	movzwq \(%rax\),%rax
 [ 	]*[a-f0-9]+:	0f c3 00             	movnti %eax,\(%rax\)
 [ 	]*[a-f0-9]+:	48 0f c3 00          	movnti %rax,\(%rax\)
+[ 	]*[a-f0-9]+:	8b 04 25 00 00 00 00 	mov    0x0,%eax
+[ 	]*[a-f0-9]+:	48 89 0c 25 00 00 00 00 	mov    %rcx,0x0
 #pass
--- a/gas/testsuite/gas/i386/x86_64.s
+++ b/gas/testsuite/gas/i386/x86_64.s
@@ -307,3 +307,6 @@ movzx rax, WORD PTR [rax]
 
 movnti dword ptr [rax], eax
 movnti qword ptr [rax], rax
+
+mov eax, tr1
+mov tr0, rcx

[PATCH 5/6] x86: simplify check_byte_reg()

[Jan Beulich]

With the introduction of what right now is the very first conditional
in the function's loop (commit dc821c5f9ae5 ["x86: replace Reg8, Reg16,
Reg32, and Reg64"]), the last if() in the same loop has become
pointless - retain just its body.

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

	* config/tc-i386.c (check_byte_reg): Drop dead conditional
	around as_bad().

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -7132,21 +7132,10 @@ check_byte_reg (void)
 	continue;
 
       /* Any other register is bad.  */
-      if (i.types[op].bitfield.class == Reg
-	  || i.types[op].bitfield.class == RegMMX
-	  || i.types[op].bitfield.class == RegSIMD
-	  || i.types[op].bitfield.class == SReg
-	  || i.types[op].bitfield.class == RegCR
-	  || i.types[op].bitfield.class == RegDR
-	  || i.types[op].bitfield.class == RegTR)
-	{
-	  as_bad (_("`%s%s' not allowed with `%s%c'"),
-		  register_prefix,
-		  i.op[op].regs->reg_name,
-		  i.tm.name,
-		  i.suffix);
-	  return 0;
-	}
+      as_bad (_("`%s%s' not allowed with `%s%c'"),
+	      register_prefix, i.op[op].regs->reg_name,
+	      i.tm.name, i.suffix);
+      return 0;
     }
   return 1;
 }

[PATCH 6/6] x86: also handle %k<N> and %bnd<N> in debugging helpers

[Jan Beulich]

Adjustment of this function was missed when support for the respective
registers was added.

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

	* config/tc-i386.c (pi): Add checks for RegMask and RegBND.

---
I'd like to note that enabling of DEBUG386 in the source file breaks
gas/i386/{,x86-64-}align-branch-6. I would think test cases should work
correctly either way.
---
Seeing that "class" can be non-zero right now solely for registers, I
wonder whether it wouldn't be better to simply check for ClassNone here,
eliminating the risk of further register introduction to again miss
adjusting this function. Of course then the risk of such omission is
when a first non-register class would get introduced.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3198,10 +3198,12 @@ pi (const char *line, i386_insn *x)
       if (x->types[j].bitfield.class == Reg
 	  || x->types[j].bitfield.class == RegMMX
 	  || x->types[j].bitfield.class == RegSIMD
+	  || x->types[j].bitfield.class == RegMask
 	  || x->types[j].bitfield.class == SReg
 	  || x->types[j].bitfield.class == RegCR
 	  || x->types[j].bitfield.class == RegDR
-	  || x->types[j].bitfield.class == RegTR)
+	  || x->types[j].bitfield.class == RegTR
+	  || x->types[j].bitfield.class == RegBND)
 	fprintf (stdout, "%s\n", x->op[j].regs->reg_name);
       if (operand_type_check (x->types[j], imm))
 	pe (x->op[j].imms);

[PATCH 3/6] ix86: enable 2nd CFI test

[Jan Beulich]

While putting together the previous patch I noticed that this test,
forever since its introduction, was dead. Update it so it will pass,
and enable it.

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

	* testsuite/gas/cfi/cfi-i386-2.d: Adjust expectations.
	* testsuite/gas/cfi/cfi.exp: Run this test.

--- a/gas/testsuite/gas/cfi/cfi-i386-2.d
+++ b/gas/testsuite/gas/cfi/cfi-i386-2.d
@@ -10,17 +10,17 @@ Contents of the .eh_frame section:
   Return address column: 8
   Augmentation data:     1b
 
-  DW_CFA_def_cfa: r4 ofs 4
-  DW_CFA_offset: r8 at cfa-4
+  DW_CFA_def_cfa: r4 \(esp\) ofs 4
+  DW_CFA_offset: r8 \(eip\) at cfa-4
   DW_CFA_nop
   DW_CFA_nop
 
-00000018 0+0018 0+001c FDE cie=0+0000 pc=0+0020..0+0029
-  DW_CFA_advance_loc: 1 to 0+0021
+00000018 0+0018 0+001c FDE cie=0+0000 pc=0+0000..0+0009
+  DW_CFA_advance_loc: 1 to 0+0001
   DW_CFA_def_cfa_offset: 8
-  DW_CFA_offset: r5 at cfa-8
-  DW_CFA_advance_loc: 4 to 0+0025
-  DW_CFA_offset: r3 at cfa-12
+  DW_CFA_offset: r5 \(ebp\) at cfa-8
+  DW_CFA_advance_loc: 4 to 0+0005
+  DW_CFA_offset: r3 \(ebx\) at cfa-12
   DW_CFA_def_cfa_offset: 12
   DW_CFA_nop
 
--- a/gas/testsuite/gas/cfi/cfi.exp
+++ b/gas/testsuite/gas/cfi/cfi.exp
@@ -48,6 +48,7 @@ if  { [istarget "i*86-*-*"] || [istarget
     if { [gas_x86_32_check] }  then {
 	set ASFLAGS "$ASFLAGS --32"
 	run_dump_test "cfi-i386"
+	run_dump_test "cfi-i386-2"
 	set ASFLAGS "$old_ASFLAGS"
     }
 

Re: [PATCH 0/6] gas/x86: adjustments to register handling

[H.J. Lu]

On Wed, Jun 3, 2020 at 3:13 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> I've noticed a couple of bugs or other anomalies, and the patches
> below are the outcome of trying to rectify these.
>
> 1: restrict use of register aliases
> 2: also allow %st(N) in CFI directives
> 3: enable 2nd CFI test
> 4: restrict %tr<N> visibility
> 5: simplify check_byte_reg()
> 6: also handle %k<N> and %bnd<N> in debugging helpers
>

OK for all.

Thanks.

-- 
H.J.