Re: Bug in i386_process_record?

[Hui Zhu <teawater at gmail dot com>]

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: prec-fix-x86-strinsn.txt
Description: Text document