Re: Bug in i386_process_record?

[Michael Snyder <msnyder@vmware.com>]

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 */
>