x86: Avoid abort on invalid broadcast

Message ID 20210819140259.93178-1-hjl.tools@gmail.com
State New
Headers show
Series
  • x86: Avoid abort on invalid broadcast
Related show

Commit Message

Alan Modra via Binutils Aug. 19, 2021, 2:02 p.m.
Print "{bad}" on invalid broadcast instead of abort.

gas/

	PR binutils/28247
	* testsuite/gas/i386/bad-bcast.d: New file.
	* testsuite/gas/i386/bad-bcast.s: Likewise.
	* testsuite/gas/i386/i386.exp: Run bad-bcast.

opcodes/

	PR binutils/28247
	* i386-dis.c (OP_E_memory): Print "{bad}" on invalid broadcast
	instead of abort.
---
 gas/testsuite/gas/i386/bad-bcast.d | 14 ++++++++++++++
 gas/testsuite/gas/i386/bad-bcast.s |  2 ++
 gas/testsuite/gas/i386/i386.exp    |  1 +
 opcodes/i386-dis.c                 |  8 ++++----
 4 files changed, 21 insertions(+), 4 deletions(-)
 create mode 100644 gas/testsuite/gas/i386/bad-bcast.d
 create mode 100644 gas/testsuite/gas/i386/bad-bcast.s

-- 
2.31.1

Comments

Alan Modra via Binutils Aug. 19, 2021, 2:18 p.m. | #1
On 19.08.2021 16:02, H.J. Lu via Binutils wrote:
> --- /dev/null

> +++ b/gas/testsuite/gas/i386/bad-bcast.d

> @@ -0,0 +1,14 @@

> +#objdump: -dw

> +#name: Disassemble bad broadcast

> +

> +.*: +file format .*

> +

> +

> +Disassembly of section .text:

> +

> +0+ <.text>:

> + +[a-f0-9]+:	62                   	.byte 0x62

> + +[a-f0-9]+:	c3                   	ret    

> + +[a-f0-9]+:	8c 1d 66 90 66 90    	mov    %ds,0x90669066

> + +[a-f0-9]+:	66 90                	xchg   %ax,%ax


Urgh, yet another piece of rubbish. Where's the "{bad}" that
you're supposedly printing? This is the 0f3a encoding space if
I'm not mistaken, so we know the encoding length no matter
whether the encoding is actually valid. We'd better not resort
to ".byte" in that case, or if we do, then all bytes of the
encoding should be consumed.

> --- /dev/null

> +++ b/gas/testsuite/gas/i386/bad-bcast.s

> @@ -0,0 +1,2 @@

> +	.text

> +	.byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90


Would you mind adding a comment indicating what this resembles?

> --- a/opcodes/i386-dis.c

> +++ b/opcodes/i386-dis.c

> @@ -11912,7 +11912,7 @@ OP_E_memory (int bytemode, int sizeflag)

>          {

>            if (vex.w)

>              {

> -              abort ();

> +	      oappend ("{bad}");


I can see that this is encoding dependent, so indeed shouldn't be
abort().

> @@ -11928,7 +11928,7 @@ OP_E_memory (int bytemode, int sizeflag)

>                    oappend ("{1to32}");

>                    break;

>                  default:

> -                  abort ();

> +		  oappend ("{bad}");

>                  }

>              }

>          }

> @@ -11948,7 +11948,7 @@ OP_E_memory (int bytemode, int sizeflag)

>  	      oappend ("{1to8}");

>  	      break;

>  	    default:

> -	      abort ();

> +	      oappend ("{bad}");

>  	    }

>  	}

>        else if (bytemode == x_mode

> @@ -11966,7 +11966,7 @@ OP_E_memory (int bytemode, int sizeflag)

>  	      oappend ("{1to16}");

>  	      break;

>  	    default:

> -	      abort ();

> +	      oappend ("{bad}");

>  	    }

>  	}

>        else


All of these, otoh, assume that EVEX.L'L=3 was filtered out earlier,
so I think abort() is legitimate there.

Jan
Alan Modra via Binutils Aug. 19, 2021, 2:45 p.m. | #2
On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

> > --- /dev/null

> > +++ b/gas/testsuite/gas/i386/bad-bcast.d

> > @@ -0,0 +1,14 @@

> > +#objdump: -dw

> > +#name: Disassemble bad broadcast

> > +

> > +.*: +file format .*

> > +

> > +

> > +Disassembly of section .text:

> > +

> > +0+ <.text>:

> > + +[a-f0-9]+: 62                      .byte 0x62

