Re: Bug in i386_process_record?

Michael Snyder
2009-08-23T03:11:33Z

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