| Message ID | 20200616141131.21396-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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. -- H.J. From 6fde587ff78a54b9e3bd70259de60cc5d7d8ced7 Mon Sep 17 00:00:00 2001 From: "Cui,Lili" <lili.cui@intel.com> Date: Tue, 16 Jun 2020 07:11:31 -0700 Subject: [PATCH] x86: Delete incorrect vmgexit entry in prefix_table * i386-dis.c (prefix_table): Delete the incorrect vmgexit. --- opcodes/ChangeLog | 4 ++++ opcodes/i386-dis.c | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog index a6adf7126d..f604f6e3f7 100644 --- a/opcodes/ChangeLog +++ b/opcodes/ChangeLog @@ -1,3 +1,7 @@ +2020-06-17 Lili Cui <lili.cui@intel.com> + + * i386-dis.c (prefix_table): Delete the incorrect vmgexit. + 2020-06-14 H.J. Lu <hongjiu.lu@intel.com> PR gas/26115 diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c index 441866d6c9..6ac1d7416a 100644 --- a/opcodes/i386-dis.c +++ b/opcodes/i386-dis.c @@ -3576,8 +3576,6 @@ static const struct dis386 prefix_table[][4] = { { { "vmmcall", { Skip_MODRM }, 0 }, { "vmgexit", { Skip_MODRM }, 0 }, - { Bad_Opcode }, - { "vmgexit", { Skip_MODRM }, 0 }, }, /* PREFIX_0F01_REG_5_MOD_0 */
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. Jan
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. -- H.J.
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. Jan
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. -- H.J.
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). Jan
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. You can take your time. -- H.J.
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
On Wed, Jun 17, 2020 at 8:50 AM Jan Beulich <jbeulich@suse.com> wrote: > > 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? There are many old entries without testcases. I have been adding them over time. But all new entries should have at least one testcase. -- H.J.
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 */
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(-) -- 2.26.2