> > + +[a-f0-9]+: c3                      ret

> > + +[a-f0-9]+: 8c 1d 66 90 66 90       mov    %ds,0x90669066

> > + +[a-f0-9]+: 66 90                   xchg   %ax,%ax

>

> Urgh, yet another piece of rubbish. Where's the "{bad}" that

> you're supposedly printing? This is the 0f3a encoding space if

> I'm not mistaken, so we know the encoding length no matter

> whether the encoding is actually valid. We'd better not resort

> to ".byte" in that case, or if we do, then all bytes of the

> encoding should be consumed.


This can use some improvements.

> > --- /dev/null

> > +++ b/gas/testsuite/gas/i386/bad-bcast.s

> > @@ -0,0 +1,2 @@

> > +     .text

> > +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

>

> Would you mind adding a comment indicating what this resembles?


I will add some comments.

> > --- a/opcodes/i386-dis.c

> > +++ b/opcodes/i386-dis.c

> > @@ -11912,7 +11912,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >          {

> >            if (vex.w)

> >              {

> > -              abort ();

> > +           oappend ("{bad}");

>

> I can see that this is encoding dependent, so indeed shouldn't be

> abort().

>

> > @@ -11928,7 +11928,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >                    oappend ("{1to32}");

> >                    break;

> >                  default:

> > -                  abort ();

> > +               oappend ("{bad}");

> >                  }

> >              }

> >          }

> > @@ -11948,7 +11948,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >             oappend ("{1to8}");

> >             break;

> >           default:

> > -           abort ();

> > +           oappend ("{bad}");

> >           }

> >       }

> >        else if (bytemode == x_mode

> > @@ -11966,7 +11966,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >             oappend ("{1to16}");

> >             break;

> >           default:

> > -           abort ();

> > +           oappend ("{bad}");

> >           }

> >       }

> >        else

>

> All of these, otoh, assume that EVEX.L'L=3 was filtered out earlier,

> so I think abort() is legitimate there.


I am putting 3 aborts.

> Jan

>


Thanks.

-- 
H.J.
From ca22cf5ed52c1b4c40dbadf893f558ef09d0c66b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 19 Aug 2021 07:39:10 -0700
Subject: [PATCH] x86: Put back 3 aborts in OP_E_memory

Put back 3 aborts where invalid lengths should have been filtered out.

gas/

	PR binutils/28247
	* testsuite/gas/i386/bad-bcast.s: Add a comment.

opcodes/

	PR binutils/28247
	* * i386-dis.c (OP_E_memory): Put back 3 aborts.
---
 gas/testsuite/gas/i386/bad-bcast.s | 1 +
 opcodes/i386-dis.c                 | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gas/testsuite/gas/i386/bad-bcast.s b/gas/testsuite/gas/i386/bad-bcast.s
index e09c3aae5de..3e49b2238ed 100644
--- a/gas/testsuite/gas/i386/bad-bcast.s
+++ b/gas/testsuite/gas/i386/bad-bcast.s
@@ -1,2 +1,3 @@
 	.text
+# Invalid 16-bit broadcast with EVEX.W == 1.
 	.byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index acb5a0faa88..aa292233d4d 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -11928,7 +11928,7 @@ OP_E_memory (int bytemode, int sizeflag)
                   oappend ("{1to32}");
                   break;
                 default:
-		  oappend ("{bad}");
+		  abort ();
                 }
             }
         }
@@ -11948,7 +11948,7 @@ OP_E_memory (int bytemode, int sizeflag)
 	      oappend ("{1to8}");
 	      break;
 	    default:
-	      oappend ("{bad}");
+	      abort ();
 	    }
 	}
       else if (bytemode == x_mode
@@ -11966,7 +11966,7 @@ OP_E_memory (int bytemode, int sizeflag)
 	      oappend ("{1to16}");
 	      break;
 	    default:
-	      oappend ("{bad}");
+	      abort ();
 	    }
 	}
       else
Alan Modra via Binutils Aug. 19, 2021, 3:27 p.m. | #3
On 19.08.2021 16:45, H.J. Lu wrote:
> On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:

>> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

>>> --- /dev/null

>>> +++ b/gas/testsuite/gas/i386/bad-bcast.s

>>> @@ -0,0 +1,2 @@

>>> +     .text

>>> +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

>>

>> Would you mind adding a comment indicating what this resembles?

> 

> I will add some comments.


