This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v3 03/10] x86: move certain MOVSX/MOVZX tests
On Wed, Jan 22, 2020 at 8:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.01.2020 17:09, H.J. Lu wrote:
> > On Wed, Jan 22, 2020 at 8:04 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 20.01.2020 16:33, H.J. Lu wrote:
> >>> On Mon, Jan 20, 2020 at 7:30 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 13.01.2020 18:33, H.J. Lu wrote:
> >>>>> On Fri, Dec 27, 2019 at 1:24 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>>>>>
> >>>>>> Some encodings are about to gain a warning - move them from test cases
> >>>>>> not expecting any diagnostics to the new, dedicated ones, to allow
> >>>>>> better focus on the actual changes in the subsequent patch.
> >>>>>>
> >>>>>> The new tests added have some wrong expectations right now, which will
> >>>>>> be corrected by the next patch. The test is being added here to make
> >>>>>> more visible which cases actually were wrong (and hence get changed),
> >>>>>> besides demonstrating that in the vast majority of cases the subsequent
> >>>>>> change doesn't alter generated code.
> >>>>>
> >>>>> We can't change the encoding nor disallow it. At most, we can issue a
> >>>>> warning. If it is the case, calling it "wrong" isn't accurate.
> >>>>
> >>>> If you look at the following patch you'll find that the two cases
> >>>> where an encoding actually changes, this will then be accompanied
> >>>
> >>> Please open a bug for wrong encoding before we decide what to do
> >>> about it. At minimum, we need a record.
> >>
> >> Seeing you say "We should make them hard errors now." in that bug,
> >> I hope it is clear that that's an orthogonal job, which - once
> >> done - will convert what gets warned about in the next patch (i.e.
> >> the cases where you're worried about encoding changes) into errors
> >> at the same time. Hence I'd like to ask what the disposition of
> >> this and the next patch is: With your cited remark in the bug, I
> >> don't see what adjustments you want to see done here. IOW it would
> >> seem to me that both patches could go in as they are.
> >
> > All MOVZX with incorrect operands should be hard errors in
> > 16-bit, 32-bit and 64-bit modes.
>
> Only once other insns behave in a similar way. Programmers should
> be able to predict assembler behavior, and hence derive what'll
> happen for one insn/operands combination from what happens from
> any other, sufficiently similar one. This is what the patch helps
> with. The consistency in behavior won't be broken down the road
> when you or someone else switches the behavior you talk about
> from being warnings to being errors. But it can't be the purpose
> of this (or any) patch to introduce special cases for certain
> insns when none are warranted. In fact we should try to reduce
> the number of special cases (which I think I'm slowly making
> progress with, see e.g. the change to md_assemble() by patch 4,
> which, yes, unavoidably comes with the need to insert less
> arbitrary special casing code in process_suffix()).
>
> If you make the change you've outlined, I'll be happy to re-base
> the patches here on top, which will merely mean adjustments to
> the testsuite additions. If, however, said change will take time,
> then may I please get approval to get in the two patches here?
>
I am checking in this patch.
--
H.J.
From be4c5e58bdc839898739e0332aee84abf6e5299a Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 22 Jan 2020 09:24:14 -0800
Subject: [PATCH] x86: Always disallow double word suffix with word general
register
In 64-bit mode, double word suffix in mnemonic with word general register
is disallowed. Otherwise, assembler gives a warning:
$ cat /tmp/x.s
movl %ax, %bx
movl %ds, %ax
movl %ax, %cs
$ gcc -c /tmp/x.s
/tmp/x.s: Assembler messages:
/tmp/x.s:1: Error: incorrect register `%bx' used with `l' suffix
/tmp/x.s:2: Error: incorrect register `%ax' used with `l' suffix
/tmp/x.s:3: Error: incorrect register `%ax' used with `l' suffix
$ gcc -c /tmp/x.s -m32
/tmp/x.s: Assembler messages:
/tmp/x.s: Assembler messages:
/tmp/x.s:1: Warning: using `%ebx' instead of `%bx' due to `l' suffix
/tmp/x.s:1: Warning: using `%eax' instead of `%ax' due to `l' suffix
/tmp/x.s:2: Warning: using `%eax' instead of `%ax' due to `l' suffix
/tmp/x.s:3: Warning: using `%eax' instead of `%ax' due to `l' suffix
This patch makes it a hard error in all modes. Now we get:
$ gcc -c /tmp/x.s -m32
/tmp/x.s: Assembler messages:
/tmp/x.s:1: Error: incorrect register `%bx' used with `l' suffix
/tmp/x.s:2: Error: incorrect register `%ax' used with `l' suffix
/tmp/x.s:3: Error: incorrect register `%ax' used with `l' suffix
PR gas/25438
* config/tc-i386.c (check_long_reg): Always disallow double word
suffix in mnemonic with word general register.
* testsuite/gas/i386/general.s: Replace word general register
with double word general register for movl.
* testsuite/gas/i386/inval.s: Add tests for movl with word general
register.
* testsuite/gas/i386/general.l: Updated.
* testsuite/gas/i386/inval.l: Likewise.
---
gas/ChangeLog | 12 ++++++++++++
gas/config/tc-i386.c | 24 ++++++------------------
gas/testsuite/gas/i386/general.l | 12 +++---------
gas/testsuite/gas/i386/general.s | 6 +++---
gas/testsuite/gas/i386/inval.l | 7 +++++++
gas/testsuite/gas/i386/inval.s | 4 ++++
6 files changed, 35 insertions(+), 30 deletions(-)
diff --git a/gas/ChangeLog b/gas/ChangeLog
index 2d247a1012..8a18aa7774 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,15 @@
+2020-01-22 H.J. Lu <hongjiu.lu@intel.com>
+
+ PR gas/25438
+ * config/tc-i386.c (check_long_reg): Always disallow double word
+ suffix in mnemonic with word general register.
+ * testsuite/gas/i386/general.s: Replace word general register
+ with double word general register for movl.
+ * testsuite/gas/i386/inval.s: Add tests for movl with word general
+ register.
+ * testsuite/gas/i386/general.l: Updated.
+ * testsuite/gas/i386/inval.l: Likewise.
+
2020-01-22 Alan Modra <amodra@gmail.com>
* config/tc-ppc.c (parse_tls_arg): Handle tls arg for
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 3a2a1b7435..34778ae760 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -6672,28 +6672,16 @@ check_long_reg (void)
i.suffix);
return 0;
}
- /* Warn if the e prefix on a general reg is missing. */
- else if ((!quiet_warnings || flag_code == CODE_64BIT)
- && i.types[op].bitfield.word
+ /* Error if the e prefix on a general reg is missing. */
+ else if (i.types[op].bitfield.word
&& (i.tm.operand_types[op].bitfield.class == Reg
|| i.tm.operand_types[op].bitfield.instance == Accum)
&& i.tm.operand_types[op].bitfield.dword)
{
- /* Prohibit these changes in the 64bit mode, since the
- lowering is more complicated. */
- if (flag_code == CODE_64BIT)
- {
- as_bad (_("incorrect register `%s%s' used with `%c' suffix"),
- register_prefix, i.op[op].regs->reg_name,
- i.suffix);
- return 0;
- }
-#if REGISTER_WARNINGS
- as_warn (_("using `%s%s' instead of `%s%s' due to `%c' suffix"),
- register_prefix,
- (i.op[op].regs + REGNAM_EAX - REGNAM_AX)->reg_name,
- register_prefix, i.op[op].regs->reg_name, i.suffix);
-#endif
+ as_bad (_("incorrect register `%s%s' used with `%c' suffix"),
+ register_prefix, i.op[op].regs->reg_name,
+ i.suffix);
+ return 0;
}
/* Warn if the r prefix on a general reg is present. */
else if (i.types[op].bitfield.qword
diff --git a/gas/testsuite/gas/i386/general.l b/gas/testsuite/gas/i386/general.l
index acd44403ee..470527716a 100644
--- a/gas/testsuite/gas/i386/general.l
+++ b/gas/testsuite/gas/i386/general.l
@@ -9,8 +9,6 @@
.*:25: Warning:.*
.*:27: Warning:.*
.*:29: Warning:.*
-.*:39: Warning:.*
-.*:41: Warning:.*
.*:48: Warning:.*
.*:51: Warning:.*
.*:124: Warning:.*
@@ -30,7 +28,6 @@
.*:142: Warning:.*
.*:143: Warning:.*
.*:144: Warning:.*
-.*:178: Warning:.*
.*:224: Warning:.*
.*:233: Warning:.*
.*:234: Warning:.*
@@ -84,11 +81,9 @@
36 007e 1F popl %ds
37 007f 8CD8 mov %ds,%eax
38 0081 8CD8 movl %ds,%eax
- 39 0083 8CD8 movl %ds,%ax
-.*Warning:.*
+ 39 0083 8CDB movl %ds,%ebx
40 0085 8ED8 mov %eax,%ds
- 41 0087 8ED8 movl %ax,%ds
-.*Warning:.*
+ 41 0087 8EDB movl %ebx,%ds
42 0089 8ED8 movl %eax,%ds
43
44 008b 661E pushw %ds
@@ -244,8 +239,7 @@
175 01a9 66F7F1 div %cx,%ax
176 01ac F7F1 div %ecx,%eax
177 01ae 8EDE mov %si,%ds
- 178 01b0 8EDE movl %si,%ds # warning here
-.*Warning:.*
+ 178 01b0 8EDF movl %edi,%ds
179 01b2 1E pushl %ds
180 01b3 1E push %ds
181 01b4 A0000000 00 mov 0,%al
diff --git a/gas/testsuite/gas/i386/general.s b/gas/testsuite/gas/i386/general.s
index 39bbfe3e0b..77f54ae91f 100644
--- a/gas/testsuite/gas/i386/general.s
+++ b/gas/testsuite/gas/i386/general.s
@@ -36,9 +36,9 @@
popl %ds
mov %ds,%eax
movl %ds,%eax
- movl %ds,%ax
+ movl %ds,%ebx
mov %eax,%ds
- movl %ax,%ds
+ movl %ebx,%ds
movl %eax,%ds
pushw %ds
@@ -175,7 +175,7 @@
div %cx,%ax
div %ecx,%eax
mov %si,%ds
- movl %si,%ds # warning here
+ movl %edi,%ds
pushl %ds
push %ds
mov 0,%al
diff --git a/gas/testsuite/gas/i386/inval.l b/gas/testsuite/gas/i386/inval.l
index 3d52a17d2e..0159e97a3f 100644
--- a/gas/testsuite/gas/i386/inval.l
+++ b/gas/testsuite/gas/i386/inval.l
@@ -88,6 +88,9 @@
.*:98: Error: .*rol.*
.*:99: Error: .*rcl.*
.*:102: Error: .*
+.*:104: Error: .*
+.*:105: Error: .*
+.*:106: Error: .*
GAS LISTING .*
@@ -196,3 +199,7 @@ GAS LISTING .*
[ ]*[1-9][0-9]*[ ]+
[ ]*[1-9][0-9]*[ ]+\.att_syntax prefix
[ ]*[1-9][0-9]*[ ]+movsd \(%esi\), %ss:\(%edi\), %ss:\(%eax\)
+[ ]*[1-9][0-9]*[ ]+
+[ ]*[1-9][0-9]*[ ]+movl %ds, %ax
+[ ]*[1-9][0-9]*[ ]+movl %ax, %ds
+[ ]*[1-9][0-9]*[ ]+movl %ax, %bx
diff --git a/gas/testsuite/gas/i386/inval.s b/gas/testsuite/gas/i386/inval.s
index 47655e545e..3ff58b34c6 100644
--- a/gas/testsuite/gas/i386/inval.s
+++ b/gas/testsuite/gas/i386/inval.s
@@ -100,3 +100,7 @@ movnti word ptr [eax], ax
.att_syntax prefix
movsd (%esi), %ss:(%edi), %ss:(%eax)
+
+ movl %ds, %ax
+ movl %ax, %ds
+ movl %ax, %bx
--
2.24.1