[1/3] x86: extend LEA's segment override warning

Message ID 5a4cb285-3d1a-8842-8b4b-94423a37e8d0@suse.com
State New
Headers show
Series
  • x86: segment override handling adjustments
Related show

Commit Message

Jan Beulich Feb. 13, 2020, 2:05 p.m.
For one both possible forms should be warned about. And then there are
a couple of MPX insns behaving LEA-like, which should be warned about in
the same way. Finally, 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. Also cover BNDC* and
	BNDMK there. 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.

Comments

H.J. Lu Feb. 13, 2020, 2:11 p.m. | #1
On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> For one both possible forms should be warned about. And then there are

> a couple of MPX insns behaving LEA-like, which should be warned about in

> the same way. Finally, 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. Also cover BNDC* and

>         BNDMK there. 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.

>


Why should it be warning, not error?


-- 
H.J.
Jan Beulich Feb. 13, 2020, 2:48 p.m. | #2
On 13.02.2020 15:11, H.J. Lu wrote:
> On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> For one both possible forms should be warned about. And then there are

>> a couple of MPX insns behaving LEA-like, which should be warned about in

>> the same way. Finally, 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. Also cover BNDC* and

>>         BNDMK there. 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.

>>

> 

> Why should it be warning, not error?


Because the code isn't wrong, just inefficient. I also don't think
converting from warning to error should be done in the same patch
as extending the coverage of what gets a diagnostic emitted.

Jan
H.J. Lu Feb. 13, 2020, 3:51 p.m. | #3
On Thu, Feb 13, 2020 at 6:48 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 13.02.2020 15:11, H.J. Lu wrote:

> > On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> For one both possible forms should be warned about. And then there are

> >> a couple of MPX insns behaving LEA-like, which should be warned about in

> >> the same way. Finally, 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. Also cover BNDC* and

> >>         BNDMK there. 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.

> >>

> >

> > Why should it be warning, not error?

>

> Because the code isn't wrong, just inefficient. I also don't think

> converting from warning to error should be done in the same patch

> as extending the coverage of what gets a diagnostic emitted.

>


What do we gain to allow it?


-- 
H.J.
Jan Beulich Feb. 13, 2020, 4 p.m. | #4
On 13.02.2020 16:51, H.J. Lu wrote:
> On Thu, Feb 13, 2020 at 6:48 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 13.02.2020 15:11, H.J. Lu wrote:

>>> On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> For one both possible forms should be warned about. And then there are

>>>> a couple of MPX insns behaving LEA-like, which should be warned about in

>>>> the same way. Finally, 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. Also cover BNDC* and

>>>>         BNDMK there. 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.

>>>>

>>>

>>> Why should it be warning, not error?

>>

>> Because the code isn't wrong, just inefficient. I also don't think

>> converting from warning to error should be done in the same patch

>> as extending the coverage of what gets a diagnostic emitted.

> 

> What do we gain to allow it?


It has always been allowed, and in one of the two cases even silently.
We're liable to break people's working code if we changed this.

Jan
H.J. Lu Feb. 13, 2020, 4:31 p.m. | #5
On Thu, Feb 13, 2020 at 8:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 13.02.2020 16:51, H.J. Lu wrote:

> > On Thu, Feb 13, 2020 at 6:48 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 13.02.2020 15:11, H.J. Lu wrote:

> >>> On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> For one both possible forms should be warned about. And then there are

> >>>> a couple of MPX insns behaving LEA-like, which should be warned about in

> >>>> the same way. Finally, 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. Also cover BNDC* and

> >>>>         BNDMK there. 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.

> >>>>

> >>>

> >>> Why should it be warning, not error?

> >>

> >> Because the code isn't wrong, just inefficient. I also don't think

> >> converting from warning to error should be done in the same patch

> >> as extending the coverage of what gets a diagnostic emitted.

> >

> > What do we gain to allow it?

>

> It has always been allowed, and in one of the two cases even silently.

> We're liable to break people's working code if we changed this.

>


Given that MPX has been deprecated, MPX codes are very unlikely to
change.  Assembler shouldn't bother with this.

-- 
H.J.
Jan Beulich Feb. 13, 2020, 4:38 p.m. | #6
On 13.02.2020 17:31, H.J. Lu wrote:
> On Thu, Feb 13, 2020 at 8:00 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 13.02.2020 16:51, H.J. Lu wrote:

>>> On Thu, Feb 13, 2020 at 6:48 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> On 13.02.2020 15:11, H.J. Lu wrote:

>>>>> On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>

>>>>>> For one both possible forms should be warned about. And then there are

>>>>>> a couple of MPX insns behaving LEA-like, which should be warned about in