Hmm, I guess I wasn't explicit enough: While the comment you add
now states the EVEX.W aspect, what I was after is to save readers
from having to look up what opcode this is and hence why broadcast
is invalid here with EVEX.W in the first place. Due to the rubbish
the disassembler produces its output isn't any help here either.
So what I was looking to have is a commented instruction with
operands (which the assembler would reject), plus expressing of
the EVEX.W aspect in whatever way is suitable. And I actually
wonder whether the insn you've chosen isn't invalid even without
broadcast, but with EVEX.W set. In which case I wouldn't be
convinced of the usefulness of the test - it would become invalid
once EVEX.W gains a meaning for the major opcode in question,
would be removed at that point, and the logic in question wouldn't
be tested anymore at all.

>>> --- a/opcodes/i386-dis.c

>>> +++ b/opcodes/i386-dis.c

>>> @@ -11912,7 +11912,7 @@ OP_E_memory (int bytemode, int sizeflag)

>>>          {

>>>            if (vex.w)

>>>              {

>>> -              abort ();

>>> +           oappend ("{bad}");

>>

>> I can see that this is encoding dependent, so indeed shouldn't be

>> abort().

>>

>>> @@ -11928,7 +11928,7 @@ OP_E_memory (int bytemode, int sizeflag)

>>>                    oappend ("{1to32}");

>>>                    break;

>>>                  default:

>>> -                  abort ();

>>> +               oappend ("{bad}");

>>>                  }

>>>              }

>>>          }

>>> @@ -11948,7 +11948,7 @@ OP_E_memory (int bytemode, int sizeflag)

>>>             oappend ("{1to8}");

>>>             break;

>>>           default:

>>> -           abort ();

>>> +           oappend ("{bad}");

>>>           }

>>>       }

>>>        else if (bytemode == x_mode

>>> @@ -11966,7 +11966,7 @@ OP_E_memory (int bytemode, int sizeflag)

>>>             oappend ("{1to16}");

>>>             break;

>>>           default:

>>> -           abort ();

>>> +           oappend ("{bad}");

>>>           }

>>>       }

>>>        else

>>

>> All of these, otoh, assume that EVEX.L'L=3 was filtered out earlier,

>> so I think abort() is legitimate there.

> 

> I am putting 3 aborts.


Thanks.

Jan
Alan Modra via Binutils Aug. 19, 2021, 3:35 p.m. | #4
On Thu, Aug 19, 2021 at 8:27 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 19.08.2021 16:45, H.J. Lu wrote:

> > On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:

> >> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

> >>> --- /dev/null

> >>> +++ b/gas/testsuite/gas/i386/bad-bcast.s

> >>> @@ -0,0 +1,2 @@

> >>> +     .text

> >>> +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

> >>

> >> Would you mind adding a comment indicating what this resembles?

> >

> > I will add some comments.

>

> Hmm, I guess I wasn't explicit enough: While the comment you add

> now states the EVEX.W aspect, what I was after is to save readers

> from having to look up what opcode this is and hence why broadcast

> is invalid here with EVEX.W in the first place. Due to the rubbish

> the disassembler produces its output isn't any help here either.

> So what I was looking to have is a commented instruction with

> operands (which the assembler would reject), plus expressing of

> the EVEX.W aspect in whatever way is suitable. And I actually

> wonder whether the insn you've chosen isn't invalid even without

> broadcast, but with EVEX.W set. In which case I wouldn't be

> convinced of the usefulness of the test - it would become invalid

> once EVEX.W gains a meaning for the major opcode in question,

> would be removed at that point, and the logic in question wouldn't

> be tested anymore at all.


The input bytes aren't valid x86 opcodes.   But abort isn't nice.
If people run into the this, ".byte" should give a clue for invalid
opcodes.

> >>> --- a/opcodes/i386-dis.c

> >>> +++ b/opcodes/i386-dis.c

> >>> @@ -11912,7 +11912,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >>>          {

> >>>            if (vex.w)

> >>>              {

> >>> -              abort ();

> >>> +           oappend ("{bad}");

> >>

> >> I can see that this is encoding dependent, so indeed shouldn't be

> >> abort().

> >>

> >>> @@ -11928,7 +11928,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >>>                    oappend ("{1to32}");

> >>>                    break;

> >>>                  default:

> >>> -                  abort ();

> >>> +               oappend ("{bad}");

> >>>                  }

> >>>              }

> >>>          }

> >>> @@ -11948,7 +11948,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >>>             oappend ("{1to8}");

> >>>             break;

> >>>           default:

> >>> -           abort ();

> >>> +           oappend ("{bad}");

