Re: Bug in i386_process_record?

[Hui Zhu <teawater@gmail.com>]

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