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

[Jan Beulich <jbeulich at suse dot com>]

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.

Jan