From: Michael Snyder <msnyder@vmware.com>
To: Hui Zhu <teawater@gmail.com>
Cc: gdb-patches ml <gdb-patches@sourceware.org>
Subject: Re: [RFA/prec] Make i386 handle segment register better
Date: Sat, 05 Sep 2009 02:42:00 -0000 [thread overview]
Message-ID: <4AA1CFD1.4000502@vmware.com> (raw)
In-Reply-To: <daef60380908292008g4eaa82a1kbb8192b8e946af1e@mail.gmail.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?
Michael
>
> 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 2:42 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 [this message]
2009-09-05 8:15 ` Mark Kettenis
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=4AA1CFD1.4000502@vmware.com \
--to=msnyder@vmware.com \
--cc=gdb-patches@sourceware.org \
--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