This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
| Other format: | [Raw text] | |
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:
prec-i386-override.txt
Description: Text document
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |