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

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

[H.J. Lu]

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.
	* testsuite/gas/i386/jump16.s: Add IRET tests.
	* testsuite/gas/i386/jump16.d: Updated.
---
 gas/ChangeLog                   | 8 ++++++++
 gas/config/tc-i386.c            | 6 ++++++
 gas/testsuite/gas/i386/jump16.d | 2 ++
 gas/testsuite/gas/i386/jump16.s | 6 ++++++
 4 files changed, 22 insertions(+)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index 8558ecd7b4..c94ffdda0b 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,11 @@
+2019-04-26  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR gas/24485
+	* config/tc-i386.c (process_suffix): Don't add DATA_PREFIX_OPCODE
+	to IRET for .code16gcc.
+	* testsuite/gas/i386/jump16.s: Add IRET tests.
+	* testsuite/gas/i386/jump16.d: Updated.
+
 2019-04-25  Alexandre Oliva  <aoliva@redhat.com>
 	    Alan Modra  <amodra@gmail.com>
 
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 5eb6c4c269..032b1d3081 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -6352,7 +6352,13 @@ process_suffix (void)
 	    if (!add_prefix (ADDR_PREFIX_OPCODE))
 	      return 0;
 	}
+      /* stackop_size is set to LONG_MNEM_SUFFIX for the .code16gcc
+	 directive to support 16bit mode with 32-bit address.  Since
+	 IRET (opcode 0xcf) in 16bit mode returns from an interrupt
+	 in 16bit mode, we shouldn't add DATA_PREFIX_OPCODE here.  */
       else if (i.suffix != QWORD_MNEM_SUFFIX
+	       && (stackop_size != LONG_MNEM_SUFFIX
+		   || i.tm.base_opcode != 0xcf)
 	       && !i.tm.opcode_modifier.ignoresize
 	       && !i.tm.opcode_modifier.floatmf
 	       && !i.tm.opcode_modifier.vex
diff --git a/gas/testsuite/gas/i386/jump16.d b/gas/testsuite/gas/i386/jump16.d
index a83bad6157..7a1dc7661e 100644
--- a/gas/testsuite/gas/i386/jump16.d
+++ b/gas/testsuite/gas/i386/jump16.d
@@ -67,4 +67,6 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	ea 10 10 90 90       	ljmp   \$0x9090,\$0x1010
 [ 	]*[a-f0-9]+:	ea 00 00 90 90       	ljmp   \$0x9090,\$0x0	ed: (R_386_)?16	xxx
 [ 	]*[a-f0-9]+:	ea 00 00 90 90       	ljmp   \$0x9090,\$0x0	f2: (R_386_)?16	xxx
+[ 	]*[a-f0-9]+:	cf                   	iret   
+[ 	]*[a-f0-9]+:	cf                   	iret   
 #pass
diff --git a/gas/testsuite/gas/i386/jump16.s b/gas/testsuite/gas/i386/jump16.s
index f8bc0ea5b4..aff5665720 100644
--- a/gas/testsuite/gas/i386/jump16.s
+++ b/gas/testsuite/gas/i386/jump16.s
@@ -71,3 +71,9 @@
 	jmp	0x9090:0x1010
 	jmp	0x9090,xxx
 	jmp	0x9090:xxx
+
+.code16gcc
+	iret
+
+.code16
+	iret
-- 
2.20.1

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

[Jan Beulich]

>>> 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:
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.

In fact I think the better solution would be to reject ambiguous
code by demanding a suffix in all cases in .code16gcc mode.

Jan

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

[H.J. Lu]

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:

[hjl@gnu-cfl-1 tmp]$ cat foo.c
extern void bar (void);

__attribute__((interrupt))
void
foo (void *frame)
{
  bar ();
}
[hjl@gnu-cfl-1 tmp]$ gcc -O2 -mgeneral-regs-only -m16 -S foo.c
[hjl@gnu-cfl-1 tmp]$ cat foo.s
.file "foo.c"
.code16gcc
.text
.p2align 4,,15
.globl foo
.type foo, @function
foo:
.LFB0:
.cfi_startproc
pushl %ebp
.cfi_def_cfa_offset 8
.cfi_offset 5, -8
movl %esp, %ebp
.cfi_def_cfa_register 5
pushl %ecx
pushl %edx
pushl %eax
andl $-16, %esp
.cfi_offset 1, -12
.cfi_offset 2, -16
.cfi_offset 0, -20
cld
call bar
leal -12(%ebp), %esp
popl %eax
.cfi_restore 0
popl %edx
.cfi_restore 2
popl %ecx
.cfi_restore 1
popl %ebp
.cfi_restore 5
.cfi_def_cfa 4, 4
iret
.cfi_endproc
.LFE0:
.size foo, .-foo
.ident "GCC: (GNU) 8.3.1 20190311 (Red Hat 8.3.1-3)"
.section .note.GNU-stack,"",@progbits
[hjl@gnu-cfl-1 tmp]$

> 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.

> 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.


-- 
H.J.

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

[Jan Beulich]

>>> 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?

>> 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.

Jan

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

[H.J. Lu]

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?

> >> 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.

-- 
H.J.

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

[Jan Beulich]

>>> 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

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

[H.J. Lu]

On Mon, Apr 29, 2019 at 9:11 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> 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.
>

So the specific case is

1. Programming in 16-bit mode with GCC using "gcc -m16".
2. Manually create a 32-bit stack frame for a function with 32-bit iret.
3. Implement such a function with .code16gcc and "iret".
4. Jump to such a function.

-- 
H.J.

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

[Jan Beulich]

>>> On 29.04.19 at 19:13, <hjl.tools@gmail.com> wrote:
> On Mon, Apr 29, 2019 at 9:11 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> 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.
>>
> 
> So the specific case is
> 
> 1. Programming in 16-bit mode with GCC using "gcc -m16".
> 2. Manually create a 32-bit stack frame for a function with 32-bit iret.
> 3. Implement such a function with .code16gcc and "iret".
> 4. Jump to such a function.

Yes. And note that the PUSHes potentially involved in step 2
default to 32-bit operand size, i.e. there's then a mixture of
requirements as to whether suffixes are needed on the insns.

In the end, just like for your address size override fix for
scatter/gather insns a few weeks back, I think this is again a
case which would better be fixed in the compiler (making it
emit the needed suffix) than in the assembler.

Jan

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3863 bytes --]

