[PATCH] Add xgetbv/xsetbv instructions support for precord.

[PATCH] Add xgetbv/xsetbv instructions support for precord.

[Jiang Jilin]

2009-10-09  Jiang Jilin  <freephp@gmail.com>

	* i386-tdep.c (i386_process_record): Add xgetbv/xsetbv instructions support
---
 gdb/i386-tdep.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index b79bcd2..1efe07f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -5172,6 +5172,19 @@ reswitch:
 	  break;
 	  /* lgdt */
 	case 2:
+	  if (ir.mod == 3)
+	    {
+	      /* xgetbv */
+	      if (ir.rm == 0)
+		{
+		  I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REAX_REGNUM);
+		  I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDX_REGNUM);
+		  break;
+		}
+	      /* xsetbv */
+	      else if (ir.rm == 1)
+		break;
+	    }
 	  /* lidt */
 	case 3:
 	  if (ir.mod == 3)
-- 
1.5.4.3

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Joel Brobecker]

> 2009-10-09  Jiang Jilin  <freephp@gmail.com>
> 
> 	* i386-tdep.c (i386_process_record): Add xgetbv/xsetbv instructions support

I will trust you on the instruction analysis and testing of your patch.
If you ran the testsuite to verify that there is no regression before
and after your patch (please confirm), then this patch is approved.
Just one minor nit: The line in the ChangeLog needs to be split as
it goes beyond 80 columns.

I could not locate you in our FSF assignment database. Do you have
an FSF assignment on file? If not, I don't think we can take this
contribution until you do (too large to be taken as an obvious change
or what we call a "tiny change").  Let us know whether you do or do not,
and whether you'd like to get you started on the paperwork.  It takes
a few weeks, so should we need it, sooner would be better than later.

-- 
Joel

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Michael Snyder]

Jiang Jilin wrote:
> 2009-10-09  Jiang Jilin  <freephp@gmail.com>
> 
> 	* i386-tdep.c (i386_process_record): Add xgetbv/xsetbv instructions support
> ---
>  gdb/i386-tdep.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index b79bcd2..1efe07f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -5172,6 +5172,19 @@ reswitch:
>  	  break;
>  	  /* lgdt */
>  	case 2:
> +	  if (ir.mod == 3)
> +	    {
> +	      /* xgetbv */
> +	      if (ir.rm == 0)
> +		{
> +		  I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REAX_REGNUM);
> +		  I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDX_REGNUM);
> +		  break;
> +		}
> +	      /* xsetbv */
> +	      else if (ir.rm == 1)
> +		break;
> +	    }
>  	  /* lidt */
>  	case 3:
>  	  if (ir.mod == 3)

Jiang (is that how you would like to be addressed?)

This change looks OK to me in principle, but I'm not qualified
to judge its technical correctness.  I would like to get Hui
Zhu's opinion.

Nice job on the formatting and coding conventions!
;-)

Michael

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Joel Brobecker]

> Sorry, but I don't know how to verify it, please help me with it. When
> I ran 'make check', it displayed some 'FAIL' messages even before my
> patch, and I noticed runtest uses
> /usr/share/dejagnu/baseboards/unix.exp, which is not given by
> http://sourceware.org/gdb/wiki/ReversibleDebugging.  How to use
> runtest to test precord.exp only?

I will let Michael and Hui explain to you how to use the special
board file precord.exp.

The FAILs are expected, as is simply impossible to mark all failures
for all platforms.  There are too many variables, such as your compiler,
your kernel, etc etc etc.  So what you need to do is run the testsuite
before and after your patch, saving the contents of gdb.sum after the
first run, and then do a comparison of the two files, to make sure
that your patch did not cause any unexpected change.  Some tests are
a little unpredictable, and cause you to see some apparent regressions
that may not be caused by your patch. You'll have to use your judgement
in these cases.

> No, I don't have an FSF  assignment on file, but I'd like to. How to
> have an FSF assignment on file?

I'll send you the form to fill in and send to the FSF immediately.

-- 
Joel

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Jiang Jilin]

On Sat, Oct 10, 2009 at 10:33 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Sorry, but I don't know how to verify it, please help me with it. When
>> I ran 'make check', it displayed some 'FAIL' messages even before my
>> patch, and I noticed runtest uses
>> /usr/share/dejagnu/baseboards/unix.exp, which is not given by
>> http://sourceware.org/gdb/wiki/ReversibleDebugging.  How to use
>> runtest to test precord.exp only?
>
> I will let Michael and Hui explain to you how to use the special
> board file precord.exp.

OK. Thank you!

Another question: when to verify whether regression occurs, is 'make
check' enough? or have to test all other *.exp in /usr/share/dejagnu/baseboards?

> The FAILs are expected, as is simply impossible to mark all failures
> for all platforms.  There are too many variables, such as your compiler,
> your kernel, etc etc etc.  So what you need to do is run the testsuite
> before and after your patch, saving the contents of gdb.sum after the
> first run, and then do a comparison of the two files, to make sure
> that your patch did not cause any unexpected change.  Some tests are
> a little unpredictable, and cause you to see some apparent regressions
> that may not be caused by your patch. You'll have to use your judgement
> in these cases.

