This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v2 5/9] x86: improve handling of insns with ambiguous operand sizes
On 08.11.2019 16:54, H.J. Lu wrote:
> 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., excuse me. This doesn't help the situation. I'm not adding these
attributes just for the fun of it. If I'm not to add them, I need some
other solution to whatever breaks without them (I'm sorry, this is 3
or 4 year old code, so I don't recall exactly what doesn't work anymore
without adding any one of them). What I'm really not looking forward to
doing are further opcode-based special cases elsewhere in the code.
Jan