This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v7] x86: replace adhoc (partly wrong) ambiguous operand checking for MOVSX/MOVZX
On Thu, Feb 13, 2020 at 5:52 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 13.02.2020 13:19, H.J. Lu wrote:
> > On Thu, Feb 13, 2020 at 1:21 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> For these to get treatment consistent with other operand size checking
> >> the special logic shouldn't live in md_assemble(), but process_suffix().
> >> And there's more logic involved than simply zapping the suffix, in
> >> particular to enforce the general "suffix trumps register size" rule in
> >> AT&T mode. The cases where behavior is being corrected can be seen from
> >> the testcase adjustments (as mentioned in the commit introducing these
> >> tests some cases had wrong expectations at that point, but it seemed
> >> better to separate testcase introduction from actual code changes).
> >>
> >> Note however that MOVS[BW]* and MOVZ[BW]* still won't be fully
> >> consistent, due to the objection to fold MOVS* templates just like was
> >> done for MOVZ* in c07315e0c6 ("x86: allow suffix-less movzw and 64-bit
> >> movzb").
> >>
> >> Note further that the assembler change points out (and this patch fixes)
> >> a wrong Intel syntax test introduced by bc31405ebb2c ("x86-64: Properly
> >> encode and decode movsxd"): When source code specifies a 16-bit
> >> destination register, disassembly expectations shouldn't have been to
> >> find a 32-bit one.
> >>
> >> gas/
> >> 2020-02-XX Jan Beulich <jbeulich@suse.com>
> >>
> >> PR gas/25438
> >> * config/tc-i386.c (md_assemble): Move movsx/movzx special
> >> casing ...
> >> (process_suffix): ... here. Consider just the first operand
> >> initially.
> >> (check_long_reg): Drop opcode 0x63 special case again.
> >> * testsuite/gas/i386/i386.s, testsuite/gas/i386/iamcu-1.s,
> >> testsuite/gas/i386/ilp32/x86-64.s, testsuite/gas/i386/x86_64.s:
> >> Move ambiguous operand size tests ...
> >> * testsuite/gas/i386/noreg16.s, testsuite/gas/i386/noreg32.s,
> >> testsuite/gas/i386/noreg64.s: ... here.
> >> * testsuite/gas/i386/i386.d, testsuite/gas/i386/i386-intel.d
> >> testsuite/gas/i386/iamcu-1.d, testsuite/gas/i386/ilp32/x86-64.d,
> >> testsuite/gas/i386/k1om.d, testsuite/gas/i386/l1om.d,
> >> testsuite/gas/i386/movx16.l, testsuite/gas/i386/movx32.l,
> >> testsuite/gas/i386/movx64.l, testsuite/gas/i386/noreg16.d,
> >> testsuite/gas/i386/noreg16.l, testsuite/gas/i386/noreg32.d,
> >> testsuite/gas/i386/noreg32.l, testsuite/gas/i386/noreg64.d,
> >> testsuite/gas/i386/noreg64.l, testsuite/gas/i386/x86-64-movsxd.d,
> >> testsuite/gas/i386/x86-64-movsxd-intel.d,
> >> testsuite/gas/i386/x86_64.d, testsuite/gas/i386/x86_64-intel.d:
> >> Adjust expectations.
> >> * testsuite/gas/i386/movx16.s, testsuite/gas/i386/movx16.l,
> >> testsuite/gas/i386/movx32.s, testsuite/gas/i386/movx32.l,
> >> testsuite/gas/i386/movx64.s, testsuite/gas/i386/movx64.l: New.
> >> * testsuite/gas/i386/i386.exp: Run new tests.
> >>
> >> opcodes/
> >> 2020-02-XX Jan Beulich <jbeulich@suse.com>
> >>
> >> PR gas/25438
> >> * i386-opc.tbl (movsx): Fold patterns. Also allow Reg32 as
> >> destination for Cpu64-only variant.
> >> (movsxd): Also allow Reg32 as destination. Drop Rex64.
> >> (movzx): Fold patterns.
> >> * i386-tbl.h: Re-generate.
> >
> > I got
> >
> > FAIL: i386 intel
> > FAIL: i386 intel
> > FAIL: i386 intel16
> > FAIL: i386 intel-ok
> > FAIL: i386 noreg16
> > FAIL: i386 noreg32
> > FAIL: i386 movx16
> > FAIL: i386 movx32
> > FAIL: i386
> > FAIL: i386 (Intel mode)
> > FAIL: gas/i386/iamcu-1
> > FAIL: x86_64
> > FAIL: x86-64 (Intel mode)
> > FAIL: i386 noreg64
> > FAIL: i386 movx64
> > FAIL: l1om
> > FAIL: k1om
> > FAIL: x86-64 (ILP32)
>
> I certainly didn't get any failures. Mind being more specific? Is this
> perhaps for an unusual target? Knowing what exactly at least some of the
> failures are would be helpful in case I can't repro it myself.
I applied your patch on
commit 99845b3b77ed1248b6fb94707f88868bde358ccc (origin/master, origin/HEAD)
Author: H.J. Lu <hjl.tools@gmail.com>
Date: Thu Feb 13 03:17:51 2020 -0800
plugin: Search bfd-plugins directories only once
and did
$ ..../configure --enable-plugins --disable-gdb --disable-gdbserver
--disable-libdecnumber --disable-readline --disable-sim
$ make
$ make check
Have you tried your patch on the current master branch?
> > Also should
> >
> > movsxb ax, BYTE PTR [rax]
> > movsxb eax, BYTE PTR [rax]
> > movsxb rax, BYTE PTR [rax]
> > movsxw eax, WORD PTR [rax]
> > movsxw rax, WORD PTR [rax]
> > movzxb ax, BYTE PTR [rax]
> > movzxb eax, BYTE PTR [rax]
> > movzxb rax, BYTE PTR [rax]
> > movzxw eax, WORD PTR [rax]
> > movzxw rax, WORD PTR [rax]
> >
> > be alllowed? It is odd to disallow movsxb, but allow movsxw.
>
> Why "disallow movsxb, but allow movsxw"? What do you refer to?
> The list above all assembles entirely fine for me. Together with
> the test failures I wonder whether you have a broken build.
>
> > I don't think Intel syntax should allow suffix in this case.
>
> This is a consistency thing again - if you don't want to allow
> suffixes here, they also shouldn't be allowed on other insns
> where they aren't really needed (add, mov, etc). This isn't the
> subject of the patch here.
[hjl@gnu-cfl-2 pr25438]$ cat x.s
.text
.intel_syntax noprefix
movsxb ax, BYTE PTR [rax]
movsxb eax, BYTE PTR [rax]
movsxb rax, BYTE PTR [rax]
movsxw eax, WORD PTR [rax]
movsxw rax, WORD PTR [rax]
movzxb ax, BYTE PTR [rax]
movzxb eax, BYTE PTR [rax]
movzxb rax, BYTE PTR [rax]
movzxw eax, WORD PTR [rax]
movzxw rax, WORD PTR [rax]
movzxd rax, DWORD PTR [rax]
[hjl@gnu-cfl-2 pr25438]$ ./as -al x.s
x.s: Assembler messages:
x.s:3: Error: `ax' not allowed with `movsxb'
x.s:4: Error: `eax' not allowed with `movsxb'
x.s:5: Error: `rax' not allowed with `movsxb'
x.s:8: Error: `ax' not allowed with `movzxb'
x.s:9: Error: `eax' not allowed with `movzxb'
x.s:10: Error: `rax' not allowed with `movzxb'
x.s:13: Error: invalid instruction suffix for `movzx'
GAS LISTING x.s page 1
1 .text
2 .intel_syntax noprefix
3 movsxb ax, BYTE PTR [rax]
4 movsxb eax, BYTE PTR [rax]
5 movsxb rax, BYTE PTR [rax]
6 ???? 660FBF00 movsxw eax, WORD PTR [rax]
7 ???? 660FBF00 movsxw rax, WORD PTR [rax]
8 movzxb ax, BYTE PTR [rax]
9 movzxb eax, BYTE PTR [rax]
10 movzxb rax, BYTE PTR [rax]
11 ???? 660FB700 movzxw eax, WORD PTR [rax]
12 ???? 660FB700 movzxw rax, WORD PTR [rax]
13 movzxd rax, DWORD PTR [rax]
Why is movzxw allowed, but not movsxb nor movsxd?
--
H.J.