> >>>           }

> >>>       }

> >>>        else if (bytemode == x_mode

> >>> @@ -11966,7 +11966,7 @@ OP_E_memory (int bytemode, int sizeflag)

> >>>             oappend ("{1to16}");

> >>>             break;

> >>>           default:

> >>> -           abort ();

> >>> +           oappend ("{bad}");

> >>>           }

> >>>       }

> >>>        else

> >>

> >> All of these, otoh, assume that EVEX.L'L=3 was filtered out earlier,

> >> so I think abort() is legitimate there.

> >

> > I am putting 3 aborts.

>

> Thanks.

>

> Jan

>



-- 
H.J.
Alan Modra via Binutils Aug. 19, 2021, 4:03 p.m. | #5
On 19.08.2021 17:35, H.J. Lu wrote:
> On Thu, Aug 19, 2021 at 8:27 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 19.08.2021 16:45, H.J. Lu wrote:

>>> On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

>>>>> --- /dev/null

>>>>> +++ b/gas/testsuite/gas/i386/bad-bcast.s

>>>>> @@ -0,0 +1,2 @@

>>>>> +     .text

>>>>> +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

>>>>

>>>> Would you mind adding a comment indicating what this resembles?

>>>

>>> I will add some comments.

>>

>> Hmm, I guess I wasn't explicit enough: While the comment you add

>> now states the EVEX.W aspect, what I was after is to save readers

>> from having to look up what opcode this is and hence why broadcast

>> is invalid here with EVEX.W in the first place. Due to the rubbish

>> the disassembler produces its output isn't any help here either.

>> So what I was looking to have is a commented instruction with

>> operands (which the assembler would reject), plus expressing of

>> the EVEX.W aspect in whatever way is suitable. And I actually

>> wonder whether the insn you've chosen isn't invalid even without

>> broadcast, but with EVEX.W set. In which case I wouldn't be

>> convinced of the usefulness of the test - it would become invalid

>> once EVEX.W gains a meaning for the major opcode in question,

>> would be removed at that point, and the logic in question wouldn't

>> be tested anymore at all.

> 

> The input bytes aren't valid x86 opcodes.   But abort isn't nice.

> If people run into the this, ".byte" should give a clue for invalid

> opcodes.


So a comment along the lines of

# "vfpclasspsx $0x90, (%eax){1to8}, %k0" but with EVEX.W set

is not helpful, you would think?

Jan
Alan Modra via Binutils Aug. 19, 2021, 4:22 p.m. | #6
On Thu, Aug 19, 2021 at 9:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 19.08.2021 17:35, H.J. Lu wrote:

> > On Thu, Aug 19, 2021 at 8:27 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 19.08.2021 16:45, H.J. Lu wrote:

> >>> On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

> >>>>> --- /dev/null

> >>>>> +++ b/gas/testsuite/gas/i386/bad-bcast.s

> >>>>> @@ -0,0 +1,2 @@

> >>>>> +     .text

> >>>>> +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

> >>>>

> >>>> Would you mind adding a comment indicating what this resembles?

> >>>

> >>> I will add some comments.

> >>

> >> Hmm, I guess I wasn't explicit enough: While the comment you add

> >> now states the EVEX.W aspect, what I was after is to save readers

> >> from having to look up what opcode this is and hence why broadcast

> >> is invalid here with EVEX.W in the first place. Due to the rubbish

> >> the disassembler produces its output isn't any help here either.

> >> So what I was looking to have is a commented instruction with

> >> operands (which the assembler would reject), plus expressing of

> >> the EVEX.W aspect in whatever way is suitable. And I actually

> >> wonder whether the insn you've chosen isn't invalid even without

> >> broadcast, but with EVEX.W set. In which case I wouldn't be

> >> convinced of the usefulness of the test - it would become invalid

> >> once EVEX.W gains a meaning for the major opcode in question,

> >> would be removed at that point, and the logic in question wouldn't

> >> be tested anymore at all.

> >

> > The input bytes aren't valid x86 opcodes.   But abort isn't nice.

> > If people run into the this, ".byte" should give a clue for invalid

> > opcodes.

>

> So a comment along the lines of

>

> # "vfpclasspsx $0x90, (%eax){1to8}, %k0" but with EVEX.W set

>

> is not helpful, you would think?

>


No.   We need a different byte sequence for vfpclasspsx since the
first 2 bytes, 0x62, 0xc3, are invalid.

