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

[Jan Beulich <jbeulich at suse dot com>]

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.

Jan