Re: [RFA/prec] Make i386 handle segment register better

[Hui Zhu <teawater@gmail.com>]

On Sat, Sep 5, 2009 at 16:14, Mark Kettenis<mark.kettenis@xs4all.nl> wrote:
>> Date: Fri, 04 Sep 2009 19:41:21 -0700
>> From: Michael Snyder <msnyder@vmware.com>
>>
>> Hui Zhu wrote:
>> > On Sun, Aug 30, 2009 at 05:21, Michael Snyder<msnyder@vmware.com> wrote:
>>
>> >> And this one is also an if/else.  So I guess my questions are:
>> >>
>> >> 1) Should you use an "else" in the "String ops" case?
>> >
>> > OK.
>> >
>> >> 2) Should we go ahead and record the register changes,
>> >> even though we can't record the memory change?
>> >
>> > I think even if we cannot record the memory change.  Keep record the
>> > change of reg is better.
>> >
>> >> 3) Should this be a warning, rather than just a debug message?
>> >> I think yes, because if this happens, it actually means that the
>> >> record log will be inaccurate.
>> >>
>> > OK.
>> >
>> >
>> > I make a new patch for it.  Please help me review it.
>>
>> I think I like this version.
>> Want to check it in?

Thanks for you help, Michael.

>
> The code is basically ok, but I'd like to ask Hui to avoid using
> meaningless variable names like "tmp".

Thanks for remind me, Mark.

I checked in this patch with change the "tmp" to "orv".


Hui


