Re: [PATCH] i386: Don't add 0x66 prefix to IRET for .code16gcc

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 29.04.19 at 18:02, <hjl.tools@gmail.com> wrote:
> On Mon, Apr 29, 2019 at 8:25 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 29.04.19 at 17:09, <hjl.tools@gmail.com> wrote:
>> > On Mon, Apr 29, 2019 at 12:01 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 26.04.19 at 19:22, <hjl.tools@gmail.com> wrote:
>> >> > The .code16gcc directive supports 16bit mode with 32-bit address.  Since
>> >> > IRET (opcode 0xcf) in 16bit mode returns from an interrupt in 16bit mode,
>> >> > we shouldn't add 0x66 prefix for IRET.
>> >> >
>> >> >       PR gas/24485
>> >> >       * config/tc-i386.c (process_suffix): Don't add DATA_PREFIX_OPCODE
>> >> >       to IRET for .code16gcc.
>> >>
>> >> This, at the very least, needs to be accompanied by a warning:
>> >
>> > This patch fixes:
>> >[...]
>> >> As the bug report validly says, the changed behavior is what is
>> >> wanted only "almost always". The report even mentions the
>> >> (supposedly uncommon) case: Code manually building a frame
>> >> and IRETing to it will now be silently(!) broken.
>> >
>> > The .code16gcc directive is to support "gcc -m16".   Any other purposes
>> > are not supported.
>>
>> But you realize that people may use inline assembly?
> 
> Inline assembly with the .code16gcc directive in an interrupt
> handler? It is a supported usage?

I don't know, but I see no reasons why it would not be. Note
that I didn't mention "in an interrupt handler" - I can see uses
for manually created frames to IRET to elsewhere.

>> >> In fact I think the better solution would be to reject ambiguous
>> >> code by demanding a suffix in all cases in .code16gcc mode.
>> >
>> > This may break existing codes.
>>
>> Of course, but breaking things at build time (with a proper
>> diagnostic) that's better than silently breaking things at
>> runtime. At the very least you can't claim it would break the
>> supposedly common case, as that was already broken (and
>> hence your fix). So the difference between suggested
>> and current behavior is that right now there's silent latent
>> breakage, whereas otherwise people would be made aware
>> of there being a problem they need to address by changing
>> some of their code.
> 
> Assembler has no way to know if an assembly sequence is
> correct and it shouldn't issue a warning for "gcc -m16" just
> because the same instruction may be incorrect.

I disagree: In this case, the assembler simply can't decide
whether adding an operand size override is correct. Instead
of silently doing the opposite of what has been done for
many years, it should point out that it needs programmer
guidance.

Jan