Re: Bug in i386_process_record?

[Michael Snyder <msnyder at vmware dot com>]

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