>
>> > 2009-08-30  Hui Zhu  <teawater@gmail.com>
>> >
>> >         * i386-tdep.c (i386_record_s): Add orig_addr.
>> >         (i386_record_check_override): New function.
>> >         (i386_record_lea_modrm): Call i386_record_check_override.
>> >         (i386_process_record): Ditto.
>> >
>> > ---
>> >  i386-tdep.c |  103 ++++++++++++++++++++++++++++++++++--------------------------
>> >  1 file changed, 59 insertions(+), 44 deletions(-)
>> >
>> > --- a/i386-tdep.c
>> > +++ b/i386-tdep.c
>> > @@ -2867,6 +2867,7 @@ struct i386_record_s
>> >  {
>> >    struct gdbarch *gdbarch;
>> >    struct regcache *regcache;
>> > +  CORE_ADDR orig_addr;
>> >    CORE_ADDR addr;
>> >    int aflag;
>> >    int dflag;
>> > @@ -3147,6 +3148,26 @@ no_rm:
>> >    return 0;
>> >  }
>> >
>> > +static int
>> > +i386_record_check_override (struct i386_record_s *irp)
>> > +{
>> > +  if (irp->override >= 0 && irp->override != X86_RECORD_DS_REGNUM)
>> > +    {
>> > +      ULONGEST tmp, ds;
>> > +
>> > +      regcache_raw_read_unsigned (irp->regcache,
>> > +                                  irp->regmap[irp->override],
>> > +                                  &tmp);
>> > +      regcache_raw_read_unsigned (irp->regcache,
>> > +                                  irp->regmap[X86_RECORD_DS_REGNUM],
>> > +                                  &ds);
>> > +      if (tmp != ds)
>> > +        return 1;
>> > +    }
>> > +
>> > +  return 0;
>> > +}
>> > +
>> >  /* Record the value of the memory that willbe changed in current instruction
>> >     to "record_arch_list".
>> >     Return -1 if something wrong. */
>> > @@ -3157,13 +3178,12 @@ i386_record_lea_modrm (struct i386_recor
>> >    struct gdbarch *gdbarch = irp->gdbarch;
>> >    uint64_t addr;
>> >
>> > -  if (irp->override >= 0)
>> > +  if (i386_record_check_override (irp))
>> >      {
>> > -      if (record_debug)
>> > -       printf_unfiltered (_("Process record ignores the memory change "
>> > -                            "of instruction at address %s because it "
>> > -                            "can't get the value of the segment register.\n"),
>> > -                          paddress (gdbarch, irp->addr));
>> > +      warning (_("Process record ignores the memory change "
>> > +                 "of instruction at address %s because it "
>> > +                 "can't get the value of the segment register."),
>> > +               paddress (gdbarch, irp->orig_addr));
>> >        return 0;
>> >      }
>> >
>> > @@ -3221,6 +3241,7 @@ i386_process_record (struct gdbarch *gdb
>> >    memset (&ir, 0, sizeof (struct i386_record_s));
>> >    ir.regcache = regcache;
>> >    ir.addr = addr;
>> > +  ir.orig_addr = addr;
>> >    ir.aflag = 1;
>> >    ir.dflag = 1;
>> >    ir.override = -1;
>> > @@ -4039,14 +4060,13 @@ reswitch:
>> >        /* mov EAX */
>> >      case 0xa2:
>> >      case 0xa3:
>> > -      if (ir.override >= 0)
>> > +      if (i386_record_check_override (&ir))
>> >          {
>> > -         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));
>> > +         warning (_("Process record ignores the memory change "
>> > +                     "of instruction at address 0x%s because "
>> > +                     "it can't get the value of the segment "
>> > +                     "register."),
>> > +                   paddress (gdbarch, ir.orig_addr));
>> >         }
>> >        else
>> >         {
>> > @@ -4458,27 +4478,24 @@ reswitch:
>> >                                        ir.regmap[X86_RECORD_REDI_REGNUM],
>> >                                        &tmpulongest);
>> >
>> > -          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))
>> > +          ir.override = X86_RECORD_ES_REGNUM;
>> > +          if (ir.aflag && i386_record_check_override (&ir))
>> >              {
>> >                /* 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));
>> > +              warning (_("Process record ignores the memory "
>> > +                         "change of instruction at address 0x%s "
>> > +                         "because it can't get the value of the "
>> > +                         "ES segment register."),
>> > +                       paddress (gdbarch, ir.orig_addr));
>> > +            }
>> > +          else
>> > +            {
>> > +              if (record_arch_list_add_mem (tmpulongest, 1 << ir.ot))
>> > +                return -1;
>> >              }
>> >
>> >            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);
>> > @@ -5086,15 +5103,14 @@ reswitch:
>> >                 opcode = opcode << 8 | ir.modrm;
>> >                 goto no_support;
>> >               }
>> > -           if (ir.override >= 0)
>> > +           if (i386_record_check_override (&ir))
>> >               {
>> > -               if (record_debug)
>> > -                 printf_unfiltered (_("Process record ignores the memory "
>> > -                                      "change of instruction at "
>> > -                                      "address %s because it can't get "
>> > -                                      "the value of the segment "
>> > -                                      "register.\n"),
>> > -                                    paddress (gdbarch, ir.addr));
>> > +               warning (_("Process record ignores the memory "
>> > +                           "change of instruction at "
>> > +                           "address %s because it can't get "
>> > +                           "the value of the segment "
>> > +                           "register."),
>> > +                         paddress (gdbarch, ir.orig_addr));
>> >               }
>> >             else
>> >               {
>> > @@ -5138,15 +5154,14 @@ reswitch:
>> >           else
>> >             {
>> >               /* sidt */
>> > -             if (ir.override >= 0)
>> > +             if (i386_record_check_override (&ir))
>> >                 {
>> > -                 if (record_debug)
>> > -                   printf_unfiltered (_("Process record ignores the memory "
>> > -                                        "change of instruction at "
>> > -                                        "address %s because it can't get "
>> > -                                        "the value of the segment "
>> > -                                        "register.\n"),
>> > -                                      paddress (gdbarch, ir.addr));
>> > +                 warning (_("Process record ignores the memory "
>> > +                             "change of instruction at "
>> > +                             "address %s because it can't get "
>> > +                             "the value of the segment "
>> > +                             "register."),
>> > +                           paddress (gdbarch, ir.orig_addr));
>> >                 }
>> >               else
>> >                 {
>>
>>
>