Re: Bug in i386_process_record?

Re: Bug in i386_process_record?

[Hui Zhu]

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

On Fri, Aug 7, 2009 at 11:39, Michael Snyder<msnyder@vmware.com> wrote:
> Hi Hui,
>
> While experimenting with your dump/load commands, I think I discovered
> a bug in i386_process_record, in the handling of the "string ops"
> and the "rep" prefix.  Looks like we are saving the same data over
> and over in the log.
>
> This was made using the attached sample program.
>
>  (gdb) break main
>    Breakpoint 1 at 0x80483c4: file memrange-reverse.c, line 29.
>  (gdb) run
>    Starting program:
>    Breakpoint 1, main ()
>    29        memset (blob1, 'a', sizeof (blob1));
>  (gdb) record
>  (gdb) next
>    30        blob1[sizeof (blob1) - 1] = '\0';
>  (gdb) record dump
>    Saving recording to file 'rec.27255'
>    Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x26070920)
>  [...]
>  Writing register 7 val 0x0000000008049684 (1 plus 8 plus 16 bytes)
>  Writing memory 0x08049680 (1 plus 8 plus 8 bytes plus 1024 bytes)
>  Writing register 1 val 0x00000000000000ff (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x0000000008049688 (1 plus 8 plus 16 bytes)
>  Writing memory 0x08049684 (1 plus 8 plus 8 bytes plus 1020 bytes)
>  Writing register 1 val 0x00000000000000fe (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x000000000804968c (1 plus 8 plus 16 bytes)
>  Writing memory 0x08049688 (1 plus 8 plus 8 bytes plus 1016 bytes)
>  Writing register 1 val 0x00000000000000fd (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x0000000008049690 (1 plus 8 plus 16 bytes)
>  Writing memory 0x0804968c (1 plus 8 plus 8 bytes plus 1012 bytes)
>  Writing register 1 val 0x00000000000000fc (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x0000000008049694 (1 plus 8 plus 16 bytes)
>  Writing memory 0x08049690 (1 plus 8 plus 8 bytes plus 1008 bytes)
>  Writing register 1 val 0x00000000000000fb (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x0000000008049698 (1 plus 8 plus 16 bytes)
>  Writing memory 0x08049694 (1 plus 8 plus 8 bytes plus 1004 bytes)
>  Writing register 1 val 0x00000000000000fa (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x000000000804969c (1 plus 8 plus 16 bytes)
>  Writing memory 0x08049698 (1 plus 8 plus 8 bytes plus 1000 bytes)
>  Writing register 1 val 0x00000000000000f9 (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  Writing record_end (1 byte)
>  Writing register 7 val 0x00000000080496a0 (1 plus 8 plus 16 bytes)
>  Writing memory 0x0804969c (1 plus 8 plus 8 bytes plus 996 bytes)
>  Writing register 1 val 0x00000000000000f8 (1 plus 8 plus 16 bytes)
>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>  [...]
>
> Altogether there were 256 duplicate entries, each one is
> four bytes shorter than the previous one.
>
>

Hi Michael,

I reproduce about issue.  This is because "i386_process_record" record
rep string insn is not right.
I make a patch for it.

Please help me review it.

Thanks,
Hui

2009-08-10  Hui Zhu  <teawater@gmail.com>

	* record.c (i386_process_record): Remove some error code.

---
 i386-tdep.c |   27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4448,9 +4448,8 @@ reswitch:
       regcache_raw_read_unsigned (ir.regcache,
                                   ir.regmap[X86_RECORD_REDI_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
+      if (ir.aflag)
         {
-          tmpulongest &= 0xffff;
           /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
           if (record_debug)
             printf_unfiltered (_("Process record ignores the memory change "
@@ -4460,27 +4459,9 @@ reswitch:
                                paddress (gdbarch, ir.addr));
         }
       if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
-        {
-          ULONGEST count, eflags;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
-          if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
-            return -1;
-        }
+        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
+      if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
+        return -1;
       if (opcode == 0xa4 || opcode == 0xa5)
         I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
       I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);

[-- Attachment #2: prec-fix-x86-strinsn.txt --]
[-- Type: text/plain, Size: 1938 bytes --]

---
 i386-tdep.c |   27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4448,9 +4448,8 @@ reswitch:
       regcache_raw_read_unsigned (ir.regcache,
                                   ir.regmap[X86_RECORD_REDI_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
+      if (ir.aflag)
         {
-          tmpulongest &= 0xffff;
           /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
           if (record_debug)
             printf_unfiltered (_("Process record ignores the memory change "
@@ -4460,27 +4459,9 @@ reswitch:
                                paddress (gdbarch, ir.addr));
         }
       if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
-        {
-          ULONGEST count, eflags;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
-          if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
-            return -1;
-        }
+        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
+      if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
+        return -1;
       if (opcode == 0xa4 || opcode == 0xa5)
         I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
       I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);

Re: Bug in i386_process_record?

[Michael Snyder]

Yes, this seems to be better.  It records only 4 bytes each time
it is called.

But there seems to be still an off-by-one error?  With the test
program that I provided, we call memset with an argument of
1024, but we actually record 1025 bytes... this code gets hit
257 times, with the last time recording only 1 byte.



Hui Zhu wrote:
> On Fri, Aug 7, 2009 at 11:39, Michael Snyder<msnyder@vmware.com> wrote:
>> Hi Hui,
>>
>> While experimenting with your dump/load commands, I think I discovered
>> a bug in i386_process_record, in the handling of the "string ops"
>> and the "rep" prefix.  Looks like we are saving the same data over
>> and over in the log.
>>
>> This was made using the attached sample program.
>>
>>  (gdb) break main
>>    Breakpoint 1 at 0x80483c4: file memrange-reverse.c, line 29.
>>  (gdb) run
>>    Starting program:
>>    Breakpoint 1, main ()
>>    29        memset (blob1, 'a', sizeof (blob1));
>>  (gdb) record
>>  (gdb) next
>>    30        blob1[sizeof (blob1) - 1] = '\0';
>>  (gdb) record dump
>>    Saving recording to file 'rec.27255'
>>    Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x26070920)
>>  [...]
>>  Writing register 7 val 0x0000000008049684 (1 plus 8 plus 16 bytes)
>>  Writing memory 0x08049680 (1 plus 8 plus 8 bytes plus 1024 bytes)
>>  Writing register 1 val 0x00000000000000ff (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x0000000008049688 (1 plus 8 plus 16 bytes)
>>  Writing memory 0x08049684 (1 plus 8 plus 8 bytes plus 1020 bytes)
>>  Writing register 1 val 0x00000000000000fe (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x000000000804968c (1 plus 8 plus 16 bytes)
>>  Writing memory 0x08049688 (1 plus 8 plus 8 bytes plus 1016 bytes)
>>  Writing register 1 val 0x00000000000000fd (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x0000000008049690 (1 plus 8 plus 16 bytes)
>>  Writing memory 0x0804968c (1 plus 8 plus 8 bytes plus 1012 bytes)
>>  Writing register 1 val 0x00000000000000fc (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x0000000008049694 (1 plus 8 plus 16 bytes)
>>  Writing memory 0x08049690 (1 plus 8 plus 8 bytes plus 1008 bytes)
>>  Writing register 1 val 0x00000000000000fb (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x0000000008049698 (1 plus 8 plus 16 bytes)
>>  Writing memory 0x08049694 (1 plus 8 plus 8 bytes plus 1004 bytes)
>>  Writing register 1 val 0x00000000000000fa (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x000000000804969c (1 plus 8 plus 16 bytes)
>>  Writing memory 0x08049698 (1 plus 8 plus 8 bytes plus 1000 bytes)
>>  Writing register 1 val 0x00000000000000f9 (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  Writing record_end (1 byte)
>>  Writing register 7 val 0x00000000080496a0 (1 plus 8 plus 16 bytes)
>>  Writing memory 0x0804969c (1 plus 8 plus 8 bytes plus 996 bytes)
>>  Writing register 1 val 0x00000000000000f8 (1 plus 8 plus 16 bytes)
>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>  [...]
>>
>> Altogether there were 256 duplicate entries, each one is
>> four bytes shorter than the previous one.
>>
>>
> 
> Hi Michael,
> 
> I reproduce about issue.  This is because "i386_process_record" record
> rep string insn is not right.
> I make a patch for it.
> 
> Please help me review it.
> 
> Thanks,
> Hui
> 
> 2009-08-10  Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (i386_process_record): Remove some error code.
> 
> ---
>  i386-tdep.c |   27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> --- a/i386-tdep.c
> +++ b/i386-tdep.c
> @@ -4448,9 +4448,8 @@ reswitch:
>        regcache_raw_read_unsigned (ir.regcache,
>                                    ir.regmap[X86_RECORD_REDI_REGNUM],
>                                    &tmpulongest);
> -      if (!ir.aflag)
> +      if (ir.aflag)
>          {
> -          tmpulongest &= 0xffff;
>            /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
>            if (record_debug)
>              printf_unfiltered (_("Process record ignores the memory change "
> @@ -4460,27 +4459,9 @@ reswitch:
>                                 paddress (gdbarch, ir.addr));
>          }
>        if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> -        {
> -          ULONGEST count, eflags;
> -          regcache_raw_read_unsigned (ir.regcache,
> -                                      ir.regmap[X86_RECORD_REDI_REGNUM],
> -                                      &count);
> -          if (!ir.aflag)
> -            count &= 0xffff;
> -          regcache_raw_read_unsigned (ir.regcache,
> -                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
> -                                      &eflags);
> -          if ((eflags >> 10) & 0x1)
> -            tmpulongest -= (count - 1) * (1 << ir.ot);
> -          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
> -            return -1;
> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
> -        }
> -      else
> -        {
> -          if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
> -            return -1;
> -        }
> +        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
> +      if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
> +        return -1;
>        if (opcode == 0xa4 || opcode == 0xa5)
>          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);

Re: Bug in i386_process_record?

[Hui Zhu]

Oops, I will try to find it out.

Thanks,
Hui

On Tue, Aug 11, 2009 at 05:57, Michael Snyder<msnyder@vmware.com> wrote:
> Yes, this seems to be better.  It records only 4 bytes each time
> it is called.
>
> But there seems to be still an off-by-one error?  With the test
> program that I provided, we call memset with an argument of
> 1024, but we actually record 1025 bytes... this code gets hit
> 257 times, with the last time recording only 1 byte.
>
>
>
> Hui Zhu wrote:
>>
>> On Fri, Aug 7, 2009 at 11:39, Michael Snyder<msnyder@vmware.com> wrote:
>>>
>>> Hi Hui,
>>>
>>> While experimenting with your dump/load commands, I think I discovered
>>> a bug in i386_process_record, in the handling of the "string ops"
>>> and the "rep" prefix.  Looks like we are saving the same data over
>>> and over in the log.
>>>
>>> This was made using the attached sample program.
>>>
>>>  (gdb) break main
>>>   Breakpoint 1 at 0x80483c4: file memrange-reverse.c, line 29.
>>>  (gdb) run
>>>   Starting program:
>>>   Breakpoint 1, main ()
>>>   29        memset (blob1, 'a', sizeof (blob1));
>>>  (gdb) record
>>>  (gdb) next
>>>   30        blob1[sizeof (blob1) - 1] = '\0';
>>>  (gdb) record dump
>>>   Saving recording to file 'rec.27255'
>>>   Writing 4-byte magic cookie RECORD_FILE_MAGIC (0x26070920)
>>>  [...]
>>>  Writing register 7 val 0x0000000008049684 (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x08049680 (1 plus 8 plus 8 bytes plus 1024 bytes)
>>>  Writing register 1 val 0x00000000000000ff (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x0000000008049688 (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x08049684 (1 plus 8 plus 8 bytes plus 1020 bytes)
>>>  Writing register 1 val 0x00000000000000fe (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x000000000804968c (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x08049688 (1 plus 8 plus 8 bytes plus 1016 bytes)
>>>  Writing register 1 val 0x00000000000000fd (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x0000000008049690 (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x0804968c (1 plus 8 plus 8 bytes plus 1012 bytes)
>>>  Writing register 1 val 0x00000000000000fc (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x0000000008049694 (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x08049690 (1 plus 8 plus 8 bytes plus 1008 bytes)
>>>  Writing register 1 val 0x00000000000000fb (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x0000000008049698 (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x08049694 (1 plus 8 plus 8 bytes plus 1004 bytes)
>>>  Writing register 1 val 0x00000000000000fa (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x000000000804969c (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x08049698 (1 plus 8 plus 8 bytes plus 1000 bytes)
>>>  Writing register 1 val 0x00000000000000f9 (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  Writing record_end (1 byte)
>>>  Writing register 7 val 0x00000000080496a0 (1 plus 8 plus 16 bytes)
>>>  Writing memory 0x0804969c (1 plus 8 plus 8 bytes plus 996 bytes)
>>>  Writing register 1 val 0x00000000000000f8 (1 plus 8 plus 16 bytes)
>>>  Writing register 8 val 0x0000000000587be7 (1 plus 8 plus 16 bytes)
>>>  [...]
>>>
>>> Altogether there were 256 duplicate entries, each one is
>>> four bytes shorter than the previous one.
>>>
>>>
>>
>> Hi Michael,
>>
>> I reproduce about issue.  This is because "i386_process_record" record
>> rep string insn is not right.
>> I make a patch for it.
>>
>> Please help me review it.
>>
>> Thanks,
>> Hui
>>
>> 2009-08-10  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (i386_process_record): Remove some error code.
>>
>> ---
>>  i386-tdep.c |   27 ++++-----------------------
>>  1 file changed, 4 insertions(+), 23 deletions(-)
>>
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -4448,9 +4448,8 @@ reswitch:
>>       regcache_raw_read_unsigned (ir.regcache,
>>                                   ir.regmap[X86_RECORD_REDI_REGNUM],
>>                                   &tmpulongest);
>> -      if (!ir.aflag)
>> +      if (ir.aflag)
>>         {
>> -          tmpulongest &= 0xffff;
>>           /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
>>           if (record_debug)
>>             printf_unfiltered (_("Process record ignores the memory change
>> "
>> @@ -4460,27 +4459,9 @@ reswitch:
>>                                paddress (gdbarch, ir.addr));
>>         }
>>       if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> -        {
>> -          ULONGEST count, eflags;
>> -          regcache_raw_read_unsigned (ir.regcache,
>> -                                      ir.regmap[X86_RECORD_REDI_REGNUM],
>> -                                      &count);
>> -          if (!ir.aflag)
>> -            count &= 0xffff;
>> -          regcache_raw_read_unsigned (ir.regcache,
>> -
>>  ir.regmap[X86_RECORD_EFLAGS_REGNUM],
>> -                                      &eflags);
>> -          if ((eflags >> 10) & 0x1)
>> -            tmpulongest -= (count - 1) * (1 << ir.ot);
>> -          if (record_arch_list_add_mem (tmpulongest, count * (1 <<
>> ir.ot)))
>> -            return -1;
>> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>> -        }
>> -      else
>> -        {
>> -          if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>> -            return -1;
>> -        }
>> +        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>> +      if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>> +        return -1;
>>       if (opcode == 0xa4 || opcode == 0xa5)
>>         I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>>       I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>
>

Re: Bug in i386_process_record?

[Hui Zhu]

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

On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com> wrote:
>
> Yes, this seems to be better.  It records only 4 bytes each time
> it is called.
>
> But there seems to be still an off-by-one error?  With the test
> program that I provided, we call memset with an argument of
> 1024, but we actually record 1025 bytes... this code gets hit
> 257 times, with the last time recording only 1 byte.
>
>

Hi Michael,

This issue is because:

0xb7edf4e7 <memset+55>:	rep stos %eax,%es:(%edi)
0xb7edf4e9 <memset+57>:	mov    %edx,%ecx
0xb7edf4eb <memset+59>:	rep stos %al,%es:(%edi)
0xb7edf4ed <memset+61>:	mov    0x8(%esp),%eax
0xb7edf4f1 <memset+65>:	pop    %edi

If the memcpy size is not align with 4, it will handle by second rep stos.
Then rep stos will not execute if %ecx is 0.
i386_process_record doesn't check %ecx, so it get this error.

I make a new patch for it.  Please help me review it.

Thanks,
Hui

2009-08-11  Hui Zhu  <teawater@gmail.com>

	* record.c (i386_process_record): Remove some error code.

---
 i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4441,50 +4441,37 @@ reswitch:
       /* insS */
     case 0x6c:
     case 0x6d:
-      if ((opcode & 1) == 0)
-	ir.ot = OT_BYTE;
-      else
-	ir.ot = ir.dflag + OT_WORD;
       regcache_raw_read_unsigned (ir.regcache,
-                                  ir.regmap[X86_RECORD_REDI_REGNUM],
+                                  ir.regmap[X86_RECORD_RECX_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
-        {
-          tmpulongest &= 0xffff;
-          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
-          if (record_debug)
-            printf_unfiltered (_("Process record ignores the memory change "
-                                 "of instruction at address 0x%s because "
-                                 "it can't get the value of the segment "
-                                 "register.\n"),
-                               paddress (gdbarch, ir.addr));
-        }
-      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+      if (tmpulongest)
         {
-          ULONGEST count, eflags;
+          if ((opcode & 1) == 0)
+	    ir.ot = OT_BYTE;
+          else
+	    ir.ot = ir.dflag + OT_WORD;
           regcache_raw_read_unsigned (ir.regcache,
                                       ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
+                                      &tmpulongest);
+          if (ir.aflag)
+            {
+              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
+              if (record_debug)
+                printf_unfiltered (_("Process record ignores the
memory change "
+                                     "of instruction at address 0x%s because "
+                                     "it can't get the value of the segment "
+                                     "register.\n"),
+                                   paddress (gdbarch, ir.addr));
+            }
+          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
             return -1;
-        }
-      if (opcode == 0xa4 || opcode == 0xa5)
-        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+          if (opcode == 0xa4 || opcode == 0xa5)
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+	}
       break;

       /* cmpsS */

[-- Attachment #2: prec-fix-x86-strinsn.txt --]
[-- Type: text/plain, Size: 3380 bytes --]

---
 i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4441,50 +4441,37 @@ reswitch:
       /* insS */
     case 0x6c:
     case 0x6d:
-      if ((opcode & 1) == 0)
-	ir.ot = OT_BYTE;
-      else
-	ir.ot = ir.dflag + OT_WORD;
       regcache_raw_read_unsigned (ir.regcache,
-                                  ir.regmap[X86_RECORD_REDI_REGNUM],
+                                  ir.regmap[X86_RECORD_RECX_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
-        {
-          tmpulongest &= 0xffff;
-          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
-          if (record_debug)
-            printf_unfiltered (_("Process record ignores the memory change "
-                                 "of instruction at address 0x%s because "
-                                 "it can't get the value of the segment "
-                                 "register.\n"),
-                               paddress (gdbarch, ir.addr));
-        }
-      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+      if (tmpulongest)
         {
-          ULONGEST count, eflags;
+          if ((opcode & 1) == 0)
+	    ir.ot = OT_BYTE;
+          else
+	    ir.ot = ir.dflag + OT_WORD;
           regcache_raw_read_unsigned (ir.regcache,
                                       ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
+                                      &tmpulongest);
+          if (ir.aflag)
+            {
+              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
+              if (record_debug)
+                printf_unfiltered (_("Process record ignores the memory change "
+                                     "of instruction at address 0x%s because "
+                                     "it can't get the value of the segment "
+                                     "register.\n"),
+                                   paddress (gdbarch, ir.addr));
+            }
+          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
             return -1;
-        }
-      if (opcode == 0xa4 || opcode == 0xa5)
-        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+          if (opcode == 0xa4 || opcode == 0xa5)
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+	}
       break;
 
       /* cmpsS */

Re: Bug in i386_process_record?

[Hui Zhu]

Ping

On Tue, Aug 11, 2009 at 14:19, Hui Zhu<teawater@gmail.com> wrote:
> On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com> wrote:
>>
>> Yes, this seems to be better.  It records only 4 bytes each time
>> it is called.
>>
>> But there seems to be still an off-by-one error?  With the test
>> program that I provided, we call memset with an argument of
>> 1024, but we actually record 1025 bytes... this code gets hit
>> 257 times, with the last time recording only 1 byte.
>>
>>
>
> Hi Michael,
>
> This issue is because:
>
> 0xb7edf4e7 <memset+55>: rep stos %eax,%es:(%edi)
> 0xb7edf4e9 <memset+57>: mov    %edx,%ecx
> 0xb7edf4eb <memset+59>: rep stos %al,%es:(%edi)
> 0xb7edf4ed <memset+61>: mov    0x8(%esp),%eax
> 0xb7edf4f1 <memset+65>: pop    %edi
>
> If the memcpy size is not align with 4, it will handle by second rep stos.
> Then rep stos will not execute if %ecx is 0.
> i386_process_record doesn't check %ecx, so it get this error.
>
> I make a new patch for it.  Please help me review it.
>
> Thanks,
> Hui
>
> 2009-08-11  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (i386_process_record): Remove some error code.
>
> ---
>  i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
>  1 file changed, 24 insertions(+), 37 deletions(-)
>
> --- a/i386-tdep.c
> +++ b/i386-tdep.c
> @@ -4441,50 +4441,37 @@ reswitch:
>       /* insS */
>     case 0x6c:
>     case 0x6d:
> -      if ((opcode & 1) == 0)
> -       ir.ot = OT_BYTE;
> -      else
> -       ir.ot = ir.dflag + OT_WORD;
>       regcache_raw_read_unsigned (ir.regcache,
> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>                                   &tmpulongest);
> -      if (!ir.aflag)
> -        {
> -          tmpulongest &= 0xffff;
> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
> -          if (record_debug)
> -            printf_unfiltered (_("Process record ignores the memory change "
> -                                 "of instruction at address 0x%s because "
> -                                 "it can't get the value of the segment "
> -                                 "register.\n"),
> -                               paddress (gdbarch, ir.addr));
> -        }
> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> +      if (tmpulongest)
>         {
> -          ULONGEST count, eflags;
> +          if ((opcode & 1) == 0)
> +           ir.ot = OT_BYTE;
> +          else
> +           ir.ot = ir.dflag + OT_WORD;
>           regcache_raw_read_unsigned (ir.regcache,
>                                       ir.regmap[X86_RECORD_REDI_REGNUM],
> -                                      &count);
> -          if (!ir.aflag)
> -            count &= 0xffff;
> -          regcache_raw_read_unsigned (ir.regcache,
> -                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
> -                                      &eflags);
> -          if ((eflags >> 10) & 0x1)
> -            tmpulongest -= (count - 1) * (1 << ir.ot);
> -          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
> -            return -1;
> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
> -        }
> -      else
> -        {
> +                                      &tmpulongest);
> +          if (ir.aflag)
> +            {
> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
> +              if (record_debug)
> +                printf_unfiltered (_("Process record ignores the
> memory change "
> +                                     "of instruction at address 0x%s because "
> +                                     "it can't get the value of the segment "
> +                                     "register.\n"),
> +                                   paddress (gdbarch, ir.addr));
> +            }
> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>             return -1;
> -        }
> -      if (opcode == 0xa4 || opcode == 0xa5)
> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> +          if (opcode == 0xa4 || opcode == 0xa5)
> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> +       }
>       break;
>
>       /* cmpsS */
>

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com> wrote:
>> Yes, this seems to be better.  It records only 4 bytes each time
>> it is called.
>>
>> But there seems to be still an off-by-one error?  With the test
>> program that I provided, we call memset with an argument of
>> 1024, but we actually record 1025 bytes... this code gets hit
>> 257 times, with the last time recording only 1 byte.
>>
>>
> 
> Hi Michael,
> 
> This issue is because:
> 
> 0xb7edf4e7 <memset+55>:	rep stos %eax,%es:(%edi)
> 0xb7edf4e9 <memset+57>:	mov    %edx,%ecx
> 0xb7edf4eb <memset+59>:	rep stos %al,%es:(%edi)
> 0xb7edf4ed <memset+61>:	mov    0x8(%esp),%eax
> 0xb7edf4f1 <memset+65>:	pop    %edi
> 
> If the memcpy size is not align with 4, it will handle by second rep stos.
> Then rep stos will not execute if %ecx is 0.
> i386_process_record doesn't check %ecx, so it get this error.
> 
> I make a new patch for it.  Please help me review it.

This seems much better.  Please give us a change log and post it for review.

By the way, I'm sorry, I only just realized that I posted two
completely different bug reports with the exact same subject line.
;-(

Cheers,
Michael

Re: Bug in i386_process_record?

[Hui Zhu]

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

On Tue, Aug 18, 2009 at 12:15, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com> wrote:
>>>
>>> Yes, this seems to be better.  It records only 4 bytes each time
>>> it is called.
>>>
>>> But there seems to be still an off-by-one error?  With the test
>>> program that I provided, we call memset with an argument of
>>> 1024, but we actually record 1025 bytes... this code gets hit
>>> 257 times, with the last time recording only 1 byte.
>>>
>>>
>>
>> Hi Michael,
>>
>> This issue is because:
>>
>> 0xb7edf4e7 <memset+55>: rep stos %eax,%es:(%edi)
>> 0xb7edf4e9 <memset+57>: mov    %edx,%ecx
>> 0xb7edf4eb <memset+59>: rep stos %al,%es:(%edi)
>> 0xb7edf4ed <memset+61>: mov    0x8(%esp),%eax
>> 0xb7edf4f1 <memset+65>: pop    %edi
>>
>> If the memcpy size is not align with 4, it will handle by second rep stos.
>> Then rep stos will not execute if %ecx is 0.
>> i386_process_record doesn't check %ecx, so it get this error.
>>
>> I make a new patch for it.  Please help me review it.
>
> This seems much better.  Please give us a change log and post it for review.
>
> By the way, I'm sorry, I only just realized that I posted two
> completely different bug reports with the exact same subject line.
> ;-(
>

Don't worry about it.  Gmail handle it very well.  :)

Thanks,
Hui

2009-08-18  Hui Zhu  <teawater@gmail.com>

	* record.c (i386_process_record): Remove some error code.

---
 i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4441,50 +4441,37 @@ reswitch:
       /* insS */
     case 0x6c:
     case 0x6d:
-      if ((opcode & 1) == 0)
-	ir.ot = OT_BYTE;
-      else
-	ir.ot = ir.dflag + OT_WORD;
       regcache_raw_read_unsigned (ir.regcache,
-                                  ir.regmap[X86_RECORD_REDI_REGNUM],
+                                  ir.regmap[X86_RECORD_RECX_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
-        {
-          tmpulongest &= 0xffff;
-          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
-          if (record_debug)
-            printf_unfiltered (_("Process record ignores the memory change "
-                                 "of instruction at address 0x%s because "
-                                 "it can't get the value of the segment "
-                                 "register.\n"),
-                               paddress (gdbarch, ir.addr));
-        }
-      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+      if (tmpulongest)
         {
-          ULONGEST count, eflags;
+          if ((opcode & 1) == 0)
+	    ir.ot = OT_BYTE;
+          else
+	    ir.ot = ir.dflag + OT_WORD;
           regcache_raw_read_unsigned (ir.regcache,
                                       ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
+                                      &tmpulongest);
+          if (ir.aflag)
+            {
+              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
+              if (record_debug)
+                printf_unfiltered (_("Process record ignores the
memory change "
+                                     "of instruction at address 0x%s because "
+                                     "it can't get the value of the segment "
+                                     "register.\n"),
+                                   paddress (gdbarch, ir.addr));
+            }
+          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
             return -1;
-        }
-      if (opcode == 0xa4 || opcode == 0xa5)
-        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+          if (opcode == 0xa4 || opcode == 0xa5)
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+	}
       break;

       /* cmpsS */

[-- Attachment #2: prec-fix-x86-strinsn.txt --]
[-- Type: text/plain, Size: 3380 bytes --]

---
 i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4441,50 +4441,37 @@ reswitch:
       /* insS */
     case 0x6c:
     case 0x6d:
-      if ((opcode & 1) == 0)
-	ir.ot = OT_BYTE;
-      else
-	ir.ot = ir.dflag + OT_WORD;
       regcache_raw_read_unsigned (ir.regcache,
-                                  ir.regmap[X86_RECORD_REDI_REGNUM],
+                                  ir.regmap[X86_RECORD_RECX_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
-        {
-          tmpulongest &= 0xffff;
-          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
-          if (record_debug)
-            printf_unfiltered (_("Process record ignores the memory change "
-                                 "of instruction at address 0x%s because "
-                                 "it can't get the value of the segment "
-                                 "register.\n"),
-                               paddress (gdbarch, ir.addr));
-        }
-      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+      if (tmpulongest)
         {
-          ULONGEST count, eflags;
+          if ((opcode & 1) == 0)
+	    ir.ot = OT_BYTE;
+          else
+	    ir.ot = ir.dflag + OT_WORD;
           regcache_raw_read_unsigned (ir.regcache,
                                       ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
-          regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
+                                      &tmpulongest);
+          if (ir.aflag)
+            {
+              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
+              if (record_debug)
+                printf_unfiltered (_("Process record ignores the memory change "
+                                     "of instruction at address 0x%s because "
+                                     "it can't get the value of the segment "
+                                     "register.\n"),
+                                   paddress (gdbarch, ir.addr));
+            }
+          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
             return -1;
-        }
-      if (opcode == 0xa4 || opcode == 0xa5)
-        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+          if (opcode == 0xa4 || opcode == 0xa5)
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+	}
       break;
 
       /* cmpsS */

Re: Bug in i386_process_record?

[Hui Zhu]

On Tue, Aug 18, 2009 at 17:21, Hui Zhu <teawater@gmail.com> wrote:
>
> On Tue, Aug 18, 2009 at 12:15, Michael Snyder<msnyder@vmware.com> wrote:
> > Hui Zhu wrote:
> >>
> >> On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com> wrote:
> >>>
> >>> Yes, this seems to be better.  It records only 4 bytes each time
> >>> it is called.
> >>>
> >>> But there seems to be still an off-by-one error?  With the test
> >>> program that I provided, we call memset with an argument of
> >>> 1024, but we actually record 1025 bytes... this code gets hit
> >>> 257 times, with the last time recording only 1 byte.
> >>>
> >>>
> >>
> >> Hi Michael,
> >>
> >> This issue is because:
> >>
> >> 0xb7edf4e7 <memset+55>: rep stos %eax,%es:(%edi)
> >> 0xb7edf4e9 <memset+57>: mov    %edx,%ecx
> >> 0xb7edf4eb <memset+59>: rep stos %al,%es:(%edi)
> >> 0xb7edf4ed <memset+61>: mov    0x8(%esp),%eax
> >> 0xb7edf4f1 <memset+65>: pop    %edi
> >>
> >> If the memcpy size is not align with 4, it will handle by second rep stos.
> >> Then rep stos will not execute if %ecx is 0.
> >> i386_process_record doesn't check %ecx, so it get this error.
> >>
> >> I make a new patch for it.  Please help me review it.
> >
> > This seems much better.  Please give us a change log and post it for review.
> >
> > By the way, I'm sorry, I only just realized that I posted two
> > completely different bug reports with the exact same subject line.
> > ;-(
> >
>
> Don't worry about it.  Gmail handle it very well.  :)
>
> Thanks,
> Hui
>
> 2009-08-18  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (i386_process_record): Remove some error code.
>

Oops, the changelog is not right.  I make a new one.

Thanks,
Hui

2009-08-21  Hui Zhu  <teawater@gmail.com>

	* i386-tdep.c (i386_process_record): Fix the error of string
	ops instructions's handler.


> ---
>  i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
>  1 file changed, 24 insertions(+), 37 deletions(-)
>
> --- a/i386-tdep.c
> +++ b/i386-tdep.c
> @@ -4441,50 +4441,37 @@ reswitch:
>       /* insS */
>     case 0x6c:
>     case 0x6d:
> -      if ((opcode & 1) == 0)
> -       ir.ot = OT_BYTE;
> -      else
> -       ir.ot = ir.dflag + OT_WORD;
>       regcache_raw_read_unsigned (ir.regcache,
> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>                                   &tmpulongest);
> -      if (!ir.aflag)
> -        {
> -          tmpulongest &= 0xffff;
> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
> -          if (record_debug)
> -            printf_unfiltered (_("Process record ignores the memory change "
> -                                 "of instruction at address 0x%s because "
> -                                 "it can't get the value of the segment "
> -                                 "register.\n"),
> -                               paddress (gdbarch, ir.addr));
> -        }
> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> +      if (tmpulongest)
>         {
> -          ULONGEST count, eflags;
> +          if ((opcode & 1) == 0)
> +           ir.ot = OT_BYTE;
> +          else
> +           ir.ot = ir.dflag + OT_WORD;
>           regcache_raw_read_unsigned (ir.regcache,
>                                       ir.regmap[X86_RECORD_REDI_REGNUM],
> -                                      &count);
> -          if (!ir.aflag)
> -            count &= 0xffff;
> -          regcache_raw_read_unsigned (ir.regcache,
> -                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
> -                                      &eflags);
> -          if ((eflags >> 10) & 0x1)
> -            tmpulongest -= (count - 1) * (1 << ir.ot);
> -          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
> -            return -1;
> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
> -        }
> -      else
> -        {
> +                                      &tmpulongest);
> +          if (ir.aflag)
> +            {
> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
> +              if (record_debug)
> +                printf_unfiltered (_("Process record ignores the
> memory change "
> +                                     "of instruction at address 0x%s because "
> +                                     "it can't get the value of the segment "
> +                                     "register.\n"),
> +                                   paddress (gdbarch, ir.addr));
> +            }
> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>             return -1;
> -        }
> -      if (opcode == 0xa4 || opcode == 0xa5)
> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> +          if (opcode == 0xa4 || opcode == 0xa5)
> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> +       }
>       break;
>
>       /* cmpsS */

Re: Bug in i386_process_record?

[Michael Snyder]

Hi, please *don't* check this in -- I found a problem with it.

Try running it with "set debug record 1" during the recording pass.

I see a whole lot of these:

Process record ignores the memory change of instruction at address 
0x0x587be9 because it can't get the value of the segment register.

Hui Zhu wrote:
> On Tue, Aug 18, 2009 at 17:21, Hui Zhu <teawater@gmail.com> wrote:
>> On Tue, Aug 18, 2009 at 12:15, Michael Snyder<msnyder@vmware.com> wrote:
>>> Hui Zhu wrote:
>>>> On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com> wrote:
>>>>> Yes, this seems to be better.  It records only 4 bytes each time
>>>>> it is called.
>>>>>
>>>>> But there seems to be still an off-by-one error?  With the test
>>>>> program that I provided, we call memset with an argument of
>>>>> 1024, but we actually record 1025 bytes... this code gets hit
>>>>> 257 times, with the last time recording only 1 byte.
>>>>>
>>>>>
>>>> Hi Michael,
>>>>
>>>> This issue is because:
>>>>
>>>> 0xb7edf4e7 <memset+55>: rep stos %eax,%es:(%edi)
>>>> 0xb7edf4e9 <memset+57>: mov    %edx,%ecx
>>>> 0xb7edf4eb <memset+59>: rep stos %al,%es:(%edi)
>>>> 0xb7edf4ed <memset+61>: mov    0x8(%esp),%eax
>>>> 0xb7edf4f1 <memset+65>: pop    %edi
>>>>
>>>> If the memcpy size is not align with 4, it will handle by second rep stos.
>>>> Then rep stos will not execute if %ecx is 0.
>>>> i386_process_record doesn't check %ecx, so it get this error.
>>>>
>>>> I make a new patch for it.  Please help me review it.
>>> This seems much better.  Please give us a change log and post it for review.
>>>
>>> By the way, I'm sorry, I only just realized that I posted two
>>> completely different bug reports with the exact same subject line.
>>> ;-(
>>>
>> Don't worry about it.  Gmail handle it very well.  :)
>>
>> Thanks,
>> Hui
>>
>> 2009-08-18  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (i386_process_record): Remove some error code.
>>
> 
> Oops, the changelog is not right.  I make a new one.
> 
> Thanks,
> Hui
> 
> 2009-08-21  Hui Zhu  <teawater@gmail.com>
> 
> 	* i386-tdep.c (i386_process_record): Fix the error of string
> 	ops instructions's handler.
> 
> 
>> ---
>>  i386-tdep.c |   61 +++++++++++++++++++++++-------------------------------------
>>  1 file changed, 24 insertions(+), 37 deletions(-)
>>
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -4441,50 +4441,37 @@ reswitch:
>>       /* insS */
>>     case 0x6c:
>>     case 0x6d:
>> -      if ((opcode & 1) == 0)
>> -       ir.ot = OT_BYTE;
>> -      else
>> -       ir.ot = ir.dflag + OT_WORD;
>>       regcache_raw_read_unsigned (ir.regcache,
>> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
>> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>>                                   &tmpulongest);
>> -      if (!ir.aflag)
>> -        {
>> -          tmpulongest &= 0xffff;
>> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
>> -          if (record_debug)
>> -            printf_unfiltered (_("Process record ignores the memory change "
>> -                                 "of instruction at address 0x%s because "
>> -                                 "it can't get the value of the segment "
>> -                                 "register.\n"),
>> -                               paddress (gdbarch, ir.addr));
>> -        }
>> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> +      if (tmpulongest)
>>         {
>> -          ULONGEST count, eflags;
>> +          if ((opcode & 1) == 0)
>> +           ir.ot = OT_BYTE;
>> +          else
>> +           ir.ot = ir.dflag + OT_WORD;
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_REDI_REGNUM],
>> -                                      &count);
>> -          if (!ir.aflag)
>> -            count &= 0xffff;
>> -          regcache_raw_read_unsigned (ir.regcache,
>> -                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
>> -                                      &eflags);
>> -          if ((eflags >> 10) & 0x1)
>> -            tmpulongest -= (count - 1) * (1 << ir.ot);
>> -          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
>> -            return -1;
>> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>> -        }
>> -      else
>> -        {
>> +                                      &tmpulongest);
>> +          if (ir.aflag)
>> +            {
>> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
>> +              if (record_debug)
>> +                printf_unfiltered (_("Process record ignores the
>> memory change "
>> +                                     "of instruction at address 0x%s because "
>> +                                     "it can't get the value of the segment "
>> +                                     "register.\n"),
>> +                                   paddress (gdbarch, ir.addr));
>> +            }
>> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>>           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>>             return -1;
>> -        }
>> -      if (opcode == 0xa4 || opcode == 0xa5)
>> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>> +          if (opcode == 0xa4 || opcode == 0xa5)
>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>> +       }
>>       break;
>>
>>       /* cmpsS */
> 

Re: Bug in i386_process_record?

[Hui Zhu]

On Sun, Aug 23, 2009 at 11:07, Michael Snyder<msnyder@vmware.com> wrote:
> Hi, please *don't* check this in -- I found a problem with it.
>
> Try running it with "set debug record 1" during the recording pass.
>
> I see a whole lot of these:
>
> Process record ignores the memory change of instruction at address
> 0x0x587be9 because it can't get the value of the segment register.


Hi Michael,

Most of the string ops instruction will use segment register.
But I check the some linux program that have string ops insn.  I found
that in linux (maybe glibc), the value of the segment register is 0,
so it will not affect anything.

And in linux user level, looks we don't have any good way to get the
value of the segment register.

So I think this patch is OK.

Thanks,
Hui

>
> Hui Zhu wrote:
>>
>> On Tue, Aug 18, 2009 at 17:21, Hui Zhu <teawater@gmail.com> wrote:
>>>
>>> On Tue, Aug 18, 2009 at 12:15, Michael Snyder<msnyder@vmware.com> wrote:
>>>>
>>>> Hui Zhu wrote:
>>>>>
>>>>> On Tue, Aug 11, 2009 at 05:57, Michael Snyder <msnyder@vmware.com>
>>>>> wrote:
>>>>>>
>>>>>> Yes, this seems to be better.  It records only 4 bytes each time
>>>>>> it is called.
>>>>>>
>>>>>> But there seems to be still an off-by-one error?  With the test
>>>>>> program that I provided, we call memset with an argument of
>>>>>> 1024, but we actually record 1025 bytes... this code gets hit
>>>>>> 257 times, with the last time recording only 1 byte.
>>>>>>
>>>>>>
>>>>> Hi Michael,
>>>>>
>>>>> This issue is because:
>>>>>
>>>>> 0xb7edf4e7 <memset+55>: rep stos %eax,%es:(%edi)
>>>>> 0xb7edf4e9 <memset+57>: mov    %edx,%ecx
>>>>> 0xb7edf4eb <memset+59>: rep stos %al,%es:(%edi)
>>>>> 0xb7edf4ed <memset+61>: mov    0x8(%esp),%eax
>>>>> 0xb7edf4f1 <memset+65>: pop    %edi
>>>>>
>>>>> If the memcpy size is not align with 4, it will handle by second rep
>>>>> stos.
>>>>> Then rep stos will not execute if %ecx is 0.
>>>>> i386_process_record doesn't check %ecx, so it get this error.
>>>>>
>>>>> I make a new patch for it.  Please help me review it.
>>>>
>>>> This seems much better.  Please give us a change log and post it for
>>>> review.
>>>>
>>>> By the way, I'm sorry, I only just realized that I posted two
>>>> completely different bug reports with the exact same subject line.
>>>> ;-(
>>>>
>>> Don't worry about it.  Gmail handle it very well.  :)
>>>
>>> Thanks,
>>> Hui
>>>
>>> 2009-08-18  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * record.c (i386_process_record): Remove some error code.
>>>
>>
>> Oops, the changelog is not right.  I make a new one.
>>
>> Thanks,
>> Hui
>>
>> 2009-08-21  Hui Zhu  <teawater@gmail.com>
>>
>>        * i386-tdep.c (i386_process_record): Fix the error of string
>>        ops instructions's handler.
>>
>>
>>> ---
>>>  i386-tdep.c |   61
>>> +++++++++++++++++++++++-------------------------------------
>>>  1 file changed, 24 insertions(+), 37 deletions(-)
>>>
>>> --- a/i386-tdep.c
>>> +++ b/i386-tdep.c
>>> @@ -4441,50 +4441,37 @@ reswitch:
>>>      /* insS */
>>>    case 0x6c:
>>>    case 0x6d:
>>> -      if ((opcode & 1) == 0)
>>> -       ir.ot = OT_BYTE;
>>> -      else
>>> -       ir.ot = ir.dflag + OT_WORD;
>>>      regcache_raw_read_unsigned (ir.regcache,
>>> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
>>> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>>>                                  &tmpulongest);
>>> -      if (!ir.aflag)
>>> -        {
>>> -          tmpulongest &= 0xffff;
>>> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4;
>>> */
>>> -          if (record_debug)
>>> -            printf_unfiltered (_("Process record ignores the memory
>>> change "
>>> -                                 "of instruction at address 0x%s because
>>> "
>>> -                                 "it can't get the value of the segment
>>> "
>>> -                                 "register.\n"),
>>> -                               paddress (gdbarch, ir.addr));
>>> -        }
>>> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>>> +      if (tmpulongest)
>>>        {
>>> -          ULONGEST count, eflags;
>>> +          if ((opcode & 1) == 0)
>>> +           ir.ot = OT_BYTE;
>>> +          else
>>> +           ir.ot = ir.dflag + OT_WORD;
>>>          regcache_raw_read_unsigned (ir.regcache,
>>>                                      ir.regmap[X86_RECORD_REDI_REGNUM],
>>> -                                      &count);
>>> -          if (!ir.aflag)
>>> -            count &= 0xffff;
>>> -          regcache_raw_read_unsigned (ir.regcache,
>>> -
>>>  ir.regmap[X86_RECORD_EFLAGS_REGNUM],
>>> -                                      &eflags);
>>> -          if ((eflags >> 10) & 0x1)
>>> -            tmpulongest -= (count - 1) * (1 << ir.ot);
>>> -          if (record_arch_list_add_mem (tmpulongest, count * (1 <<
>>> ir.ot)))
>>> -            return -1;
>>> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>>> -        }
>>> -      else
>>> -        {
>>> +                                      &tmpulongest);
>>> +          if (ir.aflag)
>>> +            {
>>> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) <<
>>> 4; */
>>> +              if (record_debug)
>>> +                printf_unfiltered (_("Process record ignores the
>>> memory change "
>>> +                                     "of instruction at address 0x%s
>>> because "
>>> +                                     "it can't get the value of the
>>> segment "
>>> +                                     "register.\n"),
>>> +                                   paddress (gdbarch, ir.addr));
>>> +            }
>>> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>>>          if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>>>            return -1;
>>> -        }
>>> -      if (opcode == 0xa4 || opcode == 0xa5)
>>> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>>> +          if (opcode == 0xa4 || opcode == 0xa5)
>>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>>> +       }
>>>      break;
>>>
>>>      /* cmpsS */
>>
>
>

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Sun, Aug 23, 2009 at 11:07, Michael Snyder<msnyder@vmware.com> wrote:
>> Hi, please *don't* check this in -- I found a problem with it.
>>
>> Try running it with "set debug record 1" during the recording pass.
>>
>> I see a whole lot of these:
>>
>> Process record ignores the memory change of instruction at address
>> 0x0x587be9 because it can't get the value of the segment register.
> 
> 
> Hi Michael,
> 
> Most of the string ops instruction will use segment register.
> But I check the some linux program that have string ops insn.  I found
> that in linux (maybe glibc), the value of the segment register is 0,
> so it will not affect anything.

Hmm, ok, but this is i386-tdep.c, not i386-linux-tdep.c...

> And in linux user level, looks we don't have any good way to get the
> value of the segment register.
> 
> So I think this patch is OK.

I see -- so, we don't really "ignore" the memory change at all.

Isn't the message misleading, then?

What about something like:

@@ -4458,11 +4458,12 @@ reswitch:
            if (ir.aflag)
              {
                /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) 
<< 4; */
-              if (record_debug)
-                printf_unfiltered (_("Process record ignores the memory 
change "
-                                     "of instruction at address 0x%s 
because "
-                                     "it can't get the value of the 
segment "
-                                     "register.\n"),
+              if (record_debug &&
+                 read_register (I386_ES_REGNUM) != 0)
+                printf_unfiltered (_("Process record ignores value of ES "
+                                     "register for instruction at 
address %s "
+                                     "because "it can't get the value of "
+                                     "the segment register.\n"),
                                     paddress (gdbarch, ir.addr));
              }
            if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))

Re: Bug in i386_process_record?

[Hui Zhu]

On Sun, Aug 23, 2009 at 12:07, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Sun, Aug 23, 2009 at 11:07, Michael Snyder<msnyder@vmware.com> wrote:
>>>
>>> Hi, please *don't* check this in -- I found a problem with it.
>>>
>>> Try running it with "set debug record 1" during the recording pass.
>>>
>>> I see a whole lot of these:
>>>
>>> Process record ignores the memory change of instruction at address
>>> 0x0x587be9 because it can't get the value of the segment register.
>>
>>
>> Hi Michael,
>>
>> Most of the string ops instruction will use segment register.
>> But I check the some linux program that have string ops insn.  I found
>> that in linux (maybe glibc), the value of the segment register is 0,
>> so it will not affect anything.
>
> Hmm, ok, but this is i386-tdep.c, not i386-linux-tdep.c...
>
>> And in linux user level, looks we don't have any good way to get the
>> value of the segment register.
>>
>> So I think this patch is OK.
>
> I see -- so, we don't really "ignore" the memory change at all.
>
> Isn't the message misleading, then?
>
> What about something like:
>
> @@ -4458,11 +4458,12 @@ reswitch:
>           if (ir.aflag)
>             {
>               /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4;
> */
> -              if (record_debug)
> -                printf_unfiltered (_("Process record ignores the memory
> change "
> -                                     "of instruction at address 0x%s
> because "
> -                                     "it can't get the value of the segment
> "
> -                                     "register.\n"),
> +              if (record_debug &&
> +                 read_register (I386_ES_REGNUM) != 0)
> +                printf_unfiltered (_("Process record ignores value of ES "
> +                                     "register for instruction at address
> %s "
> +                                     "because "it can't get the value of "
> +                                     "the segment register.\n"),
>                                    paddress (gdbarch, ir.addr));
>             }
>           if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>

read_register (I386_ES_REGNUM)
This value is not the value of ES.  This is number of TLB.  So ....

Hui

Re: Bug in i386_process_record?

[Hui Zhu]

On Sun, Aug 23, 2009 at 12:29, Hui Zhu<teawater@gmail.com> wrote:
> On Sun, Aug 23, 2009 at 12:07, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>>
>>> On Sun, Aug 23, 2009 at 11:07, Michael Snyder<msnyder@vmware.com> wrote:
>>>>
>>>> Hi, please *don't* check this in -- I found a problem with it.
>>>>
>>>> Try running it with "set debug record 1" during the recording pass.
>>>>
>>>> I see a whole lot of these:
>>>>
>>>> Process record ignores the memory change of instruction at address
>>>> 0x0x587be9 because it can't get the value of the segment register.
>>>
>>>
>>> Hi Michael,
>>>
>>> Most of the string ops instruction will use segment register.
>>> But I check the some linux program that have string ops insn.  I found
>>> that in linux (maybe glibc), the value of the segment register is 0,
>>> so it will not affect anything.
>>
>> Hmm, ok, but this is i386-tdep.c, not i386-linux-tdep.c...
>>
>>> And in linux user level, looks we don't have any good way to get the
>>> value of the segment register.
>>>
>>> So I think this patch is OK.
>>
>> I see -- so, we don't really "ignore" the memory change at all.
>>
>> Isn't the message misleading, then?
>>
>> What about something like:
>>
>> @@ -4458,11 +4458,12 @@ reswitch:
>>           if (ir.aflag)
>>             {
>>               /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4;
>> */
>> -              if (record_debug)
>> -                printf_unfiltered (_("Process record ignores the memory
>> change "
>> -                                     "of instruction at address 0x%s
>> because "
>> -                                     "it can't get the value of the segment
>> "
>> -                                     "register.\n"),
>> +              if (record_debug &&
>> +                 read_register (I386_ES_REGNUM) != 0)
>> +                printf_unfiltered (_("Process record ignores value of ES "
>> +                                     "register for instruction at address
>> %s "
>> +                                     "because "it can't get the value of "
>> +                                     "the segment register.\n"),
>>                                    paddress (gdbarch, ir.addr));
>>             }
>>           if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>>
>
> read_register (I386_ES_REGNUM)
> This value is not the value of ES.  This is number of TLB.  So ....
>
> Hui
>

ES is one of the segment register.

Hui

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Sun, Aug 23, 2009 at 12:07, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> On Sun, Aug 23, 2009 at 11:07, Michael Snyder<msnyder@vmware.com> wrote:
>>>> Hi, please *don't* check this in -- I found a problem with it.
>>>>
>>>> Try running it with "set debug record 1" during the recording pass.
>>>>
>>>> I see a whole lot of these:
>>>>
>>>> Process record ignores the memory change of instruction at address
>>>> 0x0x587be9 because it can't get the value of the segment register.
>>>
>>> Hi Michael,
>>>
>>> Most of the string ops instruction will use segment register.
>>> But I check the some linux program that have string ops insn.  I found
>>> that in linux (maybe glibc), the value of the segment register is 0,
>>> so it will not affect anything.
>> Hmm, ok, but this is i386-tdep.c, not i386-linux-tdep.c...
>>
>>> And in linux user level, looks we don't have any good way to get the
>>> value of the segment register.
>>>
>>> So I think this patch is OK.
>> I see -- so, we don't really "ignore" the memory change at all.
>>
>> Isn't the message misleading, then?
>>
>> What about something like:
>>
>> @@ -4458,11 +4458,12 @@ reswitch:
>>           if (ir.aflag)
>>             {
>>               /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4;
>> */
>> -              if (record_debug)
>> -                printf_unfiltered (_("Process record ignores the memory
>> change "
>> -                                     "of instruction at address 0x%s
>> because "
>> -                                     "it can't get the value of the segment
>> "
>> -                                     "register.\n"),
>> +              if (record_debug &&
>> +                 read_register (I386_ES_REGNUM) != 0)
>> +                printf_unfiltered (_("Process record ignores value of ES "
>> +                                     "register for instruction at address
>> %s "
>> +                                     "because "it can't get the value of "
>> +                                     "the segment register.\n"),
>>                                    paddress (gdbarch, ir.addr));
>>             }
>>           if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>>
> 
> read_register (I386_ES_REGNUM)
> This value is not the value of ES.  This is number of TLB.  So ....

So... what?  I don't know about these things.
I'm just asking, could the message be more informative or accurate than 
it is?

The message seems to say "ignores the memory change", which to me
implies "does not record the memory change".  Yet the code seems
to imply that it *does* record the memory change, it just ignores
the ES offset.

Re: Bug in i386_process_record?

[Eli Zaretskii]

> From: Hui Zhu <teawater@gmail.com>
> Date: Sun, 23 Aug 2009 12:29:33 +0800
> Cc: gdb-patches ml <gdb-patches@sourceware.org>
> 
> read_register (I386_ES_REGNUM)
> This value is not the value of ES.  This is number of TLB.

On what OS?

Re: Bug in i386_process_record?

[Hui Zhu]

If I am right, this is from the old memory manager -- segment manager.
 X86 is a old arch and support it.

Now, most of OS include Linux, they don't use this MM, they use page
manager that X86 support it too (X86 is crazy).  So they set the value
of segment reg to 0.

For the gdb, the value of segment reg is not the really value.
cs             0x73	115
ss             0x7b	123
ds             0x7b	123
es             0x7b	123
fs             0x0	0
gs             0x33	51
I have tried some insn that use segment reg such as string ops insn.
I found that the value of this segment reg cannot affect anything.

And prec just support Linux now.  I have move
"set_gdbarch_process_record (gdbarch, i386_process_record);" to
i386-linux-tdep.c.

This patch doesn't add any more thing, just fix the bug.  And this bug
seems affect a lot of program (for example, Oza's fp example).  I
suggest let it in first.  After that, we can find a good way to handle
the segment reg better.

What do you think about it?

Thanks,
Hui

On Mon, Aug 24, 2009 at 02:24, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Hui Zhu <teawater@gmail.com>
> > Date: Sun, 23 Aug 2009 12:29:33 +0800
> > Cc: gdb-patches ml <gdb-patches@sourceware.org>
> >
> > read_register (I386_ES_REGNUM)
> > This value is not the value of ES.  This is number of TLB.
>
> On what OS?

Re: Bug in i386_process_record?

[Daniel Jacobowitz]

On Mon, Aug 24, 2009 at 07:42:41AM +0800, Hui Zhu wrote:
> On Mon, Aug 24, 2009 at 02:24, Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Hui Zhu <teawater@gmail.com>
> > > Date: Sun, 23 Aug 2009 12:29:33 +0800
> > > Cc: gdb-patches ml <gdb-patches@sourceware.org>
> > >
> > > read_register (I386_ES_REGNUM)
> > > This value is not the value of ES.  This is number of TLB.
> >
> > On what OS?

If I remember correctly, the %es that GDB sees is the selector rather
than the base address.  I think any halfway recent Linux kernel
exposes both values, but GDB doesn't display the bases - this probably
bears fixing!

-- 
Daniel Jacobowitz
CodeSourcery

Re: Bug in i386_process_record?

[Hui Zhu]

On Mon, Aug 24, 2009 at 07:53, Daniel Jacobowitz<drow@false.org> wrote:
> On Mon, Aug 24, 2009 at 07:42:41AM +0800, Hui Zhu wrote:
>> On Mon, Aug 24, 2009 at 02:24, Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> > > From: Hui Zhu <teawater@gmail.com>
>> > > Date: Sun, 23 Aug 2009 12:29:33 +0800
>> > > Cc: gdb-patches ml <gdb-patches@sourceware.org>
>> > >
>> > > read_register (I386_ES_REGNUM)
>> > > This value is not the value of ES.  This is number of TLB.
>> >
>> > On what OS?
>
> If I remember correctly, the %es that GDB sees is the selector rather
> than the base address.  I think any halfway recent Linux kernel
> exposes both values, but GDB doesn't display the bases - this probably
> bears fixing!
>
If this value can be added, it will be very great for me.  :)

Thanks,
Hui

Re: Bug in i386_process_record?

[Eli Zaretskii]

> Date: Sun, 23 Aug 2009 19:53:20 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, msnyder@vmware.com,
> 	gdb-patches@sourceware.org
> 
> If I remember correctly, the %es that GDB sees is the selector rather
> than the base address.

Yes, at least in the DJGPP Port, that's so.

> I think any halfway recent Linux kernel exposes both values, but GDB
> doesn't display the bases - this probably bears fixing!

There's a system call in DJGPP to get the base address; see "info
dos".

Re: Bug in i386_process_record?

[Hui Zhu]

On Mon, Aug 24, 2009 at 07:42, Hui Zhu<teawater@gmail.com> wrote:
> If I am right, this is from the old memory manager -- segment manager.
>  X86 is a old arch and support it.
>
> Now, most of OS include Linux, they don't use this MM, they use page
> manager that X86 support it too (X86 is crazy).  So they set the value
> of segment reg to 0.
>
> For the gdb, the value of segment reg is not the really value.
> cs             0x73     115
> ss             0x7b     123
> ds             0x7b     123
> es             0x7b     123
> fs             0x0      0
> gs             0x33     51
> I have tried some insn that use segment reg such as string ops insn.
> I found that the value of this segment reg cannot affect anything.
>
> And prec just support Linux now.  I have move
> "set_gdbarch_process_record (gdbarch, i386_process_record);" to
> i386-linux-tdep.c.
>
> This patch doesn't add any more thing, just fix the bug.  And this bug
> seems affect a lot of program (for example, Oza's fp example).  I
> suggest let it in first.  After that, we can find a good way to handle
> the segment reg better.
>
> What do you think about it?
>
> Thanks,
> Hui
>
> On Mon, Aug 24, 2009 at 02:24, Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> > From: Hui Zhu <teawater@gmail.com>
>> > Date: Sun, 23 Aug 2009 12:29:33 +0800
>> > Cc: gdb-patches ml <gdb-patches@sourceware.org>
>> >
>> > read_register (I386_ES_REGNUM)
>> > This value is not the value of ES.  This is number of TLB.
>>
>> On what OS?
>

Please let me show a example for it.

cat memrange-reverse.c
/* This testcase is part of GDB, the GNU debugger.

   Copyright 2009 Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

#include <string.h>

#define SIZE_BLOB1 1024
#define SIZE_BLOB2  256

char blob1[SIZE_BLOB1], blob2[SIZE_BLOB2];

int main ()
{
  int i;

  memset (blob1, 'a', sizeof (blob1));
  blob1[sizeof (blob1) - 1] = '\0';

  memset (blob2, 'b', sizeof (blob2));
  blob2[sizeof (blob2) - 1] = '\0';

  for (i = 2; i < 8; i++)
    {
      memcpy (blob1 + (sizeof (blob1) / i), blob2, sizeof (blob2));
    }

  return 0;	/* end of main */
}



gcc -g memrange-reverse.c


gdb ./a.out
GNU gdb (GDB) 6.8.50.20090807-cvs
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
(gdb) start
Temporary breakpoint 1 at 0x80483b5: file memrange-reverse.c, line 29.
Starting program: /home/teawater/Desktop/a.out

Temporary breakpoint 1, main () at memrange-reverse.c:29
29	  memset (blob1, 'a', sizeof (blob1));
(gdb) x blob1
0x8049660 <blob1>:	0x00000000
#This address is what we really want to set.


(gdb) b *0xb7eec4e7
Breakpoint 2 at 0xb7eec4e7
(gdb) set disassemble-next-line on
(gdb) c
Continuing.

Breakpoint 2, 0xb7eec4e7 in memset () from /lib/tls/i686/cmov/libc.so.6
0xb7eec4e7 <memset+55>:	 f3 ab	rep stos %eax,%es:(%edi)
#This is the code that will set the blob1

(gdb) disassemble
Dump of assembler code for function memset:
0xb7eec4b0 <memset+0>:	cld
0xb7eec4b1 <memset+1>:	push   %edi
0xb7eec4b2 <memset+2>:	mov    0x8(%esp),%edx
0xb7eec4b6 <memset+6>:	mov    0x10(%esp),%ecx
0xb7eec4ba <memset+10>:	movzbl 0xc(%esp),%eax
0xb7eec4bf <memset+15>:	jecxz  0xb7eec4ed <memset+61>
0xb7eec4c1 <memset+17>:	mov    %edx,%edi
0xb7eec4c3 <memset+19>:	and    $0x3,%edx
0xb7eec4c6 <memset+22>:	je     0xb7eec4d9 <memset+41>
0xb7eec4c8 <memset+24>:	jp     0xb7eec4ce <memset+30>
0xb7eec4ca <memset+26>:	stos   %al,%es:(%edi)
0xb7eec4cb <memset+27>:	dec    %ecx
0xb7eec4cc <memset+28>:	je     0xb7eec4ed <memset+61>
0xb7eec4ce <memset+30>:	stos   %al,%es:(%edi)
0xb7eec4cf <memset+31>:	dec    %ecx
0xb7eec4d0 <memset+32>:	je     0xb7eec4ed <memset+61>
0xb7eec4d2 <memset+34>:	xor    $0x1,%edx
0xb7eec4d5 <memset+37>:	jne    0xb7eec4d9 <memset+41>
0xb7eec4d7 <memset+39>:	stos   %al,%es:(%edi)
0xb7eec4d8 <memset+40>:	dec    %ecx
0xb7eec4d9 <memset+41>:	mov    %ecx,%edx
0xb7eec4db <memset+43>:	shr    $0x2,%ecx
0xb7eec4de <memset+46>:	and    $0x3,%edx
0xb7eec4e1 <memset+49>:	imul   $0x1010101,%eax,%eax
0xb7eec4e7 <memset+55>:	rep stos %eax,%es:(%edi)
0xb7eec4e9 <memset+57>:	mov    %edx,%ecx
0xb7eec4eb <memset+59>:	rep stos %al,%es:(%edi)
0xb7eec4ed <memset+61>:	mov    0x8(%esp),%eax
0xb7eec4f1 <memset+65>:	pop    %edi
0xb7eec4f2 <memset+66>:	ret
End of assembler dump.
(gdb) info reg $edi
edi            0x8049660	134518368
(gdb) info reg $es
es             0x7b	123

#rep stos %eax,%es:(%edi)
$edi + 0 = 0x8049660 blob1
$edi + $es != 0x8049660 blob1


Thanks,
Hui

Re: Bug in i386_process_record?

[Eli Zaretskii]

> From: Hui Zhu <teawater@gmail.com>
> Date: Mon, 24 Aug 2009 08:00:29 +0800
> Cc: gdb-patches@sourceware.org
> 
> #rep stos %eax,%es:(%edi)
> $edi + 0 = 0x8049660 blob1
> $edi + $es != 0x8049660 blob1

Well, of course! %es:(%edi) does _not_ mean $es+$edi, it means that
$edi is used to address the section whose segment descriptor's index
(a.k.a. selector) is in $es.  That is, in your case, 0x7b is the
selector that identifies the segment descriptor of the section where
blob1[] is stored (.bss, if my rusty memory doesn't deceive me).

Am I missing something?  If not, what was this example supposed to
prove, exactly?

Re: Bug in i386_process_record?

[Hui Zhu]

On Tue, Aug 25, 2009 at 03:18, Eli Zaretskii<eliz@gnu.org> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Mon, 24 Aug 2009 08:00:29 +0800
>> Cc: gdb-patches@sourceware.org
>>
>> #rep stos %eax,%es:(%edi)
>> $edi + 0 = 0x8049660 blob1
>> $edi + $es != 0x8049660 blob1
>
> Well, of course! %es:(%edi) does _not_ mean $es+$edi, it means that
> $edi is used to address the section whose segment descriptor's index
> (a.k.a. selector) is in $es.  That is, in your case, 0x7b is the
> selector that identifies the segment descriptor of the section where
> blob1[] is stored (.bss, if my rusty memory doesn't deceive me).
>
> Am I missing something?  If not, what was this example supposed to
> prove, exactly?
>

I check the code about Kernel and read some X86 spec about it.
It seems that the segment (It is not the section)  registers in x86
protect mode is just help MMU to get the physical address.  It's
transparent for the user level program.

What do you think about remove this warning from this patch?

Thanks,
Hui

Re: Bug in i386_process_record?

[Eli Zaretskii]

> From: Hui Zhu <teawater@gmail.com>
> Date: Tue, 25 Aug 2009 13:02:44 +0800
> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
> 
> It seems that the segment (It is not the section)  registers in x86
> protect mode is just help MMU to get the physical address.  It's
> transparent for the user level program.

It's transparent if $es and $ds have the same value (which they
usually do, AFAIK).

> What do you think about remove this warning from this patch?

I would indeed do that, if we find that $es and $ds have the same
values.  Assuming that someone who knows Linux better than I do
confirms that these two registers hold the same selector when a normal
application is running in user mode.

Re: Bug in i386_process_record?

[Hui Zhu]

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

On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>
>> It seems that the segment (It is not the section)  registers in x86
>> protect mode is just help MMU to get the physical address.  It's
>> transparent for the user level program.
>
> It's transparent if $es and $ds have the same value (which they
> usually do, AFAIK).
>
>> What do you think about remove this warning from this patch?
>
> I would indeed do that, if we find that $es and $ds have the same
> values.  Assuming that someone who knows Linux better than I do
> confirms that these two registers hold the same selector when a normal
> application is running in user mode.
>

Thanks for remind me.  We cannot get the value of each segment
register, but we can get each segment register point to.  So if the
value of segment registers, it's means that the value of them is same.

I add some code about it:
          regcache_raw_read_unsigned (ir.regcache,
                                      ir.regmap[X86_RECORD_ES_REGNUM],
                                      &es);
          regcache_raw_read_unsigned (ir.regcache,
                                      ir.regmap[X86_RECORD_DS_REGNUM],
                                      &ds);
          if (ir.aflag && (es != ds))
            {

After that, we will not get the warning because the es is same with ds
in user level.

What do you think about it?

Thanks,
Hui

2009-08-26  Hui Zhu  <teawater@gmail.com>

	* i386-tdep.c (i386_process_record): Fix the error of string
	ops instructions's handler.
---
 i386-tdep.c |   69 ++++++++++++++++++++++++++++--------------------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4441,50 +4441,47 @@ reswitch:
       /* insS */
     case 0x6c:
     case 0x6d:
-      if ((opcode & 1) == 0)
-	ir.ot = OT_BYTE;
-      else
-	ir.ot = ir.dflag + OT_WORD;
       regcache_raw_read_unsigned (ir.regcache,
-                                  ir.regmap[X86_RECORD_REDI_REGNUM],
+                                  ir.regmap[X86_RECORD_RECX_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
-        {
-          tmpulongest &= 0xffff;
-          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
-          if (record_debug)
-            printf_unfiltered (_("Process record ignores the memory change "
-                                 "of instruction at address 0x%s because "
-                                 "it can't get the value of the segment "
-                                 "register.\n"),
-                               paddress (gdbarch, ir.addr));
-        }
-      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+      if (tmpulongest)
         {
-          ULONGEST count, eflags;
+          ULONGEST es, ds;
+
+          if ((opcode & 1) == 0)
+	    ir.ot = OT_BYTE;
+          else
+	    ir.ot = ir.dflag + OT_WORD;
           regcache_raw_read_unsigned (ir.regcache,
                                       ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
+                                      &tmpulongest);
+
           regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
+                                      ir.regmap[X86_RECORD_ES_REGNUM],
+                                      &es);
+          regcache_raw_read_unsigned (ir.regcache,
+                                      ir.regmap[X86_RECORD_DS_REGNUM],
+                                      &ds);
+          if (ir.aflag && (es != ds))
+            {
+              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
+              if (record_debug)
+                printf_unfiltered (_("Process record ignores the memory "
+				     "change of instruction at address 0x%s "
+				     "because it can't get the value of the "
+				     "ES segment register.\n"),
+                                   paddress (gdbarch, ir.addr));
+            }
+
+          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
             return -1;
-        }
-      if (opcode == 0xa4 || opcode == 0xa5)
-        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+          if (opcode == 0xa4 || opcode == 0xa5)
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+	}
       break;

       /* cmpsS */

[-- Attachment #2: prec-fix-x86-strinsn.txt --]
[-- Type: text/plain, Size: 3633 bytes --]

---
 i386-tdep.c |   69 ++++++++++++++++++++++++++++--------------------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

--- a/i386-tdep.c
+++ b/i386-tdep.c
@@ -4441,50 +4441,47 @@ reswitch:
       /* insS */
     case 0x6c:
     case 0x6d:
-      if ((opcode & 1) == 0)
-	ir.ot = OT_BYTE;
-      else
-	ir.ot = ir.dflag + OT_WORD;
       regcache_raw_read_unsigned (ir.regcache,
-                                  ir.regmap[X86_RECORD_REDI_REGNUM],
+                                  ir.regmap[X86_RECORD_RECX_REGNUM],
                                   &tmpulongest);
-      if (!ir.aflag)
-        {
-          tmpulongest &= 0xffff;
-          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
-          if (record_debug)
-            printf_unfiltered (_("Process record ignores the memory change "
-                                 "of instruction at address 0x%s because "
-                                 "it can't get the value of the segment "
-                                 "register.\n"),
-                               paddress (gdbarch, ir.addr));
-        }
-      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+      if (tmpulongest)
         {
-          ULONGEST count, eflags;
+          ULONGEST es, ds;
+
+          if ((opcode & 1) == 0)
+	    ir.ot = OT_BYTE;
+          else
+	    ir.ot = ir.dflag + OT_WORD;
           regcache_raw_read_unsigned (ir.regcache,
                                       ir.regmap[X86_RECORD_REDI_REGNUM],
-                                      &count);
-          if (!ir.aflag)
-            count &= 0xffff;
+                                      &tmpulongest);
+
           regcache_raw_read_unsigned (ir.regcache,
-                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
-                                      &eflags);
-          if ((eflags >> 10) & 0x1)
-            tmpulongest -= (count - 1) * (1 << ir.ot);
-          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
-            return -1;
-          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
-        }
-      else
-        {
+                                      ir.regmap[X86_RECORD_ES_REGNUM],
+                                      &es);
+          regcache_raw_read_unsigned (ir.regcache,
+                                      ir.regmap[X86_RECORD_DS_REGNUM],
+                                      &ds);
+          if (ir.aflag && (es != ds))
+            {
+              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
+              if (record_debug)
+                printf_unfiltered (_("Process record ignores the memory "
+				     "change of instruction at address 0x%s "
+				     "because it can't get the value of the "
+				     "ES segment register.\n"),
+                                   paddress (gdbarch, ir.addr));
+            }
+
+          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
             return -1;
-        }
-      if (opcode == 0xa4 || opcode == 0xa5)
-        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
-      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+          if (opcode == 0xa4 || opcode == 0xa5)
+            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
+          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
+	}
       break;
 
       /* cmpsS */

Re: Bug in i386_process_record?

[Eli Zaretskii]

> From: Hui Zhu <teawater@gmail.com>
> Date: Wed, 26 Aug 2009 10:58:39 +0800
> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
> 
> I add some code about it:
>           regcache_raw_read_unsigned (ir.regcache,
>                                       ir.regmap[X86_RECORD_ES_REGNUM],
>                                       &es);
>           regcache_raw_read_unsigned (ir.regcache,
>                                       ir.regmap[X86_RECORD_DS_REGNUM],
>                                       &ds);
>           if (ir.aflag && (es != ds))
>             {
> 
> After that, we will not get the warning because the es is same with ds
> in user level.
> 
> What do you think about it?

Sounds good to me.

Re: Bug in i386_process_record?

[Hui Zhu]

On Wed, Aug 26, 2009 at 11:19, Eli Zaretskii<eliz@gnu.org> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Wed, 26 Aug 2009 10:58:39 +0800
>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>
>> I add some code about it:
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_ES_REGNUM],
>>                                       &es);
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_DS_REGNUM],
>>                                       &ds);
>>           if (ir.aflag && (es != ds))
>>             {
>>
>> After that, we will not get the warning because the es is same with ds
>> in user level.
>>
>> What do you think about it?
>
> Sounds good to me.
>

Do you think it's OK to check in?

Hui

Re: Bug in i386_process_record?

[Eli Zaretskii]

> From: Hui Zhu <teawater@gmail.com>
> Date: Wed, 26 Aug 2009 11:27:06 +0800
> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
> 
> >> After that, we will not get the warning because the es is same with ds
> >> in user level.
> >>
> >> What do you think about it?
> >
> > Sounds good to me.
> >
> 
> Do you think it's OK to check in?

I was talking about your suggestion not to display a warning.

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>> From: Hui Zhu <teawater@gmail.com>
>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>
>>> It seems that the segment (It is not the section)  registers in x86
>>> protect mode is just help MMU to get the physical address.  It's
>>> transparent for the user level program.
>> It's transparent if $es and $ds have the same value (which they
>> usually do, AFAIK).
>>
>>> What do you think about remove this warning from this patch?
>> I would indeed do that, if we find that $es and $ds have the same
>> values.  Assuming that someone who knows Linux better than I do
>> confirms that these two registers hold the same selector when a normal
>> application is running in user mode.
>>
> 
> Thanks for remind me.  We cannot get the value of each segment
> register, but we can get each segment register point to.  So if the
> value of segment registers, it's means that the value of them is same.
> 
> I add some code about it:
>           regcache_raw_read_unsigned (ir.regcache,
>                                       ir.regmap[X86_RECORD_ES_REGNUM],
>                                       &es);
>           regcache_raw_read_unsigned (ir.regcache,
>                                       ir.regmap[X86_RECORD_DS_REGNUM],
>                                       &ds);
>           if (ir.aflag && (es != ds))
>             {
> 
> After that, we will not get the warning because the es is same with ds
> in user level.
> 
> What do you think about it?

I think it is the best version I have seen so far.
And it seems to follow the conclusions of the discussion.
And I've tested it, and it seems to work.

I would say wait until end-of-business Friday, and
if there are no more comments, check it in!

Michael



> 2009-08-26  Hui Zhu  <teawater@gmail.com>
> 
> 	* i386-tdep.c (i386_process_record): Fix the error of string
> 	ops instructions's handler.
> ---
>  i386-tdep.c |   69 ++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 33 insertions(+), 36 deletions(-)
> 
> --- a/i386-tdep.c
> +++ b/i386-tdep.c
> @@ -4441,50 +4441,47 @@ reswitch:
>        /* insS */
>      case 0x6c:
>      case 0x6d:
> -      if ((opcode & 1) == 0)
> -	ir.ot = OT_BYTE;
> -      else
> -	ir.ot = ir.dflag + OT_WORD;
>        regcache_raw_read_unsigned (ir.regcache,
> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>                                    &tmpulongest);
> -      if (!ir.aflag)
> -        {
> -          tmpulongest &= 0xffff;
> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
> -          if (record_debug)
> -            printf_unfiltered (_("Process record ignores the memory change "
> -                                 "of instruction at address 0x%s because "
> -                                 "it can't get the value of the segment "
> -                                 "register.\n"),
> -                               paddress (gdbarch, ir.addr));
> -        }
> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> +      if (tmpulongest)
>          {
> -          ULONGEST count, eflags;
> +          ULONGEST es, ds;
> +
> +          if ((opcode & 1) == 0)
> +	    ir.ot = OT_BYTE;
> +          else
> +	    ir.ot = ir.dflag + OT_WORD;
>            regcache_raw_read_unsigned (ir.regcache,
>                                        ir.regmap[X86_RECORD_REDI_REGNUM],
> -                                      &count);
> -          if (!ir.aflag)
> -            count &= 0xffff;
> +                                      &tmpulongest);
> +
>            regcache_raw_read_unsigned (ir.regcache,
> -                                      ir.regmap[X86_RECORD_EFLAGS_REGNUM],
> -                                      &eflags);
> -          if ((eflags >> 10) & 0x1)
> -            tmpulongest -= (count - 1) * (1 << ir.ot);
> -          if (record_arch_list_add_mem (tmpulongest, count * (1 << ir.ot)))
> -            return -1;
> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
> -        }
> -      else
> -        {
> +                                      ir.regmap[X86_RECORD_ES_REGNUM],
> +                                      &es);
> +          regcache_raw_read_unsigned (ir.regcache,
> +                                      ir.regmap[X86_RECORD_DS_REGNUM],
> +                                      &ds);
> +          if (ir.aflag && (es != ds))
> +            {
> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
> +              if (record_debug)
> +                printf_unfiltered (_("Process record ignores the memory "
> +				     "change of instruction at address 0x%s "
> +				     "because it can't get the value of the "
> +				     "ES segment register.\n"),
> +                                   paddress (gdbarch, ir.addr));
> +            }
> +
> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>            if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>              return -1;
> -        }
> -      if (opcode == 0xa4 || opcode == 0xa5)
> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> +          if (opcode == 0xa4 || opcode == 0xa5)
> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> +	}
>        break;
> 
>        /* cmpsS */

Re: Bug in i386_process_record?

[Michael Snyder]

Michael Snyder wrote:
> Hui Zhu wrote:
>> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>>> From: Hui Zhu <teawater@gmail.com>
>>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>>
>>>> It seems that the segment (It is not the section)  registers in x86
>>>> protect mode is just help MMU to get the physical address.  It's
>>>> transparent for the user level program.
>>> It's transparent if $es and $ds have the same value (which they
>>> usually do, AFAIK).
>>>
>>>> What do you think about remove this warning from this patch?
>>> I would indeed do that, if we find that $es and $ds have the same
>>> values.  Assuming that someone who knows Linux better than I do
>>> confirms that these two registers hold the same selector when a normal
>>> application is running in user mode.
>>>
>> Thanks for remind me.  We cannot get the value of each segment
>> register, but we can get each segment register point to.  So if the
>> value of segment registers, it's means that the value of them is same.
>>
>> I add some code about it:
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_ES_REGNUM],
>>                                       &es);
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_DS_REGNUM],
>>                                       &ds);
>>           if (ir.aflag && (es != ds))
>>             {
>>
>> After that, we will not get the warning because the es is same with ds
>> in user level.
>>
>> What do you think about it?
> 
> I think it is the best version I have seen so far.
> And it seems to follow the conclusions of the discussion.
> And I've tested it, and it seems to work.
> 
> I would say wait until end-of-business Friday, and
> if there are no more comments, check it in!

Hui,

Do you think you could add some new tests to i386-reverse.exp,
to verify the string instructions?

Thanks,
Michael

Re: Bug in i386_process_record?

[Hui Zhu]

On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com> wrote:
> Michael Snyder wrote:
>>
>> Hui Zhu wrote:
>>>
>>> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>>>>
>>>>> From: Hui Zhu <teawater@gmail.com>
>>>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>>>
>>>>> It seems that the segment (It is not the section)  registers in x86
>>>>> protect mode is just help MMU to get the physical address.  It's
>>>>> transparent for the user level program.
>>>>
>>>> It's transparent if $es and $ds have the same value (which they
>>>> usually do, AFAIK).
>>>>
>>>>> What do you think about remove this warning from this patch?
>>>>
>>>> I would indeed do that, if we find that $es and $ds have the same
>>>> values.  Assuming that someone who knows Linux better than I do
>>>> confirms that these two registers hold the same selector when a normal
>>>> application is running in user mode.
>>>>
>>> Thanks for remind me.  We cannot get the value of each segment
>>> register, but we can get each segment register point to.  So if the
>>> value of segment registers, it's means that the value of them is same.
>>>
>>> I add some code about it:
>>>          regcache_raw_read_unsigned (ir.regcache,
>>>                                      ir.regmap[X86_RECORD_ES_REGNUM],
>>>                                      &es);
>>>          regcache_raw_read_unsigned (ir.regcache,
>>>                                      ir.regmap[X86_RECORD_DS_REGNUM],
>>>                                      &ds);
>>>          if (ir.aflag && (es != ds))
>>>            {
>>>
>>> After that, we will not get the warning because the es is same with ds
>>> in user level.
>>>
>>> What do you think about it?
>>
>> I think it is the best version I have seen so far.
>> And it seems to follow the conclusions of the discussion.
>> And I've tested it, and it seems to work.
>>
>> I would say wait until end-of-business Friday, and
>> if there are no more comments, check it in!
>
> Hui,
>
> Do you think you could add some new tests to i386-reverse.exp,
> to verify the string instructions?
>
> Thanks,
> Michael
>

OK. I will do it.

Thanks,
Hui

Re: Bug in i386_process_record?

[Hui Zhu]

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

On Thu, Aug 27, 2009 at 09:43, Hui Zhu<teawater@gmail.com> wrote:
> On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com> wrote:
>> Michael Snyder wrote:
>>>
>>> Hui Zhu wrote:
>>>>
>>>> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>>>>>
>>>>>> From: Hui Zhu <teawater@gmail.com>
>>>>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>>>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>>>>
>>>>>> It seems that the segment (It is not the section)  registers in x86
>>>>>> protect mode is just help MMU to get the physical address.  It's
>>>>>> transparent for the user level program.
>>>>>
>>>>> It's transparent if $es and $ds have the same value (which they
>>>>> usually do, AFAIK).
>>>>>
>>>>>> What do you think about remove this warning from this patch?
>>>>>
>>>>> I would indeed do that, if we find that $es and $ds have the same
>>>>> values.  Assuming that someone who knows Linux better than I do
>>>>> confirms that these two registers hold the same selector when a normal
>>>>> application is running in user mode.
>>>>>
>>>> Thanks for remind me.  We cannot get the value of each segment
>>>> register, but we can get each segment register point to.  So if the
>>>> value of segment registers, it's means that the value of them is same.
>>>>
>>>> I add some code about it:
>>>>          regcache_raw_read_unsigned (ir.regcache,
>>>>                                      ir.regmap[X86_RECORD_ES_REGNUM],
>>>>                                      &es);
>>>>          regcache_raw_read_unsigned (ir.regcache,
>>>>                                      ir.regmap[X86_RECORD_DS_REGNUM],
>>>>                                      &ds);
>>>>          if (ir.aflag && (es != ds))
>>>>            {
>>>>
>>>> After that, we will not get the warning because the es is same with ds
>>>> in user level.
>>>>
>>>> What do you think about it?
>>>
>>> I think it is the best version I have seen so far.
>>> And it seems to follow the conclusions of the discussion.
>>> And I've tested it, and it seems to work.
>>>
>>> I would say wait until end-of-business Friday, and
>>> if there are no more comments, check it in!
>>
>> Hui,
>>
>> Do you think you could add some new tests to i386-reverse.exp,
>> to verify the string instructions?
>>
>> Thanks,
>> Michael
>>
>
> OK. I will do it.
>
> Thanks,
> Hui
>

Hi Michael,

I make a patch to add the test for string insn.

Please help me review it.

Thanks,
Hui

2009-08-27  Hui Zhu  <teawater@gmail.com>

	* gdb.reverse/i386-reverse.c (string_insn_tests): New function.
	(main): Call "string_insn_tests".

---
 testsuite/gdb.reverse/i386-reverse.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- a/testsuite/gdb.reverse/i386-reverse.c
+++ b/testsuite/gdb.reverse/i386-reverse.c
@@ -38,9 +38,25 @@ inc_dec_tests (void)
   asm ("dec %edi");
 } /* end inc_dec_tests */

+void
+string_insn_tests (void)
+{
+  register char x asm("ax");
+  char *dstp = (char *) 1;
+  int d0;
+  int len = 0;
+
+  asm volatile("rep\n"
+	       "stosb" /* %0, %2, %3 */ :
+	       "=D" (dstp), "=c" (d0) :
+	       "0" (dstp), "1" (len), "a" (x) :
+	       "memory");
+}
+
 int
 main ()
 {
   inc_dec_tests ();
+  string_insn_tests ();
   return 0;	/* end of main */
 }

[-- Attachment #2: prec-testsuite-string-insn.txt --]
[-- Type: text/plain, Size: 658 bytes --]

---
 testsuite/gdb.reverse/i386-reverse.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- a/testsuite/gdb.reverse/i386-reverse.c
+++ b/testsuite/gdb.reverse/i386-reverse.c
@@ -38,9 +38,25 @@ inc_dec_tests (void)
   asm ("dec %edi");
 } /* end inc_dec_tests */
 
+void
+string_insn_tests (void)
+{
+  register char x asm("ax");
+  char *dstp = (char *) 1;
+  int d0;
+  int len = 0;
+
+  asm volatile("rep\n"
+	       "stosb" /* %0, %2, %3 */ :
+	       "=D" (dstp), "=c" (d0) :
+	       "0" (dstp), "1" (len), "a" (x) :
+	       "memory");
+}
+
 int 
 main ()
 {
   inc_dec_tests ();
+  string_insn_tests ();
   return 0;	/* end of main */
 }

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Thu, Aug 27, 2009 at 09:43, Hui Zhu<teawater@gmail.com> wrote:
>> On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com> wrote:
>>> Do you think you could add some new tests to i386-reverse.exp,
>>> to verify the string instructions?
>>>
>>> Thanks,
>>> Michael
>>>
>> OK. I will do it.
>>
>> Thanks,
>> Hui
>>
> 
> Hi Michael,
> 
> I make a patch to add the test for string insn.
> 
> Please help me review it.

Good start -- but you need to write some expect script to go with it!
;-)


> 2009-08-27  Hui Zhu  <teawater@gmail.com>
> 
> 	* gdb.reverse/i386-reverse.c (string_insn_tests): New function.
> 	(main): Call "string_insn_tests".
> 
> ---
>  testsuite/gdb.reverse/i386-reverse.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> --- a/testsuite/gdb.reverse/i386-reverse.c
> +++ b/testsuite/gdb.reverse/i386-reverse.c
> @@ -38,9 +38,25 @@ inc_dec_tests (void)
>    asm ("dec %edi");
>  } /* end inc_dec_tests */
> 
> +void
> +string_insn_tests (void)
> +{
> +  register char x asm("ax");
> +  char *dstp = (char *) 1;
> +  int d0;
> +  int len = 0;
> +
> +  asm volatile("rep\n"
> +	       "stosb" /* %0, %2, %3 */ :
> +	       "=D" (dstp), "=c" (d0) :
> +	       "0" (dstp), "1" (len), "a" (x) :
> +	       "memory");
> +}
> +
>  int
>  main ()
>  {
>    inc_dec_tests ();
> +  string_insn_tests ();
>    return 0;	/* end of main */
>  }

Re: Bug in i386_process_record?

[Hui Zhu]

On Fri, Aug 28, 2009 at 09:35, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Thu, Aug 27, 2009 at 09:43, Hui Zhu<teawater@gmail.com> wrote:
>>>
>>> On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com> wrote:
>>>>
>>>> Do you think you could add some new tests to i386-reverse.exp,
>>>> to verify the string instructions?
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>> OK. I will do it.
>>>
>>> Thanks,
>>> Hui
>>>
>>
>> Hi Michael,
>>
>> I make a patch to add the test for string insn.
>>
>> Please help me review it.
>
> Good start -- but you need to write some expect script to go with it!
> ;-)

Hi Michael,

This patch can make inferior without string_insn_patch get fail in:
gdb_test "continue" \
    " end of main .*" \
    "continue to end of main"
Prec will get error in asm volatile("rep\n" line when continue.

Do you think I need make string_insn test  divide with inc_test in
expect script?

Thanks,
Hui

>
>
>> 2009-08-27  Hui Zhu  <teawater@gmail.com>
>>
>>        * gdb.reverse/i386-reverse.c (string_insn_tests): New function.
>>        (main): Call "string_insn_tests".
>>
>> ---
>>  testsuite/gdb.reverse/i386-reverse.c |   16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> --- a/testsuite/gdb.reverse/i386-reverse.c
>> +++ b/testsuite/gdb.reverse/i386-reverse.c
>> @@ -38,9 +38,25 @@ inc_dec_tests (void)
>>   asm ("dec %edi");
>>  } /* end inc_dec_tests */
>>
>> +void
>> +string_insn_tests (void)
>> +{
>> +  register char x asm("ax");
>> +  char *dstp = (char *) 1;
>> +  int d0;
>> +  int len = 0;
>> +
>> +  asm volatile("rep\n"
>> +              "stosb" /* %0, %2, %3 */ :
>> +              "=D" (dstp), "=c" (d0) :
>> +              "0" (dstp), "1" (len), "a" (x) :
>> +              "memory");
>> +}
>> +
>>  int
>>  main ()
>>  {
>>   inc_dec_tests ();
>> +  string_insn_tests ();
>>   return 0;    /* end of main */
>>  }
>
>

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Fri, Aug 28, 2009 at 09:35, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> On Thu, Aug 27, 2009 at 09:43, Hui Zhu<teawater@gmail.com> wrote:
>>>> On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com> wrote:
>>>>> Do you think you could add some new tests to i386-reverse.exp,
>>>>> to verify the string instructions?
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>> OK. I will do it.
>>>>
>>>> Thanks,
>>>> Hui
>>>>
>>> Hi Michael,
>>>
>>> I make a patch to add the test for string insn.
>>>
>>> Please help me review it.
>> Good start -- but you need to write some expect script to go with it!
>> ;-)
> 
> Hi Michael,
> 
> This patch can make inferior without string_insn_patch get fail in:
> gdb_test "continue" \
>     " end of main .*" \
>     "continue to end of main"
> Prec will get error in asm volatile("rep\n" line when continue.
> 
> Do you think I need make string_insn test  divide with inc_test in
> expect script?

My intention when I wrote the i386-reverse test was
that it should be extended with more tests over time.

In fact, I had this one in mind.   ;-)

Don't worry about "without string_insn_patch", since you will
check it in tomorrow.  You will check in this test later than
that, so nobody will get this test unles they already have the
string_insn_patch.

Re: Bug in i386_process_record?

[Hui Zhu]

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

On Fri, Aug 28, 2009 at 11:06, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Fri, Aug 28, 2009 at 09:35, Michael Snyder<msnyder@vmware.com> wrote:
>>>
>>> Hui Zhu wrote:
>>>>
>>>> On Thu, Aug 27, 2009 at 09:43, Hui Zhu<teawater@gmail.com> wrote:
>>>>>
>>>>> On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com>
>>>>> wrote:
>>>>>>
>>>>>> Do you think you could add some new tests to i386-reverse.exp,
>>>>>> to verify the string instructions?
>>>>>>
>>>>>> Thanks,
>>>>>> Michael
>>>>>>
>>>>> OK. I will do it.
>>>>>
>>>>> Thanks,
>>>>> Hui
>>>>>
>>>> Hi Michael,
>>>>
>>>> I make a patch to add the test for string insn.
>>>>
>>>> Please help me review it.
>>>
>>> Good start -- but you need to write some expect script to go with it!
>>> ;-)
>>
>> Hi Michael,
>>
>> This patch can make inferior without string_insn_patch get fail in:
>> gdb_test "continue" \
>>    " end of main .*" \
>>    "continue to end of main"
>> Prec will get error in asm volatile("rep\n" line when continue.
>>
>> Do you think I need make string_insn test  divide with inc_test in
>> expect script?
>
> My intention when I wrote the i386-reverse test was
> that it should be extended with more tests over time.
>
> In fact, I had this one in mind.   ;-)
>
> Don't worry about "without string_insn_patch", since you will
> check it in tomorrow.  You will check in this test later than
> that, so nobody will get this test unles they already have the
> string_insn_patch.

OK.

I make a new version patch that divide the inc_test and
string_insn_test.  Please help me review it.

Thanks,
Hui

2009-08-28  Hui Zhu  <teawater@gmail.com>

	* gdb.reverse/i386-reverse.c (string_insn_tests): New function.
	(main): Call "string_insn_tests".
	* gdb.reverse/i386-reverse.exp: Add test for string insn.

---
 testsuite/gdb.reverse/i386-reverse.c   |   16 ++++++++++++++++
 testsuite/gdb.reverse/i386-reverse.exp |   27 +++++++++++++++++----------
 2 files changed, 33 insertions(+), 10 deletions(-)

--- a/testsuite/gdb.reverse/i386-reverse.c
+++ b/testsuite/gdb.reverse/i386-reverse.c
@@ -38,9 +38,25 @@ inc_dec_tests (void)
   asm ("dec %edi");
 } /* end inc_dec_tests */

+void
+string_insn_tests (void)
+{
+  register char x asm("ax");
+  char *dstp = (char *) 1;
+  int d0;
+  int len = 0;
+
+  asm volatile("rep\n"
+	       "stosb" /* %0, %2, %3 */ :
+	       "=D" (dstp), "=c" (d0) :
+	       "0" (dstp), "1" (len), "a" (x) :
+	       "memory");
+} /* end string_insn_tests */
+
 int
 main ()
 {
   inc_dec_tests ();
+  string_insn_tests ();
   return 0;	/* end of main */
 }
--- a/testsuite/gdb.reverse/i386-reverse.exp
+++ b/testsuite/gdb.reverse/i386-reverse.exp
@@ -53,6 +53,7 @@ if { [gdb_compile "${srcdir}/${subdir}/$

 set end_of_main          [gdb_get_line_number " end of main "]
 set end_of_inc_dec_tests [gdb_get_line_number " end inc_dec_tests "]
+set string_insn_tests    [gdb_get_line_number " end string_insn_tests "]

 # Get things started.

@@ -201,21 +202,13 @@ gdb_expect {

 # gdb_test "step" "end inc_dec_tests .*" "step to end inc_dec_tests 1st time"

-gdb_test "break $end_of_main" \
-    "Breakpoint $decimal at .* line $end_of_main\." \
-    "set breakpoint at end of main"
-
-gdb_test "continue" \
-    " end of main .*" \
-    "continue to end of main"
-
 gdb_test "break $end_of_inc_dec_tests" \
     "Breakpoint $decimal at .* line $end_of_inc_dec_tests\." \
     "set breakpoint at end of inc_dec_tests"

-gdb_test "reverse-continue" \
+gdb_test "continue" \
     " end inc_dec_tests .*" \
-    "reverse to inc_dec_tests"
+    "continue to inc_dec_tests"

 #
 # Now reverse step, and check register values.
@@ -285,4 +278,18 @@ gdb_test "info reg eax" "eax *$predec_ea
 gdb_test "reverse-step" "inc .eax.*" "reverse-step to inc eax"
 gdb_test "info reg eax" "eax *$preinc_eax\t.*" "eax after reverse-inc"

+gdb_test "continue" \
+    " end inc_dec_tests .*" \
+    "continue to inc_dec_tests"

+#
+# Test string instruction.
+#
+
+gdb_test "break $string_insn_tests" \
+    "Breakpoint $decimal at .* line $string_insn_tests\." \
+    "set breakpoint at end of string_insn_tests"
+
+gdb_test "continue" \
+    " end string_insn_tests .*" \
+    "continue to inc_dec_tests"

[-- Attachment #2: prec-testsuite-string-insn.txt --]
[-- Type: text/plain, Size: 2418 bytes --]

---
 testsuite/gdb.reverse/i386-reverse.c   |   16 ++++++++++++++++
 testsuite/gdb.reverse/i386-reverse.exp |   27 +++++++++++++++++----------
 2 files changed, 33 insertions(+), 10 deletions(-)

--- a/testsuite/gdb.reverse/i386-reverse.c
+++ b/testsuite/gdb.reverse/i386-reverse.c
@@ -38,9 +38,25 @@ inc_dec_tests (void)
   asm ("dec %edi");
 } /* end inc_dec_tests */
 
+void
+string_insn_tests (void)
+{
+  register char x asm("ax");
+  char *dstp = (char *) 1;
+  int d0;
+  int len = 0;
+
+  asm volatile("rep\n"
+	       "stosb" /* %0, %2, %3 */ :
+	       "=D" (dstp), "=c" (d0) :
+	       "0" (dstp), "1" (len), "a" (x) :
+	       "memory");
+} /* end string_insn_tests */
+
 int 
 main ()
 {
   inc_dec_tests ();
+  string_insn_tests ();
   return 0;	/* end of main */
 }
--- a/testsuite/gdb.reverse/i386-reverse.exp
+++ b/testsuite/gdb.reverse/i386-reverse.exp
@@ -53,6 +53,7 @@ if { [gdb_compile "${srcdir}/${subdir}/$
 
 set end_of_main          [gdb_get_line_number " end of main "]
 set end_of_inc_dec_tests [gdb_get_line_number " end inc_dec_tests "]
+set string_insn_tests    [gdb_get_line_number " end string_insn_tests "]
 
 # Get things started.
 
@@ -201,21 +202,13 @@ gdb_expect {
 
 # gdb_test "step" "end inc_dec_tests .*" "step to end inc_dec_tests 1st time"
 
-gdb_test "break $end_of_main" \
-    "Breakpoint $decimal at .* line $end_of_main\." \
-    "set breakpoint at end of main"
-
-gdb_test "continue" \
-    " end of main .*" \
-    "continue to end of main"
-
 gdb_test "break $end_of_inc_dec_tests" \
     "Breakpoint $decimal at .* line $end_of_inc_dec_tests\." \
     "set breakpoint at end of inc_dec_tests"
 
-gdb_test "reverse-continue" \
+gdb_test "continue" \
     " end inc_dec_tests .*" \
-    "reverse to inc_dec_tests"
+    "continue to inc_dec_tests"
 
 #
 # Now reverse step, and check register values.
@@ -285,4 +278,18 @@ gdb_test "info reg eax" "eax *$predec_ea
 gdb_test "reverse-step" "inc .eax.*" "reverse-step to inc eax"
 gdb_test "info reg eax" "eax *$preinc_eax\t.*" "eax after reverse-inc"
 
+gdb_test "continue" \
+    " end inc_dec_tests .*" \
+    "continue to inc_dec_tests"
 
+#
+# Test string instruction.
+#
+
+gdb_test "break $string_insn_tests" \
+    "Breakpoint $decimal at .* line $string_insn_tests\." \
+    "set breakpoint at end of string_insn_tests"
+
+gdb_test "continue" \
+    " end string_insn_tests .*" \
+    "continue to inc_dec_tests"

Re: Bug in i386_process_record?

[Michael Snyder]

Hui Zhu wrote:
> On Fri, Aug 28, 2009 at 11:06, Michael Snyder<msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> On Fri, Aug 28, 2009 at 09:35, Michael Snyder<msnyder@vmware.com> wrote:
>>>> Hui Zhu wrote:
>>>>> On Thu, Aug 27, 2009 at 09:43, Hui Zhu<teawater@gmail.com> wrote:
>>>>>> On Thu, Aug 27, 2009 at 08:28, Michael Snyder<msnyder@vmware.com>
>>>>>> wrote:
>>>>>>> Do you think you could add some new tests to i386-reverse.exp,
>>>>>>> to verify the string instructions?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Michael
>>>>>>>
>>>>>> OK. I will do it.
>>>>>>
>>>>>> Thanks,
>>>>>> Hui
>>>>>>
>>>>> Hi Michael,
>>>>>
>>>>> I make a patch to add the test for string insn.
>>>>>
>>>>> Please help me review it.
>>>> Good start -- but you need to write some expect script to go with it!
>>>> ;-)
>>> Hi Michael,
>>>
>>> This patch can make inferior without string_insn_patch get fail in:
>>> gdb_test "continue" \
>>>    " end of main .*" \
>>>    "continue to end of main"
>>> Prec will get error in asm volatile("rep\n" line when continue.
>>>
>>> Do you think I need make string_insn test  divide with inc_test in
>>> expect script?
>> My intention when I wrote the i386-reverse test was
>> that it should be extended with more tests over time.
>>
>> In fact, I had this one in mind.   ;-)
>>
>> Don't worry about "without string_insn_patch", since you will
>> check it in tomorrow.  You will check in this test later than
>> that, so nobody will get this test unles they already have the
>> string_insn_patch.
> 
> OK.
> 
> I make a new version patch that divide the inc_test and
> string_insn_test.  Please help me review it.

So far, so good.   ;-)

Now, you need to actually test something.
Like, record the value of the memory before and after the instruction,
then reverse step the instruction, and test that the value of the
memory actually reverts to its earlier value.

Re: Bug in i386_process_record?

[Hui Zhu]

On Thu, Aug 27, 2009 at 07:45, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>>>
>>>> From: Hui Zhu <teawater@gmail.com>
>>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>>
>>>> It seems that the segment (It is not the section)  registers in x86
>>>> protect mode is just help MMU to get the physical address.  It's
>>>> transparent for the user level program.
>>>
>>> It's transparent if $es and $ds have the same value (which they
>>> usually do, AFAIK).
>>>
>>>> What do you think about remove this warning from this patch?
>>>
>>> I would indeed do that, if we find that $es and $ds have the same
>>> values.  Assuming that someone who knows Linux better than I do
>>> confirms that these two registers hold the same selector when a normal
>>> application is running in user mode.
>>>
>>
>> Thanks for remind me.  We cannot get the value of each segment
>> register, but we can get each segment register point to.  So if the
>> value of segment registers, it's means that the value of them is same.
>>
>> I add some code about it:
>>          regcache_raw_read_unsigned (ir.regcache,
>>                                      ir.regmap[X86_RECORD_ES_REGNUM],
>>                                      &es);
>>          regcache_raw_read_unsigned (ir.regcache,
>>                                      ir.regmap[X86_RECORD_DS_REGNUM],
>>                                      &ds);
>>          if (ir.aflag && (es != ds))
>>            {
>>
>> After that, we will not get the warning because the es is same with ds
>> in user level.
>>
>> What do you think about it?
>
> I think it is the best version I have seen so far.
> And it seems to follow the conclusions of the discussion.
> And I've tested it, and it seems to work.
>
> I would say wait until end-of-business Friday, and
> if there are no more comments, check it in!
>
> Michael
>

OK.

Thanks,
Hui

>> 2009-08-26  Hui Zhu  <teawater@gmail.com>
>>
>>        * i386-tdep.c (i386_process_record): Fix the error of string
>>        ops instructions's handler.
>> ---
>>  i386-tdep.c |   69
>> ++++++++++++++++++++++++++++--------------------------------
>>  1 file changed, 33 insertions(+), 36 deletions(-)
>>
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -4441,50 +4441,47 @@ reswitch:
>>       /* insS */
>>     case 0x6c:
>>     case 0x6d:
>> -      if ((opcode & 1) == 0)
>> -       ir.ot = OT_BYTE;
>> -      else
>> -       ir.ot = ir.dflag + OT_WORD;
>>       regcache_raw_read_unsigned (ir.regcache,
>> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
>> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>>                                   &tmpulongest);
>> -      if (!ir.aflag)
>> -        {
>> -          tmpulongest &= 0xffff;
>> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
>> -          if (record_debug)
>> -            printf_unfiltered (_("Process record ignores the memory
>> change "
>> -                                 "of instruction at address 0x%s because
>> "
>> -                                 "it can't get the value of the segment "
>> -                                 "register.\n"),
>> -                               paddress (gdbarch, ir.addr));
>> -        }
>> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> +      if (tmpulongest)
>>         {
>> -          ULONGEST count, eflags;
>> +          ULONGEST es, ds;
>> +
>> +          if ((opcode & 1) == 0)
>> +           ir.ot = OT_BYTE;
>> +          else
>> +           ir.ot = ir.dflag + OT_WORD;
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_REDI_REGNUM],
>> -                                      &count);
>> -          if (!ir.aflag)
>> -            count &= 0xffff;
>> +                                      &tmpulongest);
>> +
>>           regcache_raw_read_unsigned (ir.regcache,
>> -
>>  ir.regmap[X86_RECORD_EFLAGS_REGNUM],
>> -                                      &eflags);
>> -          if ((eflags >> 10) & 0x1)
>> -            tmpulongest -= (count - 1) * (1 << ir.ot);
>> -          if (record_arch_list_add_mem (tmpulongest, count * (1 <<
>> ir.ot)))
>> -            return -1;
>> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>> -        }
>> -      else
>> -        {
>> +                                      ir.regmap[X86_RECORD_ES_REGNUM],
>> +                                      &es);
>> +          regcache_raw_read_unsigned (ir.regcache,
>> +                                      ir.regmap[X86_RECORD_DS_REGNUM],
>> +                                      &ds);
>> +          if (ir.aflag && (es != ds))
>> +            {
>> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) <<
>> 4; */
>> +              if (record_debug)
>> +                printf_unfiltered (_("Process record ignores the memory "
>> +                                    "change of instruction at address
>> 0x%s "
>> +                                    "because it can't get the value of
>> the "
>> +                                    "ES segment register.\n"),
>> +                                   paddress (gdbarch, ir.addr));
>> +            }
>> +
>> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>>           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>>             return -1;
>> -        }
>> -      if (opcode == 0xa4 || opcode == 0xa5)
>> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>> +          if (opcode == 0xa4 || opcode == 0xa5)
>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>> +       }
>>       break;
>>
>>       /* cmpsS */
>
>

Re: Bug in i386_process_record?

[Hui Zhu]

On Thu, Aug 27, 2009 at 07:45, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>>>
>>>> From: Hui Zhu <teawater@gmail.com>
>>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>>
>>>> It seems that the segment (It is not the section)  registers in x86
>>>> protect mode is just help MMU to get the physical address.  It's
>>>> transparent for the user level program.
>>>
>>> It's transparent if $es and $ds have the same value (which they
>>> usually do, AFAIK).
>>>
>>>> What do you think about remove this warning from this patch?
>>>
>>> I would indeed do that, if we find that $es and $ds have the same
>>> values.  Assuming that someone who knows Linux better than I do
>>> confirms that these two registers hold the same selector when a normal
>>> application is running in user mode.
>>>
>>
>> Thanks for remind me.  We cannot get the value of each segment
>> register, but we can get each segment register point to.  So if the
>> value of segment registers, it's means that the value of them is same.
>>
>> I add some code about it:
>>          regcache_raw_read_unsigned (ir.regcache,
>>                                      ir.regmap[X86_RECORD_ES_REGNUM],
>>                                      &es);
>>          regcache_raw_read_unsigned (ir.regcache,
>>                                      ir.regmap[X86_RECORD_DS_REGNUM],
>>                                      &ds);
>>          if (ir.aflag && (es != ds))
>>            {
>>
>> After that, we will not get the warning because the es is same with ds
>> in user level.
>>
>> What do you think about it?
>
> I think it is the best version I have seen so far.
> And it seems to follow the conclusions of the discussion.
> And I've tested it, and it seems to work.
>
> I would say wait until end-of-business Friday, and
> if there are no more comments, check it in!
>
Checked in.

Thanks,
Hui

>
>
>
>> 2009-08-26  Hui Zhu  <teawater@gmail.com>
>>
>>        * i386-tdep.c (i386_process_record): Fix the error of string
>>        ops instructions's handler.
>> ---
>>  i386-tdep.c |   69
>> ++++++++++++++++++++++++++++--------------------------------
>>  1 file changed, 33 insertions(+), 36 deletions(-)
>>
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -4441,50 +4441,47 @@ reswitch:
>>       /* insS */
>>     case 0x6c:
>>     case 0x6d:
>> -      if ((opcode & 1) == 0)
>> -       ir.ot = OT_BYTE;
>> -      else
>> -       ir.ot = ir.dflag + OT_WORD;
>>       regcache_raw_read_unsigned (ir.regcache,
>> -                                  ir.regmap[X86_RECORD_REDI_REGNUM],
>> +                                  ir.regmap[X86_RECORD_RECX_REGNUM],
>>                                   &tmpulongest);
>> -      if (!ir.aflag)
>> -        {
>> -          tmpulongest &= 0xffff;
>> -          /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */
>> -          if (record_debug)
>> -            printf_unfiltered (_("Process record ignores the memory
>> change "
>> -                                 "of instruction at address 0x%s because
>> "
>> -                                 "it can't get the value of the segment "
>> -                                 "register.\n"),
>> -                               paddress (gdbarch, ir.addr));
>> -        }
>> -      if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> +      if (tmpulongest)
>>         {
>> -          ULONGEST count, eflags;
>> +          ULONGEST es, ds;
>> +
>> +          if ((opcode & 1) == 0)
>> +           ir.ot = OT_BYTE;
>> +          else
>> +           ir.ot = ir.dflag + OT_WORD;
>>           regcache_raw_read_unsigned (ir.regcache,
>>                                       ir.regmap[X86_RECORD_REDI_REGNUM],
>> -                                      &count);
>> -          if (!ir.aflag)
>> -            count &= 0xffff;
>> +                                      &tmpulongest);
>> +
>>           regcache_raw_read_unsigned (ir.regcache,
>> -
>>  ir.regmap[X86_RECORD_EFLAGS_REGNUM],
>> -                                      &eflags);
>> -          if ((eflags >> 10) & 0x1)
>> -            tmpulongest -= (count - 1) * (1 << ir.ot);
>> -          if (record_arch_list_add_mem (tmpulongest, count * (1 <<
>> ir.ot)))
>> -            return -1;
>> -          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>> -        }
>> -      else
>> -        {
>> +                                      ir.regmap[X86_RECORD_ES_REGNUM],
>> +                                      &es);
>> +          regcache_raw_read_unsigned (ir.regcache,
>> +                                      ir.regmap[X86_RECORD_DS_REGNUM],
>> +                                      &ds);
>> +          if (ir.aflag && (es != ds))
>> +            {
>> +              /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) <<
>> 4; */
>> +              if (record_debug)
>> +                printf_unfiltered (_("Process record ignores the memory "
>> +                                    "change of instruction at address
>> 0x%s "
>> +                                    "because it can't get the value of
>> the "
>> +                                    "ES segment register.\n"),
>> +                                   paddress (gdbarch, ir.addr));
>> +            }
>> +
>> +          if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ))
>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
>>           if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>>             return -1;
>> -        }
>> -      if (opcode == 0xa4 || opcode == 0xa5)
>> -        I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>> -      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>> +          if (opcode == 0xa4 || opcode == 0xa5)
>> +            I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RESI_REGNUM);
>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDI_REGNUM);
>> +          I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>> +       }
>>       break;
>>
>>       /* cmpsS */
>
>

Re: Bug in i386_process_record?

[Eli Zaretskii]

> From: Hui Zhu <teawater@gmail.com>
> Date: Mon, 24 Aug 2009 07:42:41 +0800
> Cc: gdb-patches@sourceware.org
> 
> For the gdb, the value of segment reg is not the really value.
> cs             0x73	115
> ss             0x7b	123
> ds             0x7b	123
> es             0x7b	123
> fs             0x0	0
> gs             0x33	51
> I have tried some insn that use segment reg such as string ops insn.
> I found that the value of this segment reg cannot affect anything.

Did you try setjmp and longjmp?

Re: Bug in i386_process_record?

[Hui Zhu]

On Tue, Aug 25, 2009 at 03:19, Eli Zaretskii<eliz@gnu.org> wrote:
>> From: Hui Zhu <teawater@gmail.com>
>> Date: Mon, 24 Aug 2009 07:42:41 +0800
>> Cc: gdb-patches@sourceware.org
>>
>> For the gdb, the value of segment reg is not the really value.
>> cs             0x73   115
>> ss             0x7b   123
>> ds             0x7b   123
>> es             0x7b   123
>> fs             0x0    0
>> gs             0x33   51
>> I have tried some insn that use segment reg such as string ops insn.
>> I found that the value of this segment reg cannot affect anything.
>
> Did you try setjmp and longjmp?
>

Signal and setjmp longjump is still not tested.  I will do it.

Thanks,
Hui

Re: Bug in i386_process_record?

[Eli Zaretskii]

> Date: Sat, 22 Aug 2009 21:07:38 -0700
> From: Michael Snyder <msnyder@vmware.com>
> CC: gdb-patches ml <gdb-patches@sourceware.org>
> 
> > Most of the string ops instruction will use segment register.
> > But I check the some linux program that have string ops insn.  I found
> > that in linux (maybe glibc), the value of the segment register is 0,
> > so it will not affect anything.
> 
> Hmm, ok, but this is i386-tdep.c, not i386-linux-tdep.c...

Exactly.  And what happens on systems where the segment register is
NOT zero?