Re: Bug in i386_process_record?

[Hui Zhu <teawater@gmail.com>]

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