I got it, the key is diff the gdb.sum.

>> No, I don't have an FSF  assignment on file, but I'd like to. How to
>> have an FSF assignment on file?
>
> I'll send you the form to fill in and send to the FSF immediately.
>

Thank you very much!

> Joel
>



-- 
Jiang

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Joel Brobecker]

> Another question: when to verify whether regression occurs, is 'make
> check' enough? or have to test all other *.exp in
> /usr/share/dejagnu/baseboards?

No, just once run before and one run after with the same board file
should be enough.

Just because I'm not familiar with these board files, wouldn't it
have been nice to have the information directly embedded somewhere
in the GDB testsuite library? At AdaCore, our own gdb-testsuite
knows what each target supports, so it knows whether a test might
be relevant or not.  From what I can tell, the testsuite does not
run the precord testcases unless I use the precord.exp board file.

-- 
Joel

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Hui Zhu]

Thanks Jilin, Michael and Joel.

I think this patch is very good.  We can find the "xgetbv" and
"xsetbv" in "Intel® 64 and IA-32 Architectures Software Developer’s
Manual Volume 2B: Instruction Set Reference, N-Z" Vol. 2B 4-509 and
Vol. 2B 4-529.
And I test this patch with reverse test, everything is OK.

BTW, I think the prec code that handle "0f 01" is not very well.
Jilin, do you interest with make this code better?

Thanks,
Hui



On Sat, Oct 10, 2009 at 10:33, Joel Brobecker <brobecker@adacore.com> wrote:
>> Sorry, but I don't know how to verify it, please help me with it. When
>> I ran 'make check', it displayed some 'FAIL' messages even before my
>> patch, and I noticed runtest uses
>> /usr/share/dejagnu/baseboards/unix.exp, which is not given by
>> http://sourceware.org/gdb/wiki/ReversibleDebugging.  How to use
>> runtest to test precord.exp only?
>
> I will let Michael and Hui explain to you how to use the special
> board file precord.exp.
>
> The FAILs are expected, as is simply impossible to mark all failures
> for all platforms.  There are too many variables, such as your compiler,
> your kernel, etc etc etc.  So what you need to do is run the testsuite
> before and after your patch, saving the contents of gdb.sum after the
> first run, and then do a comparison of the two files, to make sure
> that your patch did not cause any unexpected change.  Some tests are
> a little unpredictable, and cause you to see some apparent regressions
> that may not be caused by your patch. You'll have to use your judgement
> in these cases.
>
>> No, I don't have an FSF  assignment on file, but I'd like to. How to
>> have an FSF assignment on file?
>
> I'll send you the form to fill in and send to the FSF immediately.
>
> --
> Joel
>

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Jiang Jilin]

On Sat, Oct 10, 2009 at 3:57 PM, Hui Zhu <teawater@gmail.com> wrote:
> Thanks Jilin, Michael and Joel.
>
> I think this patch is very good.  We can find the "xgetbv" and
> "xsetbv" in "Intel® 64 and IA-32 Architectures Software Developer’s
> Manual Volume 2B: Instruction Set Reference, N-Z" Vol. 2B 4-509 and
> Vol. 2B 4-529.
> And I test this patch with reverse test, everything is OK.
>
> BTW, I think the prec code that handle "0f 01" is not very well.
> Jilin, do you interest with make this code better?

Yes, I'm doing now. :)

Thanks!

> Thanks,
> Hui
>
>
>


-- 
Jiang

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Jiang Jilin]

On Sat, Oct 10, 2009 at 3:57 PM, Hui Zhu <teawater@gmail.com> wrote:
> Thanks Jilin, Michael and Joel.
>
> I think this patch is very good.  We can find the "xgetbv" and
> "xsetbv" in "Intel® 64 and IA-32 Architectures Software Developer’s
> Manual Volume 2B: Instruction Set Reference, N-Z" Vol. 2B 4-509 and
> Vol. 2B 4-529.
> And I test this patch with reverse test, everything is OK.
>
> BTW, I think the prec code that handle "0f 01" is not very well.
> Jilin, do you interest with make this code better?

forget to say, please don't check in the patch right now.

Thanks!

Jiang

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Jiang Jilin]

Hi Michael,

Is this small patch acceptable? If yes, please tell me who can help me
check it in,
you know, I've no write permission.

Thanks!

