From: Mark Kettenis <mark.kettenis@xs4all.nl>
To: msnyder@vmware.com
Cc: teawater@gmail.com, gdb-patches@sourceware.org
Subject: Re: [RFA/prec] Make i386 handle segment register better
Date: Sat, 05 Sep 2009 08:15:00 -0000 [thread overview]
Message-ID: <200909050814.n858EvHR016392@brahms.sibelius.xs4all.nl> (raw)
In-Reply-To: <4AA1CFD1.4000502@vmware.com> (message from Michael Snyder on Fri, 04 Sep 2009 19:41:21 -0700)
> 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
> > {
>
>
next prev parent reply other threads:[~2009-09-05 8:15 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-08-29 16:12 Hui Zhu
2009-08-29 21:34 ` Michael Snyder
2009-08-30 3:21 ` Hui Zhu
2009-09-05 2:42 ` Michael Snyder
2009-09-05 8:15 ` Mark Kettenis [this message]
2009-09-05 15:38 ` Hui Zhu
2009-09-06 6:52 ` Hui Zhu
2009-09-06 15:06 ` Hui Zhu
2009-09-07 0:07 ` Michael Snyder
2009-09-07 11:17 ` Hui Zhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200909050814.n858EvHR016392@brahms.sibelius.xs4all.nl \
--to=mark.kettenis@xs4all.nl \
--cc=gdb-patches@sourceware.org \
--cc=msnyder@vmware.com \
--cc=teawater@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox