Re: [PATCH v3 01/10] x86: improve handling of insns with ambiguous operand sizes

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Jan 10, 2020 at 12:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.01.2020 23:35,  H.J. Lu  wrote:
> > On Fri, Dec 27, 2019 at 1:22 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> Commit b76bc5d54e ("x86: don't default variable shift count insns to
> >> 8-bit operand size") pointed out a very bad case, but the underlying
> >> problem is, as mentioned on various occasions, much larger: Silently
> >> selecting a (nowhere documented afaict) certain default operand size
> >> when there's no "sizing" suffix and no suitable register operand(s) is
> >> simply dangerous (for the programmer to make mistakes).
> >>
> >> While in Intel syntax mode such mistakes already lead to an error (which
> >> is going to remain that way), AT&T syntax mode now gains warnings in
> >> such cases by default, which can be suppressed or promoted to an error
> >> if so desired by the programmer. Furthermore at least general purpose
> >> insns now consistently have a default applied (alongside the warning
> >> emission), rather than accepting some and refusing others.
> >>
> >> No warnings are (as before) to be generated for "DefaultSize" insns as
> >> well as ones acting on selector and other fixed-width values. For
> >> SYSRET, however, the DefaultSize needs to be dropped - it had been
> >> wrongly put there in the first place, as it's unrelated to .code16gcc
> >> (no stack accesses involved).
> >>
> >> As set forth as a prereq when I first mentioned this intended change a
> >> few years back, Linux as well as gcc have meanwhile been patched to
> >> avoid (emission of) ambiguous operands (and hence triggering of the new
> >> warning).
> >>
> >> Note that I think that in 64-bit mode IRET and far RET would better get
> >> a diagnostic too, as it's reasonably likely that a suffix-less instance
> >> really is meant to be a 64-bit one. But I guess I better make this a
> >> separate follow-on patch.
> >>
> >> Note further that floating point operations with integer operands are an
> >> exception for now: They continue to use short (16-bit) operands by
> >> default even in 32- and 64-bit modes.
> >>
> >> gas/
> >> 2020-01-XX  Jan Beulich  <jbeulich@suse.com>
> >>
> >>         * config/tc-i386.c (process_suffix): Drop SYSRET special case
> >>         and an intel_syntax check. Re-write lack-of-suffix processing
> >>         logic.
> >>         * doc/c-i386.texi: Document operand size defaults for suffix-
> >>         less AT&T syntax insns.
> >>         * testsuite/gas/i386/bundle.s, testsuite/gas/i386/lock-1.s,
> >>         testsuite/gas/i386/opcode.s, testsuite/gas/i386/sse3.s,
> >>         testsuite/gas/i386/x86-64-avx-scalar.s,
> >>         testsuite/gas/i386/x86-64-avx.s,
> >>         testsuite/gas/i386/x86-64-bundle.s,
> >>         testsuite/gas/i386/x86-64-intel64.s,
> >>         testsuite/gas/i386/x86-64-lock-1.s,
> >>         testsuite/gas/i386/x86-64-opcode.s,
> >>         testsuite/gas/i386/x86-64-sse2avx.s,
> >>         testsuite/gas/i386/x86-64-sse3.s: Add missing suffixes.
> >>         * testsuite/gas/i386/nops.s, testsuite/gas/i386/sse-noavx.s,
> >>         testsuite/gas/i386/x86-64-nops.s,
> >>         testsuite/gas/i386/x86-64-ptwrite.s,
> >>         testsuite/gas/i386/x86-64-simd.s,
> >>         testsuite/gas/i386/x86-64-sse-noavx.s,
> >>         testsuite/gas/i386/x86-64-suffix.s: Drop bogus suffix-less
> >>         insns.
> >>         * testsuite/gas/i386/noreg16.s, testsuite/gas/i386/noreg32.s,
> >>         testsuite/gas/i386/noreg64.s: Add further tests.
> >>         * testsuite/gas/i386/ilp32/x86-64-nops.d,
> >>         testsuite/gas/i386/nops.d, testsuite/gas/i386/noreg16.d,
> >>         testsuite/gas/i386/noreg32.d, testsuite/gas/i386/noreg64.d,
> >>         testsuite/gas/i386/sse-noavx.d,
> >>         testsuite/gas/i386/x86-64-intel64.d,
> >>         testsuite/gas/i386/x86-64-nops.d,
> >>         testsuite/gas/i386/x86-64-opcode.d,
> >>         testsuite/gas/i386/x86-64-ptwrite-intel.d,
> >>         testsuite/gas/i386/x86-64-ptwrite.d,
> >>         testsuite/gas/i386/x86-64-simd-intel.d,
> >>         testsuite/gas/i386/x86-64-simd-suffix.d,
> >>         testsuite/gas/i386/x86-64-simd.d,
> >>         testsuite/gas/i386/x86-64-sse-noavx.d
> >>         testsuite/gas/i386/x86-64-suffix.d,
> >>         testsuite/gas/i386/x86-64-suffix-intel.d: Adjust expectations.
> >>         * testsuite/gas/i386/noreg16.l, testsuite/gas/i386/noreg32.l,
> >>         testsuite/gas/i386/noreg64.l: New.
> >>         * testsuite/gas/i386/i386.exp: Run new tests.
> >>
> >> opcodes/
> >> 2020-01-XX  Jan Beulich  <jbeulich@suse.com>
> >>
> >>         * i386-opc.tbl (sysret): Drop DefaultSize.
> >>         * i386-tbl.h: Re-generate.
> >> ---
> >> v3: Don't add any new DefaultSize. Also diagnose SYSRET in 64-bit mode.
> >>     Re-base.
> >> v2: Re-base over NOP addition to testcases modified here. Add IN/OUT
> >>     tests. Add shift/rotate insn single operand tests. Extend
> >>     "Instruction Naming" doc section to spell out defaults.
> >
> > Since we don't change encoding, we shouldn't drop any tests.  If a warning is
> > a problem, we need to create new tests to silence the warning.
>
> Could you give a concrete example of a truly dropped test? Some pieces
> get moved, but I view this as fulfilling "create new tests to silence
> the warning".
>

The patch is OK.

Thanks.


-- 
H.J.