[PATCH 0/3] x86: improve handling of equates to registers

[PATCH 0/3] x86: improve handling of equates to registers

[Jan Beulich]

Fuzzed input led to Alan addressing PR gas/28977 in, I think, a
sub-optimal way. An alternative is proposed in patch 1 alongside
the intention to then revert the original workaround.

In the course of discussing things with Alan he pointed out another
problematic construct, which patch 2 makes work. Variants of that
involving forward references, which are e.g. not resolvable at the
time an insn using it is being processed, would cause fatal and/or
internal errors, which patch 3 "softens" to an ordinary error.

1: don't attempt to resolve equates and alike from i386_parse_name()
2: improve resolution of register equates
3: reject relocations involving registers

Jan

[PATCH 1/3] x86: don't attempt to resolve equates and alike from i386_parse_name()

[Jan Beulich]

PR gas/28977

Perhaps right from its introduction in 4d1bb7955a8b it was wrong for
i386_parse_name() to call parse_register(). This being a hook from the
expression parser, it shouldn't be resolving e.g. equated symbols.
That's relevant only for all other callers of parse_register().

To compensate, in Intel syntax mode check_register() needs calling;
perhaps not doing so was an oversight right when the function was
introduced. This is necessary in particular to force EVEX encoding when
VRex registers are used (but of course also to reject bad uses of
registers, i.e. fully matching what parse_register() needs it for).
---
This is intended as a replacement for the then to be reverted
5fac3f02edac ("PR28977 tc-i386.c internal error in parse_register").

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -13000,11 +13000,12 @@ parse_register (char *reg_string, char *
 int
 i386_parse_name (char *name, expressionS *e, char *nextcharP)
 {
-  const reg_entry *r;
+  const reg_entry *r = NULL;
   char *end = input_line_pointer;
 
   *end = *nextcharP;
-  r = parse_register (name, &input_line_pointer);
+  if (*name == REGISTER_PREFIX || allow_naked_reg)
+    r = parse_real_register (name, &input_line_pointer);
   if (r && end <= input_line_pointer)
     {
       *nextcharP = *input_line_pointer;
--- a/gas/config/tc-i386-intel.c
+++ b/gas/config/tc-i386-intel.c
@@ -289,6 +289,13 @@ i386_intel_simplify_register (expression
       return 0;
     }
 
+  if (!check_register (&i386_regtab[reg_num]))
+    {
+      as_bad (_("register '%s%s' cannot be used here"),
+	      register_prefix, i386_regtab[reg_num].reg_name);
+      return 0;
+    }
+
   if (!intel_state.in_bracket)
     {
       if (i.op[this_operand].regs)
--- /dev/null
+++ b/gas/testsuite/gas/i386/equ-2.l
@@ -0,0 +1,17 @@
+.*: Assembler messages:
+.*:8: Error: .*
+#...
+GAS LISTING .*
+
+
+[ 	]*[0-9]+[ 	]+# .*
+[ 	]*[0-9]+[ 	]+equ:
+[ 	]*[0-9]+[ 	]+s = %edx % %ecx
+[ 	]*[0-9]+[ 	]+x = s
+[ 	]*[0-9]+[ 	]+y = s
+[ 	]*[0-9]+[ 	]+z = s
+[ 	]*[0-9]+[ 	]*
+[ 	]*[0-9]+[ 	]+t = %ymm5%%%!%%%%!%%%%%%%%!%ebp%%%%%%%%%%%%%%%%%%M
+[ 	]*[0-9]+[ 	]+a = t
+[ 	]*[0-9]+[ 	]+b = t
+[ 	]*[0-9]+[ 	]+c = t
--- /dev/null
+++ b/gas/testsuite/gas/i386/equ-2.s
@@ -0,0 +1,11 @@
+# PR gas/28977
+equ:
+	s = %edx % %ecx
+	x = s
+	y = s
+	z = s
+
+	t = %ymm5%%%!%%%%!%%%%%%%%!%ebp%%%%%%%%%%%%%%%%%%M
+	a = t
+	b = t
+	c = t
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -99,6 +99,7 @@ if [gas_32_check] then {
     run_list_test "suffix-bad"
     run_dump_test "immed32"
     run_dump_test "equ"
+    run_list_test "equ-2" "-al"
     run_list_test "equ-bad"
     run_dump_test "divide"
     run_dump_test "quoted"

[PATCH 2/3] x86: improve resolution of register equates

[Jan Beulich]

Allow transitive (or recursive) equates to work in addition to direct
ones. The only requirements are that
- the equate being straight of a register, i.e. no expressions involved
  (albeit I'm afraid something like "%eax + 0" will be viewed as %eax),
- at the point of use there's no forward ref left which cannot be
  resolved, yet.
---
The adjustment to use "fresh" symbols (a, b, c) in the test case is to
account for what I'd call a separate anomaly: y and z "latch" their
values upon first use, i.e. what they resolve to doesn't change anymore
when x changes. This looks to be separate because it can as well be
observed in an architecture independent way, by e.g. using constants
instead of registers and data generation directives instead of insns. I
think this is also related to why the example in PR gas/28977 had to use
two assignments in order to actually observe the misbehavior.

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -12974,6 +12974,14 @@ parse_register (char *reg_string, char *
       input_line_pointer = reg_string;
       c = get_symbol_name (&reg_string);
       symbolP = symbol_find (reg_string);
+      while (symbolP && S_GET_SEGMENT (symbolP) != reg_section)
+	{
+	  const expressionS *e = symbol_get_value_expression(symbolP);
+
+	  if (e->X_op != O_symbol || e->X_add_number)
+	    break;
+	  symbolP = e->X_add_symbol;
+	}
       if (symbolP && S_GET_SEGMENT (symbolP) == reg_section)
 	{
 	  const expressionS *e = symbol_get_value_expression (symbolP);
--- a/gas/config/tc-i386-intel.c
+++ b/gas/config/tc-i386-intel.c
@@ -529,6 +529,9 @@ static int i386_intel_simplify (expressi
       if (e->X_add_symbol
 	  && !i386_intel_simplify_symbol (e->X_add_symbol))
 	return 0;
+      if (!the_reg && this_operand >= 0
+	  && e->X_op == O_symbol && !e->X_add_number)
+	the_reg = i.op[this_operand].regs;
       if (e->X_op == O_add || e->X_op == O_subtract)
 	{
 	  base = intel_state.base;
--- a/gas/testsuite/gas/i386/equ.d
+++ b/gas/testsuite/gas/i386/equ.d
@@ -13,6 +13,8 @@ Disassembly of section .text:
 [ 0-9a-f]+:[ 	0-9a-f]+test[ 	]+%ecx,%ecx
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+%fs:\(%ecx,%ecx,4\),%ecx
 [ 0-9a-f]+:[ 	0-9a-f]+fadd[ 	]+%st\(1\),%st
+[ 0-9a-f]+:[ 	0-9a-f]+fmul[ 	]+%st\(1\),%st
+[ 0-9a-f]+:[ 	0-9a-f]+fsub[ 	]+%st\(1\),%st
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+\$0xfffffffe,%eax
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+0xfffffffe,%eax
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+\$0x0,%eax[ 	0-9a-f]+:[ 	a-zA-Z0-9_]+xtrn
@@ -21,7 +23,11 @@ Disassembly of section .text:
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+%gs:\(%edx,%edx,8\),%edx
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+%gs:\(%edx,%edx,8\),%edx
 [ 0-9a-f]+:[ 	0-9a-f]+fadd[ 	]+%st\(1\),%st
+[ 0-9a-f]+:[ 	0-9a-f]+fmul[ 	]+%st\(1\),%st
+[ 0-9a-f]+:[ 	0-9a-f]+fsub[ 	]+%st\(1\),%st
 [ 0-9a-f]+:[ 	0-9a-f]+fadd[ 	]+%st\(7\),%st
+[ 0-9a-f]+:[ 	0-9a-f]+fmul[ 	]+%st\(7\),%st
+[ 0-9a-f]+:[ 	0-9a-f]+fsub[ 	]+%st\(7\),%st
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+0x4\(%edx\),%eax
 [ 0-9a-f]+:[ 	0-9a-f]+mov[ 	]+0x4\(%edx\),%eax
 #pass
--- a/gas/testsuite/gas/i386/equ.s
+++ b/gas/testsuite/gas/i386/equ.s
@@ -13,8 +13,12 @@ _start:
  .equ s, %fs
 	testl	r, r
 	movl	s:(r,r,4), r
+ .equ z, y
+ .equ y, x
  .equ x, %st(1)
 	fadd	x
+	fmul	y
+	fsub	z
 
  .if r <> %ecx
  .err
@@ -37,8 +41,14 @@ _start:
 	mov	r, s:[r+r*8]
 	mov	r, s:[8*r+r]
 	fadd	x
- .equ x, st(7)
-	fadd	x
+	fmul	y
+	fsub	z
+ .equ c, b
+ .equ b, a
+ .equ a, st(7)
+	fadd	a
+	fmul	b
+	fsub	c
  .equ r, edx + 4
 	mov	eax, [r]
 	mov	eax, [r]

[PATCH 3/3] x86: reject relocations involving registers

[Jan Beulich]

To prevent fatal or even internal errors, add a simple check to
i386_validate_fix(), rejecting relocations when their target symbol is
an equate of a register (or resolved to reg_section for any other
reason).
---
I wasn't sure whether
a) ->fx_subsy might require similar treatment (don't know whether
   generic code would suitably prevent such),
b) expr_section may also need checking for (don't know whether symbols
   can remain to be associated with that section).

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -14189,6 +14189,17 @@ i386_cons_align (int ignore ATTRIBUTE_UN
 int
 i386_validate_fix (fixS *fixp)
 {
+  if (fixp->fx_addsy && S_GET_SEGMENT(fixp->fx_addsy) == reg_section)
+    {
+      reloc_howto_type *howto;
+
+      howto = bfd_reloc_type_lookup (stdoutput, fixp->fx_r_type);
+      as_bad_where (fixp->fx_file, fixp->fx_line,
+		    _("invalid %s relocation against register"),
+		    howto ? howto->name : "<unknown>");
+      return 0;
+    }
+
 #if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
   if (fixp->fx_r_type == BFD_RELOC_SIZE32
       || fixp->fx_r_type == BFD_RELOC_SIZE64)
--- a/gas/testsuite/gas/i386/inval-equ-2.l
+++ b/gas/testsuite/gas/i386/inval-equ-2.l
@@ -1,4 +1,7 @@
 .*: Assembler messages:
+.*:3: Error: .*
+.*:5: Error: .*
+.*:8: Error: .*
 .*: Error: .*
 .*: Error: .*
 .*: Error: .*
@@ -15,6 +18,9 @@ GAS LISTING .*
 [ 	]*6[ 	]+\.globl  bar2
 [ 	]*7[ 	]+\.set    bar3,\(%eax\+1\)
 [ 	]*8[ 	]+\?\?\?\? A1...... 		mov bar3,%eax
+.*  Error: invalid .* relocation against register
+.*  Error: invalid .* relocation against register
+.*  Error: invalid .* relocation against register
 .*  Error: can't make global register symbol `bar1'
 .*  Error: can't make global register symbol `bar2'
 .*  Error: can't make global register symbol `bar3'

Re: [PATCH 0/3] x86: improve handling of equates to registers

[H.J. Lu]

On Tue, Mar 22, 2022 at 6:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Fuzzed input led to Alan addressing PR gas/28977 in, I think, a
> sub-optimal way. An alternative is proposed in patch 1 alongside
> the intention to then revert the original workaround.
>
> In the course of discussing things with Alan he pointed out another
> problematic construct, which patch 2 makes work. Variants of that
> involving forward references, which are e.g. not resolvable at the
> time an insn using it is being processed, would cause fatal and/or
> internal errors, which patch 3 "softens" to an ordinary error.
>
> 1: don't attempt to resolve equates and alike from i386_parse_name()
> 2: improve resolution of register equates
> 3: reject relocations involving registers
>

OK to all.

Thanks.

-- 
H.J.