Re: Bug in i386_process_record?

Michael Snyder
2009-08-26T23:45:02Z

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!

Michael

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