Re: [PATCH v2 5/9] x86: improve handling of insns with ambiguous operand sizes

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

On Fri, Nov 8, 2019 at 12:09 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.11.2019 18:47,  H.J. Lu  wrote:
> > On Thu, Nov 7, 2019 at 2:27 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 06.11.2019 23:54,  H.J. Lu  wrote:
> >>> On Mon, Nov 4, 2019 at 11:45 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 04.11.2019 18:12, H.J. Lu wrote:
> >>>>> On Mon, Nov 4, 2019 at 2:29 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>
> >>>>>> On 31.10.2019 18:26,  H.J. Lu  wrote:
> >>>>>>> On Thu, Oct 31, 2019 at 2:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>
> >>>>>>>> On 31.10.2019 00:57, H.J. Lu wrote:
> >>>>>>>>> On Wed, Oct 30, 2019 at 12:59 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 29.10.2019 18:55,  H.J. Lu  wrote:
> >>>>>>>>>>> On Mon, Oct 28, 2019 at 1:05 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. The set of
> >>>>>>>>>>>> "DefaultSize" ones gets slightly widened for the purposes here.
> >>>>>>>>>>>
> >>>>>>>>>>> What is the advantage to add DefaultSize vs the alternative?
> >>>>>>>>>>
> >>>>>>>>>> I don't know what alternative you refer to; if you mean some
> >>>>>>>>>> hypothetical one, then the advantage of simply adding
> >>>>>>>>>> DefaultSize as done here is likely that it allows to not add or
> >>>>>>>>>> further complicate logic in tc-i386*.c. Furthermore the ones which
> >>>>>>>>>> get the attribute added should have had it already before, if the
> >>>>>>>>>> comment "default insn size depends on mode" is to be trusted.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> DefaultSize is added to some instructions and then they are excluded:
> >>>>>>>>>
> >>>>>>>>> +          /* exclude jmp/ljmp */
> >>>>>>>>> +          && strcmp (i.tm.name, "jmp") && strcmp (i.tm.name, "ljmp")
> >>>>>>>>> +          /* exclude byte-displacement jumps */
> >>>>>>>>> +          && !i.tm.opcode_modifier.jumpbyte
> >>>>>>>>> +          /* exclude lgdt/lidt/sgdt/sidt */
> >>>>>>>>> +          && (i.tm.base_opcode != 0x0f01 || i.tm.extension_opcode > 3)
> >>>>>>>>>            /* exclude fldenv/frstor/fsave/fstenv */
> >>>>>>>>>            && i.tm.opcode_modifier.no_ssuf)
> >>>>>>>>>
> >>>>>>>>> It looks odd.
> >>>>>>>>
> >>>>>>>> But this isn't the only place where defaultsize gets evaluated.
> >>>>>>>> See how lgdt/lidt/sgdt/sidt already have the attribute in the
> >>>>>>>> opcode table, but need exclusion here now too. The alternative
> >>>>>>>> would be two independent attributes - one to be evaluated here,
> >>>>>>>> and the other to be evaluated further down in the function. Yet
> >>>>>>>> again - this dual use has been there before, and just needs
> >>>>>>>> suitable extending of the logic now.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Normally instructions with DefaultSize have i.suffix unset.  Except with
> >>>>>>> .code16gcc, which is used to support 16-bit mode with 32-bit address,
> >>>>>>> i.suffix is set to 'l' for 32-bit address.
> >>>>>>
> >>>>>> I don't follow you here: Since when is there a connection between
> >>>>>> 'l' suffix and addressing mode? All .code16gcc distinguishes from
> >>>>>> plain .code16 is stack pointer width, isn't it? In which case
> >>>>>> using fldenv etc in their 32-bit operand size form looks wrong.
> >>>>>
> >>>>> fldenv doesn't use 32-bit operand size.
> >>>>
> >>>> Then what is it that DefaultSize is needed for on its template?
> >>>>
> >>>>>> Or is this behavior firmly documented? The main gas documentation
> >>>>>> certainly doesn't, afaics.
> >>>>>
> >>>>> I don't think code16gcc is well documented.
> >>>>
> >>>> Which is not very helpful.
> >>>>
> >>>>>>> However, iret/fldenv/frstor/fsave/fstenv
> >>>>>>> are exceptions since they need 16-bit variants.  So we need 2 different
> >>>>>>> DefaultSize behaviors for .code16gcc, one uses LONG_MNEM_SUFFIX
> >>>>>>> and the other uses WORD_MNEM_SUFFIX.  We should update
> >>>>>>> DefaultSize to properly encode iret/fldenv/frstor/fsave/fstenv for
> >>>>>>> .code16gcc, instead of checking i.tm.opcode_modifier.no_ssuf.
> >>>>>>> Something like this.
> >>>>>>
> >>>>>> Plausible, but still afaict orthogonal to what I'm doing here, and
> >>>>>> what you look to be unhappy about.
> >>>>>
> >>>>> DefaultSize only impacts .code16gcc.   Why adding DefaultSize to these
> >>>>> instructions?
> >>>>
> >>>> There are two uses in process_suffix(), and only one of them is
> >>>> .coge16gcc related afaict. The other also affects 64-bit mode,
> >>>> or else I don't understand why various Cpu64 templates also have
> >>>> the attribute.
> >>>
> >>> DefaultSize makes no difference on Cpu64 push/pop in AT&T syntax.
> >>> It is only used by Intel syntax.  In AT&T syntax, only i.tm.opcode_modifier.w
> >>> instructions need suffix.   I don't think we should add DefaultSize to more
> >>> instructions.
> >>
> >> What I continue to miss is what you suggest as an alternative. Did
> >> you mean to commit that other change, widening the attribute to 2
> >> bits, and you'd then expect me to re-base over it? Aiui this would
> >> improve the situation, but not necessarily avoid adding exceptions
> >> (I'd have to check if it actually does). I'd also like to note that
> >> your mention of only push/pop for 64-bit looks incomplete to me -
> >> as said, call, ret, enter etc also have that attribute.
> >
> > I don't think DefaultSize matters for them either in AT&T syntax.
> > and I don't think we should add DefaultSize to more instructions.
>
> Then _again_ - what is your alternative suggestion?
>

Don't add DefaultSize to more instructions.


-- 
H.J.