On Mon, Apr 29, 2019 at 11:44 PM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 29.04.19 at 19:13, <hjl.tools@gmail.com> wrote:
> > On Mon, Apr 29, 2019 at 9:11 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> >>> 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.
> >>
> >
> > So the specific case is
> >
> > 1. Programming in 16-bit mode with GCC using "gcc -m16".
> > 2. Manually create a 32-bit stack frame for a function with 32-bit iret.
> > 3. Implement such a function with .code16gcc and "iret".
> > 4. Jump to such a function.
>
> Yes. And note that the PUSHes potentially involved in step 2
> default to 32-bit operand size, i.e. there's then a mixture of
> requirements as to whether suffixes are needed on the insns.
>
> In the end, just like for your address size override fix for
> scatter/gather insns a few weeks back, I think this is again a
> case which would better be fixed in the compiler (making it
> emit the needed suffix) than in the assembler.
>

I am checking in this patch.

-- 
H.J.

[-- Attachment #2: 0001-i386-Issue-a-warning-to-IRET-without-suffix-for-.cod.patch --]
[-- Type: text/x-patch, Size: 3952 bytes --]

From dd74e760b9faffbcd057721960c25507e9784215 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 30 Apr 2019 08:40:42 -0700
Subject: [PATCH] i386: Issue a warning to IRET without suffix for .code16gcc

The .code16gcc directive to support 16-bit mode with 32-bit address.
For IRET without a suffix, generate 16-bit IRET with a warning to
return from an interrupt handler in 16-bit mode.

	PR gas/24485
	* config/tc-i386.c (process_suffix): Issue a warning to IRET
	without a suffix for .code16gcc.
	* testsuite/gas/i386/jump16.s: Add tests for iretX.
	* testsuite/gas/i386/jump16.d: Updated.
	* testsuite/gas/i386/jump16.e: New file.
---
 gas/config/tc-i386.c            | 20 +++++++++++++-------
 gas/testsuite/gas/i386/jump16.d |  5 +++++
 gas/testsuite/gas/i386/jump16.e |  3 +++
 gas/testsuite/gas/i386/jump16.s | 16 ++++++++++++++++
 4 files changed, 37 insertions(+), 7 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/jump16.e

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 032b1d3081..7159432f13 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -6242,7 +6242,19 @@ process_suffix (void)
 	   /* exclude fldenv/frstor/fsave/fstenv */
 	   && i.tm.opcode_modifier.no_ssuf)
     {
-      i.suffix = stackop_size;
+      if (stackop_size == LONG_MNEM_SUFFIX
+	  && i.tm.base_opcode == 0xcf)
+	{
+	  /* stackop_size is set to LONG_MNEM_SUFFIX for the
+	     .code16gcc directive to support 16-bit mode with
+	     32-bit address.  For IRET without a suffix, generate
+	     16-bit IRET (opcode 0xcf) to return from an interrupt
+	     handler.  */
+	  i.suffix = WORD_MNEM_SUFFIX;
+	  as_warn (_("generating 16-bit `iret' for .code16gcc directive"));
+	}
+      else
+	i.suffix = stackop_size;
     }
   else if (intel_syntax
 	   && !i.suffix
@@ -6352,13 +6364,7 @@ process_suffix (void)
 	    if (!add_prefix (ADDR_PREFIX_OPCODE))
 	      return 0;
 	}
