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

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

On Thu, Aug 8, 2019 at 12:56 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 08.08.2019 01:58,  H.J. Lu  wrote:
> > On Wed, Aug 7, 2019 at 1:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 06.08.2019 23:38,  H.J. Lu  wrote:
> >>> On Tue, Aug 6, 2019 at 1:26 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>
> >>>> On Tue, Aug 6, 2019 at 7:27 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).
> >>>>
> >>>> If there may be an ambiguity, a size suffix can be used.  Assembler isn't
> >>>> responsible for programmer's errors.
> >>>>
> >>>>> 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
> >>>>
> >>>> This punishes the perfectly good assembly sources.
> >>>>
> >>>>> 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. The set of
> >>>>> "DefaultSize" one gets slightly widened for the purposes here.
> >>>>
> >>>> Widen the default size set avoids generate warnings.  It sounds to
> >>>> me these warnings isn't really necessary.
> >>>>
> >>>>> 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 emitting of ambiguous operands (and hence triggering of the new
> >>>>> warning).
> >>>>>
> >>>>> Note 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.
> >>>>>
> >>>
> >>> Instruction suffix has been an issue with AT&T syntax.  But improvements
> >>> shouldn't assemble current working assembly sources cleanly.
> >>
> >> I guess you mean "should", not "shouldn't"? And "cleanly" to me does
> >> not imply without warnings, just without change to generated code
> >> (unless the generated code was outright wrong). Pointing out possible
> >> issues should not be restricted to cases that didn't assemble without
> >> error so far. Or else this would be one more argument against e.g.
> >> your recent "REP;" handling adjustment.
> >>
> >>>   It is reasonable
> >>> to require that programmers should know what they are doing.  We should
> >>>
> >>> 1.  Require suffix if there may be ambiguity.
> >>
> >> This would actively break existing code (reading "require" as "emit an
> >> error if it is missing"). After all it's the present _inconsistency_
> >> in behavior that this series tries to address.
> >>
> >>> 2.  Generate a warning if needed under a new option, (-mambiguity-check=?).
> >>
> >> First of all this contradicts 1 above: There's no point generating a
> >> warning when we also generate an error. As to a separate option - if
> >> you really don't want me to re-use the existing one (which is a pretty
> >> good fit), I can certainly key this to a new one. But the default will
> >> remain to be for the warning to be enabled.
> >>
> >>> 3.  Document the default operand size for AT&T syntax if there may be ambiguity.
> >>
> >> Will do.
> >>
> >
> > Since AT&T syntax has no size info on memory operand, we can't tell memory
> > size when mnemonic supports more than one memory size.   Currently AT&T
> > syntax adds suffix to distinguish different memory sizes for these instructions.
> > For some instructions, there is a default memory size when there is no suffix.
> > In this sense, there is no ambiguity for assembler.   The only issue is that it
> > isn't easy for programmer to tell what the default memory size is.
> >
> > To help programmers with these ambiguities when there is no suffix, we can
> >
> > 1. Document these ambiguities.
> > 2. Issue warning when there is an exception in rules.   Warning should
> > be kept as few as possible.   For most parts, the default size is 32-bit.
>
> "For most parts" is already telling enough, I think. Recall how
>
>         shl     %cl, (%eax)
>
> used to be wrongly deriving byte memory operand size from the
> shift count register used? If things had been working flawlessly
> so far, I'd maybe agree that there's no need for warnings by
> default. But seeing how inconsistent _and broken_ things have
> been, this is simply not an option.
>
> > add     $1, (%bx)
> >
> > shouldn't generate a warning since the destination is 32-bit.
>
> Good example (but of course I don't agree with the "no warning"
> statement). Why is it then that
>
>         add     $0x89, (%eax)
>         add     $0x12345678, (%eax)
>
> fail to assemble without the patch applied? Once again - may I
> ask that you look at the additions to noreg*.s that this and
> the subsequent patches do? These are all cases not working
> currently, but which should work (and according to you even
> without warning), and which this series makes work.

Given that in AT&T syntax needs the suffix for memory operand size,
we should document existing quirks and shouldn't accept more
instructions without suffix.

> >  We can add
> > a pseudo prefix {imm32} to request 32-bit immediate operand.
>
> I don't see how this relates here.
>

We can encode

{imm32} addl   $0x70,(%rax)

with 32-bit immediate.

-- 
H.J.