This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA/prec] Make i386 handle segment register better
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: msnyder at vmware dot com
- Cc: teawater at gmail dot com, gdb-patches at sourceware dot org
- Date: Sat, 5 Sep 2009 10:14:57 +0200 (CEST)
- Subject: Re: [RFA/prec] Make i386 handle segment register better
- References: <daef60380908290853g10a263a2jfd0c5c08aa7e1ab3@mail.gmail.com> <4A999BC3.5020606@vmware.com> <daef60380908292008g4eaa82a1kbb8192b8e946af1e@mail.gmail.com> <4AA1CFD1.4000502@vmware.com>
> 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?
The code is basically ok, but I'd like to ask Hui to avoid using
meaningless variable names like "tmp".
> > 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
> > {
>
>