[v2,1/4] x86: extend LEA's segment override warning

Message ID 21d1d85b-d591-bcc5-9ae9-eea0901997fb@suse.com
State New
Headers show
Series
  • x86: segment override handling adjustments
Related show

Commit Message

Jan Beulich Feb. 14, 2020, 11:43 a.m.
For one both possible forms should be warned about. And then, to guard
against future surprises, qualify the original opcode check by excluding
VEX/EVEX-like templates.

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

	* config/tc-i386.c (process_operands): Also check insn prefix
	for ineffectual segment override warning. Don't cover possible
	VEX/EVEX encoded insns there.
	* testsuite/gas/i386/lea.s, testsuite/gas/i386/lea.d,
	testsuite/gas/i386/lea.e: New.
	* testsuite/gas/i386/i386.exp: Run new test.
---
v2: Move controversial MPX part to separate patch.

Comments

H.J. Lu Feb. 14, 2020, 12:02 p.m. | #1
On Fri, Feb 14, 2020 at 3:43 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> For one both possible forms should be warned about. And then, to guard

> against future surprises, qualify the original opcode check by excluding

> VEX/EVEX-like templates.

>

> gas/

> 2020-02-XX  Jan Beulich  <jbeulich@suse.com>

>

>         * config/tc-i386.c (process_operands): Also check insn prefix

>         for ineffectual segment override warning. Don't cover possible

>         VEX/EVEX encoded insns there.

>         * testsuite/gas/i386/lea.s, testsuite/gas/i386/lea.d,

>         testsuite/gas/i386/lea.e: New.

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

> ---

> v2: Move controversial MPX part to separate patch.

>

> --- a/gas/config/tc-i386.c

> +++ b/gas/config/tc-i386.c

> @@ -7194,9 +7194,10 @@ duplicate:

>         }

>      }

>

> -  if (i.tm.base_opcode == 0x8d /* lea */

> -      && i.seg[0]

> -      && !quiet_warnings)

> +  if ((i.seg[0] || i.prefix[SEG_PREFIX])


Does your testcase verify that we need to check both
i.seg[0] and i.prefix[SEG_PREFIX]?  If yes, patch is OK.

Thanks.

> +      && !quiet_warnings

> +      && i.tm.base_opcode == 0x8d /* lea */

> +      && !is_any_vex_encoding(&i.tm))

>      as_warn (_("segment override on `%s' is ineffectual"), i.tm.name);

>


-- 
H.J.
Jan Beulich Feb. 14, 2020, 12:06 p.m. | #2
On 14.02.2020 13:02, H.J. Lu wrote:
> On Fri, Feb 14, 2020 at 3:43 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> For one both possible forms should be warned about. And then, to guard

>> against future surprises, qualify the original opcode check by excluding

>> VEX/EVEX-like templates.

>>

>> gas/

>> 2020-02-XX  Jan Beulich  <jbeulich@suse.com>

>>

>>         * config/tc-i386.c (process_operands): Also check insn prefix

>>         for ineffectual segment override warning. Don't cover possible

>>         VEX/EVEX encoded insns there.

>>         * testsuite/gas/i386/lea.s, testsuite/gas/i386/lea.d,

>>         testsuite/gas/i386/lea.e: New.

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

>> ---

>> v2: Move controversial MPX part to separate patch.

>>

>> --- a/gas/config/tc-i386.c

>> +++ b/gas/config/tc-i386.c

>> @@ -7194,9 +7194,10 @@ duplicate:

>>         }

>>      }

>>

>> -  if (i.tm.base_opcode == 0x8d /* lea */

>> -      && i.seg[0]

>> -      && !quiet_warnings)

>> +  if ((i.seg[0] || i.prefix[SEG_PREFIX])

> 

> Does your testcase verify that we need to check both

> i.seg[0] and i.prefix[SEG_PREFIX]?


Yes it does - that's why there are two different forms of LEA.
It was actually me not seeing the warning on one of the forms
that prompted me to see what's wrong with the condition.

>  If yes, patch is OK.


Thanks.

Jan

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -7194,9 +7194,10 @@  duplicate:
 	}
     }
 
-  if (i.tm.base_opcode == 0x8d /* lea */
-      && i.seg[0]
-      && !quiet_warnings)
+  if ((i.seg[0] || i.prefix[SEG_PREFIX])
+      && !quiet_warnings
+      && i.tm.base_opcode == 0x8d /* lea */
+      && !is_any_vex_encoding(&i.tm))
     as_warn (_("segment override on `%s' is ineffectual"), i.tm.name);
 
   /* If a segment was explicitly specified, and the specified segment
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -65,6 +65,7 @@  if [expr ([istarget "i*86-*-*"] ||  [ist
     run_dump_test "intelok"
     run_dump_test "prefix"
     run_list_test "prefix32" "-al"
+    run_dump_test "lea"
     run_dump_test "amd"
     run_dump_test "katmai"
     run_dump_test "jump"
--- /dev/null
+++ b/gas/testsuite/gas/i386/lea.d
@@ -0,0 +1,12 @@ 
+#objdump: -dw
+#name: i386 LEA-like warnings
+#warning_output: lea.e
+
+.*: +file format .*
+
+Disassembly of section .text:
+
+0+ <start>:
+[ 	]*[0-9a-f]+:[ 	]+36 8d 00[ 	]+lea[ 	]+%ss:\(%eax\),%eax
+[ 	]*[0-9a-f]+:[ 	]+36 8d 00[ 	]+lea[ 	]+%ss:\(%eax\),%eax
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/lea.e
@@ -0,0 +1,3 @@ 
+.*: Assembler messages:
+.*:3: Warning: .* `lea' .*
+.*:4: Warning: .* `lea' .*
--- /dev/null
+++ b/gas/testsuite/gas/i386/lea.s
@@ -0,0 +1,4 @@ 
+	.text
+start:
+	lea	%ss:(%eax), %eax
+	ss lea	(%eax), %eax