Bug in i386_process_record?

[Hui Zhu]

On Thu, Aug 27, 2009 at 07:45, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> On Wed, Aug 26, 2009 at 02:42, Eli Zaretskii<eliz@gnu.org> wrote:
>>>>
>>>> From: Hui Zhu <teawater@gmail.com>
>>>> Date: Tue, 25 Aug 2009 13:02:44 +0800
>>>> Cc: msnyder@vmware.com, gdb-patches@sourceware.org
>>>>
>>>> It seems that the segment (It is not the section)  registers in x86
>>>> protect mode is just help MMU to get the physical address.  It's
>>>> transparent for the user level program.
>>>
>>> It's transparent if $es and $ds have the same value (which they
>>> usually do, AFAIK).
>>>
>>>> What do you think about remove this warning from this patch?
>>>
>>> I would indeed do that, if we find that $es and $ds have the same
>>> values.  Assuming that someone who knows Linux better than I do
>>> confirms that these two registers hold the same selector when a normal
>>> application is running in user mode.
>>>
>>
>> Thanks for remind me.  We cannot get the value of each segment
>> register, but we can get each segment register point to.  So if the
>> value of segment registers, it's means that the value of them is same.
>>
>> I add some code about it:
>>          regcache_raw_read_unsigned (ir.regcache,
>>                                      ir.regmap[X86_RECORD_ES_REGNUM],
>>                                      &es);
>>          regcache_raw_read_unsigned (ir.regcache,
>>                                      ir.regmap[X86_RECORD_DS_REGNUM],
>>                                      &ds);
>>          if (ir.aflag && (es != ds))
>>            {
>>
>> After that, we will not get the warning because the es is same with ds
>> in user level.
>>
>> What do you think about it?
>
> I think it is the best version I have seen so far.
> And it seems to follow the conclusions of the discussion.
> And I've tested it, and it seems to work.
>
> I would say wait until end-of-business Friday, and
> if there are no more comments, check it in!
>
Checked in.

Thanks,
Hui

>
>
>
>> 2009-08-26  Hui Zhu  <teawater@gmail.com>
>>
>>        * i386-tdep.c (i386_process_record): Fix the error of string
>>        ops instructions's handler.
>> ---
>>  i386-tdep.c |   69
>> ++++++++++++++++++++++++++++--------------------------------
>>  1 file changed, 33 insertions(+), 36 deletions(-)
>>
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -4441,50 +4441,47 @@ 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;
>> +          ULONGEST es, ds;
>> +
>> +          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;
>> +                                      &tmpulongest);
>> +
>>           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
>> -        {
>> +                                      ir.regmap[X86_RECORD_ES_REGNUM],
>> +                                      &es);
>> +          regcache_raw_read_unsigned (ir.regcache,
>> +                                      ir.regmap[X86_RECORD_DS_REGNUM],
>> +                                      &ds);
>> +          if (ir.aflag && (es != ds))
>> +            {
>> +              /* 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 "
>> +                                    "ES 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 */
>
>