>>>>>> the same way. Finally, 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. Also cover BNDC* and

>>>>>>         BNDMK there. 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.

>>>>>>

>>>>>

>>>>> Why should it be warning, not error?

>>>>

>>>> Because the code isn't wrong, just inefficient. I also don't think

>>>> converting from warning to error should be done in the same patch

>>>> as extending the coverage of what gets a diagnostic emitted.

>>>

>>> What do we gain to allow it?

>>

>> It has always been allowed, and in one of the two cases even silently.

>> We're liable to break people's working code if we changed this.

> 

> Given that MPX has been deprecated, MPX codes are very unlikely to

> change.  Assembler shouldn't bother with this.


What a strange position to take. Anyway, are you saying the change is
going to be okay if I remove the MPX aspects from it? (It is only
this 3rd reply of yours where you mention MPX, so I'm a little
puzzled.)

Jan
H.J. Lu Feb. 13, 2020, 4:44 p.m. | #7
On Thu, Feb 13, 2020 at 8:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 13.02.2020 17:31, H.J. Lu wrote:

> > On Thu, Feb 13, 2020 at 8:00 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 13.02.2020 16:51, H.J. Lu wrote:

> >>> On Thu, Feb 13, 2020 at 6:48 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> On 13.02.2020 15:11, H.J. Lu wrote:

> >>>>> On Thu, Feb 13, 2020 at 6:05 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>>

> >>>>>> For one both possible forms should be warned about. And then there are

> >>>>>> a couple of MPX insns behaving LEA-like, which should be warned about in

> >>>>>> the same way. Finally, 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. Also cover BNDC* and

> >>>>>>         BNDMK there. 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.

> >>>>>>

> >>>>>

> >>>>> Why should it be warning, not error?

> >>>>

> >>>> Because the code isn't wrong, just inefficient. I also don't think

> >>>> converting from warning to error should be done in the same patch

> >>>> as extending the coverage of what gets a diagnostic emitted.

> >>>

> >>> What do we gain to allow it?

> >>

> >> It has always been allowed, and in one of the two cases even silently.

> >> We're liable to break people's working code if we changed this.

> >

> > Given that MPX has been deprecated, MPX codes are very unlikely to

> > change.  Assembler shouldn't bother with this.

>

> What a strange position to take. Anyway, are you saying the change is

> going to be okay if I remove the MPX aspects from it? (It is only

> this 3rd reply of yours where you mention MPX, so I'm a little

> puzzled.)

>


Yes.

-- 
H.J.

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -7197,9 +7197,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 && !is_any_vex_encoding(&i.tm)) /* lea */
+          || ((i.tm.base_opcode | 0x010001) == 0xf30f1b) /* bnd{c[lnu],mk} */))
     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,20 @@ 
+#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
+[ 	]*[0-9a-f]+:[ 	]+36 f3 0f 1a 00[ 	]+bndcl[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f3 0f 1a 00[ 	]+bndcl[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f2 0f 1b 00[ 	]+bndcn[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f2 0f 1b 00[ 	]+bndcn[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f2 0f 1a 00[ 	]+bndcu[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f2 0f 1a 00[ 	]+bndcu[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f3 0f 1b 00[ 	]+bndmk[ 	]+%ss:\(%eax\),%bnd0
+[ 	]*[0-9a-f]+:[ 	]+36 f3 0f 1b 00[ 	]+bndmk[ 	]+%ss:\(%eax\),%bnd0
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/i386/lea.e
@@ -0,0 +1,11 @@ 
+.*: Assembler messages:
+.*:3: Warning: .* `lea' .*
+.*:4: Warning: .* `lea' .*
+.*:6: Warning: .* `bndcl' .*
+.*:7: Warning: .* `bndcl' .*
+.*:9: Warning: .* `bndcn' .*
+.*:10: Warning: .* `bndcn' .*
+.*:12: Warning: .* `bndcu' .*
+.*:13: Warning: .* `bndcu' .*
+.*:15: Warning: .* `bndmk' .*
+.*:16: Warning: .* `bndmk' .*
--- /dev/null
+++ b/gas/testsuite/gas/i386/lea.s
@@ -0,0 +1,16 @@ 
+	.text
+start:
+	lea	%ss:(%eax), %eax
+	ss lea	(%eax), %eax
+
+	bndcl	%ss:(%eax), %bnd0
+	ss bndcl (%eax), %bnd0
+
+	bndcn	%ss:(%eax), %bnd0
+	ss bndcn (%eax), %bnd0
+
+	bndcu	%ss:(%eax), %bnd0
+	ss bndcu (%eax), %bnd0
+
+	bndmk	%ss:(%eax), %bnd0
+	ss bndmk (%eax), %bnd0