[PATCH] x86: Incorrect vmgexit entry in prefix_table

[Jan Beulich]

On 17.06.2020 17:42, H.J. Lu wrote:
> On Wed, Jun 17, 2020 at 8:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 17.06.2020 16:32, H.J. Lu wrote:
>>> On Wed, Jun 17, 2020 at 7:17 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 17.06.2020 16:11, H.J. Lu wrote:
>>>>> On Wed, Jun 17, 2020 at 7:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 17.06.2020 16:02, H.J. Lu wrote:
>>>>>>> On Tue, Jun 16, 2020 at 7:11 AM H.J. Lu via Binutils
>>>>>>> <binutils@sourceware.org> wrote:
>>>>>>>>
>>>>>>>> From: "Cui,Lili" <lili.cui@intel.com>
>>>>>>>>
>>>>>>>>         * i386-dis.c (prefix_table): Delete the incorrect vmgexit.
>>>>>>>> ---
>>>>>>>>  opcodes/i386-dis.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
>>>>>>>> index 441866d6c97..55e595a4be0 100644
>>>>>>>> --- a/opcodes/i386-dis.c
>>>>>>>> +++ b/opcodes/i386-dis.c
>>>>>>>> @@ -3577,7 +3577,7 @@ static const struct dis386 prefix_table[][4] = {
>>>>>>>>      { "vmmcall",       { Skip_MODRM }, 0 },
>>>>>>>>      { "vmgexit",       { Skip_MODRM }, 0 },
>>>>>>>>      { Bad_Opcode },
>>>>>>>> -    { "vmgexit",       { Skip_MODRM }, 0 },
>>>>>>>> +    { Bad_Opcode },
>>>>>>>>    },
>>>>>>>>
>>>>>>>>    /* PREFIX_0F01_REG_5_MOD_0 */
>>>>>>>> --
>>>>>>>> 2.26.2
>>>>>>>>
>>>>>>>
>>>>>>> This is the patch I am checking in.
>>>>>>
>>>>>> But both patches are wrong - as per the PM there are two encodings,
>>>>>> i.e. either prefix is valid. Afaics from the quoted text above (I
>>>>>> didn't get to see the original mail, presumably due to email issues
>>>>>> over here) there's no justification here at all why the second
>>>>>> entry would be invalid. Please revert.
>>>>>
>>>>> Please add a testcase to verify it.
>>>>
>>>> I didn't at the time I added support for it primarily because
>>>> there's no way to sensibly express this to gas. And I'm very
>>>> hesitant to add .byte-based tests (except when wanting to test
>>>> the disassembler against invalid encodings) ... In any event
>>>> the bad commit needs reverting first, or any such test added
>>>> would fail. And also in any event it would have been prudent
>>>> to check why things are the way they are, before "fixing" them.
>>>> At that point one of the two of you could have decided to add
>>>> such a test to your liking.
>>>
>>> All entries in dis-i386.c should have at least one test.  Currently
>>> "make check" is clean.
>>
>> And a test like what you ask for can't be added without breaking
>> it. Please undo the breakage, independent of the possible test
>> case addition (to be honest, I have better uses of my time).
>>
> 
> Testcase does 2 things:
> 
> 1. Verify implementation.
> 2. Prevent accidental change.
> 
> Without a testcase indicates it isn't important.

Do you really want me to go through and point out what else has
no testcase, yet quite certainly _is_ important?

Jan