-- 
H.J.
Alan Modra via Binutils Aug. 20, 2021, 10:58 a.m. | #7
On 19.08.2021 18:22, H.J. Lu wrote:
> On Thu, Aug 19, 2021 at 9:03 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 19.08.2021 17:35, H.J. Lu wrote:

>>> On Thu, Aug 19, 2021 at 8:27 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> On 19.08.2021 16:45, H.J. Lu wrote:

>>>>> On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

>>>>>>> --- /dev/null

>>>>>>> +++ b/gas/testsuite/gas/i386/bad-bcast.s

>>>>>>> @@ -0,0 +1,2 @@

>>>>>>> +     .text

>>>>>>> +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

>>>>>>

>>>>>> Would you mind adding a comment indicating what this resembles?

>>>>>

>>>>> I will add some comments.

>>>>

>>>> Hmm, I guess I wasn't explicit enough: While the comment you add

>>>> now states the EVEX.W aspect, what I was after is to save readers

>>>> from having to look up what opcode this is and hence why broadcast

>>>> is invalid here with EVEX.W in the first place. Due to the rubbish

>>>> the disassembler produces its output isn't any help here either.

>>>> So what I was looking to have is a commented instruction with

>>>> operands (which the assembler would reject), plus expressing of

>>>> the EVEX.W aspect in whatever way is suitable. And I actually

>>>> wonder whether the insn you've chosen isn't invalid even without

>>>> broadcast, but with EVEX.W set. In which case I wouldn't be

>>>> convinced of the usefulness of the test - it would become invalid

>>>> once EVEX.W gains a meaning for the major opcode in question,

>>>> would be removed at that point, and the logic in question wouldn't

>>>> be tested anymore at all.

>>>

>>> The input bytes aren't valid x86 opcodes.   But abort isn't nice.

>>> If people run into the this, ".byte" should give a clue for invalid

>>> opcodes.

>>

>> So a comment along the lines of

>>

>> # "vfpclasspsx $0x90, (%eax){1to8}, %k0" but with EVEX.W set

>>

>> is not helpful, you would think?

>>

> 

> No.   We need a different byte sequence for vfpclasspsx since the

> first 2 bytes, 0x62, 0xc3, are invalid.


But that's part of the problem: There are way too many things that
are all invalid at the same time for this encoding. Hence it is not
a concise test of the one issue you actually want to test. I've
passed the sequence (with the one missing opcode byte added as 0xcc)
to my disassembler, and I get

EVEX.128.0F3A.W1:66 R18{k5}, R14, [r8-6F996F9A]{1toN}, CC

So the issues are (at least)
- the missing immediate byte
- the destination being outside the %k0-%k7 range
- the embedded prefix not representing the 0x66 legacy one
- EVEX.VVVV not being 0b1111
- EVEX.W set

Any change to the logic detecting any of the other invalid aspects
will therefore risk to invalidate the testcase.

Jan
Alan Modra via Binutils Aug. 20, 2021, 2:04 p.m. | #8
On Fri, Aug 20, 2021 at 3:59 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 19.08.2021 18:22, H.J. Lu wrote:

> > On Thu, Aug 19, 2021 at 9:03 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 19.08.2021 17:35, H.J. Lu wrote:

> >>> On Thu, Aug 19, 2021 at 8:27 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> On 19.08.2021 16:45, H.J. Lu wrote:

> >>>>> On Thu, Aug 19, 2021 at 7:18 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>> On 19.08.2021 16:02, H.J. Lu via Binutils wrote:

> >>>>>>> --- /dev/null

> >>>>>>> +++ b/gas/testsuite/gas/i386/bad-bcast.s

> >>>>>>> @@ -0,0 +1,2 @@

> >>>>>>> +     .text

> >>>>>>> +     .byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90

> >>>>>>

> >>>>>> Would you mind adding a comment indicating what this resembles?

> >>>>>

> >>>>> I will add some comments.

> >>>>

> >>>> Hmm, I guess I wasn't explicit enough: While the comment you add

> >>>> now states the EVEX.W aspect, what I was after is to save readers

> >>>> from having to look up what opcode this is and hence why broadcast

> >>>> is invalid here with EVEX.W in the first place. Due to the rubbish

> >>>> the disassembler produces its output isn't any help here either.

> >>>> So what I was looking to have is a commented instruction with

> >>>> operands (which the assembler would reject), plus expressing of

> >>>> the EVEX.W aspect in whatever way is suitable. And I actually

> >>>> wonder whether the insn you've chosen isn't invalid even without

> >>>> broadcast, but with EVEX.W set. In which case I wouldn't be

