From: Hui Zhu <teawater@gmail.com>
To: Michael Snyder <msnyder@vmware.com>
Cc: gdb-patches ml <gdb-patches@sourceware.org>
Subject: Re: [RFA/prec] Make i386 handle segment register better
Date: Sun, 30 Aug 2009 03:21:00 -0000 [thread overview]
Message-ID: <daef60380908292008g4eaa82a1kbb8192b8e946af1e@mail.gmail.com> (raw)
In-Reply-To: <4A999BC3.5020606@vmware.com>
[-- Attachment #1: Type: text/plain, Size: 11866 bytes --]
On Sun, Aug 30, 2009 at 05:21, Michael Snyder<msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> Hi guys,
>>
>> In prec-fix-x86-strinsn.txt patch, I add code the compare the ES and
>> DS to make sure if es if same with ds or not.
>> I think it works not bad, so I make a patch to check other segment
>> regiser like it.
>>
>> Please help me with it.
>
> Thanks for doing this!
> I think it looks good, but I have a couple of questions:
>
>> 2009-08-29 Hui Zhu <teawater@gmail.com>
>>
>> * i386-tdep.c (i386_record_check_override): New function.
>> (i386_record_lea_modrm): Call i386_record_check_override.
>> (i386_process_record): Ditto.
>>
>> ---
>> i386-tdep.c | 37 ++++++++++++++++++++++++++-----------
>> 1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -3147,6 +3147,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,7 +3177,7 @@ 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 "
>
> In this case, you "return 0", so it is true that we
> "ignore the memory change".
>
> In some cases below, you use an "if/else", so it is also
> true that we "ignore the memory change".
>
> But in the "String ops" case, there is no "else", so we
> really do *not* ignore the memory change.
>
> Should we be consistant, and add an "else" to the string ops case?
>
> See further comments at end.
>
>> @@ -4039,7 +4059,7 @@ 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
>> "
>
> OK, this one is an "if/else", so you don't record the memory.
>
>> @@ -4458,13 +4478,8 @@ 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)
>
> But in this case, there is no "else", so you still record
> the memory even if i386_record_check_override returns true.
>
>
>
>> @@ -5086,7 +5101,7 @@ 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 "
>
> This is an "if/else" so you don't record the memory.
>
>> @@ -5138,7 +5153,7 @@ reswitch:
>> else
>> {
>> /* sidt */
>> - if (ir.override >= 0)
>> + if (i386_record_check_override (&ir))
>> {
>> if (record_debug)
>> printf_unfiltered (_("Process record ignores the memory
>> "
>
> 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.
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
{
[-- Attachment #2: prec-i386-override.txt --]
[-- Type: text/plain, Size: 6296 bytes --]
---
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-08-30 3:09 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 [this message]
2009-09-05 2:42 ` Michael Snyder
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=daef60380908292008g4eaa82a1kbb8192b8e946af1e@mail.gmail.com \
--to=teawater@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=msnyder@vmware.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