On Sat, Oct 10, 2009 at 6:22 AM, Michael Snyder <msnyder@vmware.com> wrote:
> Jiang Jilin wrote:
>>
>> 2009-10-09  Jiang Jilin  <freephp@gmail.com>
>>
>>        * i386-tdep.c (i386_process_record): Add xgetbv/xsetbv instructions
>> support
>> ---
>>  gdb/i386-tdep.c |   13 +++++++++++++
>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index b79bcd2..1efe07f 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>> @@ -5172,6 +5172,19 @@ reswitch:
>>          break;
>>          /* lgdt */
>>        case 2:
>> +         if (ir.mod == 3)
>> +           {
>> +             /* xgetbv */
>> +             if (ir.rm == 0)
>> +               {
>> +                 I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REAX_REGNUM);
>> +                 I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDX_REGNUM);
>> +                 break;
>> +               }
>> +             /* xsetbv */
>> +             else if (ir.rm == 1)
>> +               break;
>> +           }
>>          /* lidt */
>>        case 3:
>>          if (ir.mod == 3)
>
> Jiang (is that how you would like to be addressed?)
>
> This change looks OK to me in principle, but I'm not qualified
> to judge its technical correctness.  I would like to get Hui
> Zhu's opinion.
>
> Nice job on the formatting and coding conventions!
> ;-)
>
> Michael
>
>
>
>
>



-- 
Jiang

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Michael Snyder]

Jiang Jilin wrote:
> Hi Michael,
> 
> Is this small patch acceptable? If yes, please tell me who can help me
> check it in,
> you know, I've no write permission.

Cool, I think Hui, Joel and I all approved this patch.
I'll check it in -- consider it committed.




> On Sat, Oct 10, 2009 at 6:22 AM, Michael Snyder <msnyder@vmware.com> wrote:
>> Jiang Jilin wrote:
>>> 2009-10-09  Jiang Jilin  <freephp@gmail.com>
>>>
>>>        * i386-tdep.c (i386_process_record): Add xgetbv/xsetbv instructions
>>> support
>>> ---
>>>  gdb/i386-tdep.c |   13 +++++++++++++
>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>>> index b79bcd2..1efe07f 100644
>>> --- a/gdb/i386-tdep.c
>>> +++ b/gdb/i386-tdep.c
>>> @@ -5172,6 +5172,19 @@ reswitch:
>>>          break;
>>>          /* lgdt */
>>>        case 2:
>>> +         if (ir.mod == 3)
>>> +           {
>>> +             /* xgetbv */
>>> +             if (ir.rm == 0)
>>> +               {
>>> +                 I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REAX_REGNUM);
>>> +                 I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDX_REGNUM);
>>> +                 break;
>>> +               }
>>> +             /* xsetbv */
>>> +             else if (ir.rm == 1)
>>> +               break;
>>> +           }
>>>          /* lidt */
>>>        case 3:
>>>          if (ir.mod == 3)
>> Jiang (is that how you would like to be addressed?)
>>
>> This change looks OK to me in principle, but I'm not qualified
>> to judge its technical correctness.  I would like to get Hui
>> Zhu's opinion.
>>
>> Nice job on the formatting and coding conventions!
>> ;-)
>>
>> Michael
>>
>>
>>
>>
>>
> 
> 
> 

Re: [PATCH] Add xgetbv/xsetbv instructions support for precord.

[Jiang Jilin]

On Mon, Oct 12, 2009 at 11:38 PM, Michael Snyder <msnyder@vmware.com> wrote:
> Jiang Jilin wrote:
>>
>> Hi Michael,
>>
>> Is this small patch acceptable? If yes, please tell me who can help me
>> check it in,
>> you know, I've no write permission.
>
> Cool, I think Hui, Joel and I all approved this patch.
> I'll check it in -- consider it committed.

Thank you all!

>> On Sat, Oct 10, 2009 at 6:22 AM, Michael Snyder <msnyder@vmware.com>
>> wrote:
>>>
>>> Jiang Jilin wrote:
>>>>
>>>> 2009-10-09  Jiang Jilin  <freephp@gmail.com>
>>>>
>>>>       * i386-tdep.c (i386_process_record): Add xgetbv/xsetbv
>>>> instructions
>>>> support
>>>> ---
>>>>  gdb/i386-tdep.c |   13 +++++++++++++
>>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>>>> index b79bcd2..1efe07f 100644
>>>> --- a/gdb/i386-tdep.c
>>>> +++ b/gdb/i386-tdep.c
>>>> @@ -5172,6 +5172,19 @@ reswitch:
>>>>         break;
>>>>         /* lgdt */
>>>>       case 2:
>>>> +         if (ir.mod == 3)
>>>> +           {
>>>> +             /* xgetbv */
>>>> +             if (ir.rm == 0)
>>>> +               {
>>>> +                 I386_RECORD_ARCH_LIST_ADD_REG
>>>> (X86_RECORD_REAX_REGNUM);
>>>> +                 I386_RECORD_ARCH_LIST_ADD_REG
>>>> (X86_RECORD_REDX_REGNUM);
>>>> +                 break;
>>>> +               }
>>>> +             /* xsetbv */
>>>> +             else if (ir.rm == 1)
>>>> +               break;
>>>> +           }
>>>>         /* lidt */
>>>>       case 3:
>>>>         if (ir.mod == 3)
>>>
>>> Jiang (is that how you would like to be addressed?)
>>>
>>> This change looks OK to me in principle, but I'm not qualified
>>> to judge its technical correctness.  I would like to get Hui
>>> Zhu's opinion.
>>>
>>> Nice job on the formatting and coding conventions!
>>> ;-)
>>>
>>> Michael
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>



-- 
Jiang