-      /* stackop_size is set to LONG_MNEM_SUFFIX for the .code16gcc
-	 directive to support 16bit mode with 32-bit address.  Since
-	 IRET (opcode 0xcf) in 16bit mode returns from an interrupt
-	 in 16bit mode, we shouldn't add DATA_PREFIX_OPCODE here.  */
       else if (i.suffix != QWORD_MNEM_SUFFIX
-	       && (stackop_size != LONG_MNEM_SUFFIX
-		   || i.tm.base_opcode != 0xcf)
 	       && !i.tm.opcode_modifier.ignoresize
 	       && !i.tm.opcode_modifier.floatmf
 	       && !i.tm.opcode_modifier.vex
diff --git a/gas/testsuite/gas/i386/jump16.d b/gas/testsuite/gas/i386/jump16.d
index 7a1dc7661e..c883eba0e8 100644
--- a/gas/testsuite/gas/i386/jump16.d
+++ b/gas/testsuite/gas/i386/jump16.d
@@ -1,5 +1,6 @@
 #name: i386 jump16
 #objdump: -drw -mi8086
+#warning_output: jump16.e
 
 .*:     file format .*i386.*
 
@@ -69,4 +70,8 @@ Disassembly of section .text:
 [ 	]*[a-f0-9]+:	ea 00 00 90 90       	ljmp   \$0x9090,\$0x0	f2: (R_386_)?16	xxx
 [ 	]*[a-f0-9]+:	cf                   	iret   
 [ 	]*[a-f0-9]+:	cf                   	iret   
+[ 	]*[a-f0-9]+:	66 cf                	iretl  
+[ 	]*[a-f0-9]+:	cf                   	iret   
+[ 	]*[a-f0-9]+:	cf                   	iret   
+[ 	]*[a-f0-9]+:	66 cf                	iretl  
 #pass
diff --git a/gas/testsuite/gas/i386/jump16.e b/gas/testsuite/gas/i386/jump16.e
new file mode 100644
index 0000000000..2ad7ea056b
--- /dev/null
+++ b/gas/testsuite/gas/i386/jump16.e
@@ -0,0 +1,3 @@
+.*: Assembler messages:
+.*:77: Warning: generating 16-bit `iret' for .code16gcc directive
+.*:88: Warning: generating 16-bit `iret' for .code16gcc directive
diff --git a/gas/testsuite/gas/i386/jump16.s b/gas/testsuite/gas/i386/jump16.s
index aff5665720..fb9e830c52 100644
--- a/gas/testsuite/gas/i386/jump16.s
+++ b/gas/testsuite/gas/i386/jump16.s
@@ -72,8 +72,24 @@
 	jmp	0x9090,xxx
 	jmp	0x9090:xxx
 
+	.att_syntax
 .code16gcc
 	iret
+	iretw
+	iretl
 
 .code16
 	iret
+	iretw
+	iretl
+
+	.intel_syntax noprefix
+.code16gcc
+	iret
+	iretw
+	iretd
+
+.code16
+	iret
+	iretw
+	iretd
-- 
2.20.1

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

[Jan Beulich]

>>> On 30.04.19 at 17:48, <hjl.tools@gmail.com> wrote:
> On Mon, Apr 29, 2019 at 11:44 PM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 29.04.19 at 19:13, <hjl.tools@gmail.com> wrote:
>> > On Mon, Apr 29, 2019 at 9:11 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >>
>> >> >>> 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.
>> >>
>> >
>> > So the specific case is
>> >
>> > 1. Programming in 16-bit mode with GCC using "gcc -m16".
>> > 2. Manually create a 32-bit stack frame for a function with 32-bit iret.
>> > 3. Implement such a function with .code16gcc and "iret".
>> > 4. Jump to such a function.
>>
>> Yes. And note that the PUSHes potentially involved in step 2
>> default to 32-bit operand size, i.e. there's then a mixture of
>> requirements as to whether suffixes are needed on the insns.
>>
>> In the end, just like for your address size override fix for
>> scatter/gather insns a few weeks back, I think this is again a
>> case which would better be fixed in the compiler (making it
>> emit the needed suffix) than in the assembler.
>>
> 
> I am checking in this patch.

Thanks much.

Jan