> >>>> convinced of the usefulness of the test - it would become invalid

> >>>> once EVEX.W gains a meaning for the major opcode in question,

> >>>> would be removed at that point, and the logic in question wouldn't

> >>>> be tested anymore at all.

> >>>

> >>> The input bytes aren't valid x86 opcodes.   But abort isn't nice.

> >>> If people run into the this, ".byte" should give a clue for invalid

> >>> opcodes.

> >>

> >> So a comment along the lines of

> >>

> >> # "vfpclasspsx $0x90, (%eax){1to8}, %k0" but with EVEX.W set

> >>

> >> is not helpful, you would think?

> >>

> >

> > No.   We need a different byte sequence for vfpclasspsx since the

> > first 2 bytes, 0x62, 0xc3, are invalid.

>

> But that's part of the problem: There are way too many things that

> are all invalid at the same time for this encoding. Hence it is not

> a concise test of the one issue you actually want to test. I've

> passed the sequence (with the one missing opcode byte added as 0xcc)

> to my disassembler, and I get

>

> EVEX.128.0F3A.W1:66 R18{k5}, R14, [r8-6F996F9A]{1toN}, CC

>

> So the issues are (at least)

> - the missing immediate byte

> - the destination being outside the %k0-%k7 range

> - the embedded prefix not representing the 0x66 legacy one

> - EVEX.VVVV not being 0b1111

> - EVEX.W set

>

> Any change to the logic detecting any of the other invalid aspects

> will therefore risk to invalidate the testcase.


Correct.  The comment is mainly for binutils developers.

# Invalid 16-bit broadcast with EVEX.W == 1.

is good enough.


-- 
H.J.

Patch

diff --git a/gas/testsuite/gas/i386/bad-bcast.d b/gas/testsuite/gas/i386/bad-bcast.d
new file mode 100644
index 00000000000..9fc474a42ff
--- /dev/null
+++ b/gas/testsuite/gas/i386/bad-bcast.d
@@ -0,0 +1,14 @@ 
+#objdump: -dw
+#name: Disassemble bad broadcast
+
+.*: +file format .*
+
+
+Disassembly of section .text:
+
+0+ <.text>:
+ +[a-f0-9]+:	62                   	.byte 0x62
+ +[a-f0-9]+:	c3                   	ret    
+ +[a-f0-9]+:	8c 1d 66 90 66 90    	mov    %ds,0x90669066
+ +[a-f0-9]+:	66 90                	xchg   %ax,%ax
+#pass
diff --git a/gas/testsuite/gas/i386/bad-bcast.s b/gas/testsuite/gas/i386/bad-bcast.s
new file mode 100644
index 00000000000..e09c3aae5de
--- /dev/null
+++ b/gas/testsuite/gas/i386/bad-bcast.s
@@ -0,0 +1,2 @@ 
+	.text
+	.byte 0x62, 0xc3, 0x8c, 0x1d, 0x66, 0x90, 0x66, 0x90, 0x66, 0x90
diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
index f5eda2cf331..80959726d0e 100644
--- a/gas/testsuite/gas/i386/i386.exp
+++ b/gas/testsuite/gas/i386/i386.exp
@@ -646,6 +646,7 @@  if [gas_32_check] then {
 	run_dump_test "dw2-compress-2"
 	run_dump_test "dw2-compressed-2"
 
+	run_dump_test "bad-bcast"
 	run_dump_test "bad-size"
 
 	run_dump_test "size-1"
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 2c7027ca6f1..acb5a0faa88 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -11912,7 +11912,7 @@  OP_E_memory (int bytemode, int sizeflag)
         {
           if (vex.w)
             {
-              abort ();
+	      oappend ("{bad}");
             }
           else
             {
@@ -11928,7 +11928,7 @@  OP_E_memory (int bytemode, int sizeflag)
                   oappend ("{1to32}");
                   break;
                 default:
-                  abort ();
+		  oappend ("{bad}");
                 }
             }
         }
@@ -11948,7 +11948,7 @@  OP_E_memory (int bytemode, int sizeflag)
 	      oappend ("{1to8}");
 	      break;
 	    default:
-	      abort ();
+	      oappend ("{bad}");
 	    }
 	}
       else if (bytemode == x_mode
@@ -11966,7 +11966,7 @@  OP_E_memory (int bytemode, int sizeflag)
 	      oappend ("{1to16}");
 	      break;
 	    default:
-	      abort ();
+	      oappend ("{bad}");
 	    }
 	}
       else