* [RFA/prec] Make i386 handle segment register better
@ 2009-08-29 16:12 Hui Zhu
2009-08-29 21:34 ` Michael Snyder
0 siblings, 1 reply; 10+ messages in thread
From: Hui Zhu @ 2009-08-29 16:12 UTC (permalink / raw)
To: gdb-patches ml; +Cc: Michael Snyder
[-- Attachment #1: Type: text/plain, Size: 3167 bytes --]
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,
Hui
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 "
@@ -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 "
@@ -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)
@@ -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 "
@@ -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 "
[-- Attachment #2: prec-i386-override.txt --]
[-- Type: text/plain, Size: 2729 bytes --]
---
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 "
@@ -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 "
@@ -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)
@@ -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 "
@@ -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 "
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [RFA/prec] Make i386 handle segment register better 2009-08-29 16:12 [RFA/prec] Make i386 handle segment register better Hui Zhu @ 2009-08-29 21:34 ` Michael Snyder 2009-08-30 3:21 ` Hui Zhu 0 siblings, 1 reply; 10+ messages in thread From: Michael Snyder @ 2009-08-29 21:34 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml 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? 2) Should we go ahead and record the register changes, even though we can't record the memory change? 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. That's all for now, Michael ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-08-29 21:34 ` Michael Snyder @ 2009-08-30 3:21 ` Hui Zhu 2009-09-05 2:42 ` Michael Snyder 0 siblings, 1 reply; 10+ messages in thread From: Hui Zhu @ 2009-08-30 3:21 UTC (permalink / raw) To: Michael Snyder; +Cc: gdb-patches ml [-- 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 { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-08-30 3:21 ` Hui Zhu @ 2009-09-05 2:42 ` Michael Snyder 2009-09-05 8:15 ` Mark Kettenis 0 siblings, 1 reply; 10+ messages in thread From: Michael Snyder @ 2009-09-05 2:42 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml 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 > { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-09-05 2:42 ` Michael Snyder @ 2009-09-05 8:15 ` Mark Kettenis 2009-09-05 15:38 ` Hui Zhu 0 siblings, 1 reply; 10+ messages in thread From: Mark Kettenis @ 2009-09-05 8:15 UTC (permalink / raw) To: msnyder; +Cc: teawater, gdb-patches > 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 > > { > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-09-05 8:15 ` Mark Kettenis @ 2009-09-05 15:38 ` Hui Zhu 2009-09-06 6:52 ` Hui Zhu 0 siblings, 1 reply; 10+ messages in thread From: Hui Zhu @ 2009-09-05 15:38 UTC (permalink / raw) To: Mark Kettenis; +Cc: msnyder, gdb-patches On Sat, Sep 5, 2009 at 16:14, Mark Kettenis<mark.kettenis@xs4all.nl> wrote: >> 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? Thanks for you help, Michael. > > The code is basically ok, but I'd like to ask Hui to avoid using > meaningless variable names like "tmp". Thanks for remind me, Mark. I checked in this patch with change the "tmp" to "orv". Hui > >> > 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 >> > { >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-09-05 15:38 ` Hui Zhu @ 2009-09-06 6:52 ` Hui Zhu 2009-09-06 15:06 ` Hui Zhu 0 siblings, 1 reply; 10+ messages in thread From: Hui Zhu @ 2009-09-06 6:52 UTC (permalink / raw) To: Mark Kettenis, Michael Snyder; +Cc: gdb-patches Hi guys, Sorry I didn't do more test for this patch on amd64 before I check it in. But this patch really work not very good in amd64. For example: Process record: i386_process_record addr = 0x7ffff7b13cc9 signal = 0 Process record: add mem addr = 0xffffffffffffffa8 len = 4 to record list. Process record: error reading memory at addr = 0xffffffffffffffa8 len = 4. Process record: failed to record execution log. Program received signal SIGTRAP, Trace/breakpoint trap. 0x00007ffff7b13cc9 in pause () from /lib/libc.so.6 (gdb) disassemble Dump of assembler code for function pause: 0x00007ffff7b13c70 <pause+0>: cmpl $0x0,0x2c93d1(%rip) # 0x7ffff7ddd048 0x00007ffff7b13c77 <pause+7>: jne 0x7ffff7b13c89 <pause+25> 0x00007ffff7b13c79 <pause+9>: mov $0x22,%eax 0x00007ffff7b13c7e <pause+14>: syscall 0x00007ffff7b13c80 <pause+16>: cmp $0xfffffffffffff001,%rax 0x00007ffff7b13c86 <pause+22>: jae 0x7ffff7b13cbd <pause+77> 0x00007ffff7b13c88 <pause+24>: retq 0x00007ffff7b13c89 <pause+25>: sub $0x18,%rsp 0x00007ffff7b13c8d <pause+29>: callq 0x7ffff7b60770 0x00007ffff7b13c92 <pause+34>: mov %rax,(%rsp) 0x00007ffff7b13c96 <pause+38>: mov $0x22,%eax 0x00007ffff7b13c9b <pause+43>: syscall 0x00007ffff7b13c9d <pause+45>: mov (%rsp),%rdi 0x00007ffff7b13ca1 <pause+49>: mov %rax,0x8(%rsp) 0x00007ffff7b13ca6 <pause+54>: callq 0x7ffff7b60740 0x00007ffff7b13cab <pause+59>: mov 0x8(%rsp),%rax 0x00007ffff7b13cb0 <pause+64>: add $0x18,%rsp 0x00007ffff7b13cb4 <pause+68>: cmp $0xfffffffffffff001,%rax 0x00007ffff7b13cba <pause+74>: jae 0x7ffff7b13cbd <pause+77> 0x00007ffff7b13cbc <pause+76>: retq 0x00007ffff7b13cbd <pause+77>: mov 0x2c42cc(%rip),%rcx # 0x7ffff7dd7f90 0x00007ffff7b13cc4 <pause+84>: xor %edx,%edx 0x00007ffff7b13cc6 <pause+86>: sub %rax,%rdx 0x00007ffff7b13cc9 <pause+89>: mov %edx,%fs:(%rcx) 0x00007ffff7b13ccc <pause+92>: or $0xffffffffffffffff,%rax 0x00007ffff7b13cd0 <pause+96>: jmp 0x7ffff7b13cbc <pause+76> End of assembler dump. (gdb) info reg rax 0xfffffffffffffffc -4 rbx 0x4007c0 4196288 rcx 0xffffffffffffffa8 -88 rdx 0x4 4 rsi 0x0 0 rdi 0x1 1 rbp 0x7fffffffe2e0 0x7fffffffe2e0 rsp 0x7fffffffe2b8 0x7fffffffe2b8 r8 0x7fffffffe210 140737488347664 r9 0x7fffffffe170 140737488347504 r10 0x7fffffffe040 140737488347200 r11 0x346 838 r12 0x400640 4195904 r13 0x7fffffffe3b0 140737488348080 r14 0x0 0 r15 0x0 0 rip 0x7ffff7b13cc9 0x7ffff7b13cc9 <pause+89> eflags 0x313 [ CF AF TF IF ] cs 0x33 51 ss 0x2b 43 ds 0x0 0 es 0x0 0 fs 0x0 0 gs 0x0 0 fctrl 0x37f 895 fstat 0x0 0 ftag 0xffff 65535 fiseg 0x0 0 fioff 0x0 0 foseg 0x0 0 fooff 0x0 0 fop 0x0 0 mxcsr 0x1f80 [ IM DM ZM OM UM PM ] (gdb) record stop Delete recorded log and stop recording?(y or n) y Process record: record_close (gdb) set disassemble-next-line on (gdb) si 0x00007ffff7b13ccc in pause () from /lib/libc.so.6 0x00007ffff7b13ccc <pause+92>: 48 83 c8 ff or $0xffffffffffffffff,%rax (gdb) info registers rax 0xfffffffffffffffc -4 rbx 0x4007c0 4196288 rcx 0xffffffffffffffa8 -88 rdx 0x4 4 rsi 0x0 0 rdi 0x1 1 rbp 0x7fffffffe2e0 0x7fffffffe2e0 rsp 0x7fffffffe2b8 0x7fffffffe2b8 r8 0x7fffffffe210 140737488347664 r9 0x7fffffffe170 140737488347504 r10 0x7fffffffe040 140737488347200 r11 0x346 838 r12 0x400640 4195904 r13 0x7fffffffe3b0 140737488348080 r14 0x0 0 r15 0x0 0 rip 0x7ffff7b13ccc 0x7ffff7b13ccc <pause+92> eflags 0x313 [ CF AF TF IF ] cs 0x33 51 ss 0x2b 43 ds 0x0 0 es 0x0 0 fs 0x0 0 gs 0x0 0 fctrl 0x37f 895 fstat 0x0 0 ftag 0xffff 65535 fiseg 0x0 0 fioff 0x0 0 foseg 0x0 0 fooff 0x0 0 fop 0x0 0 mxcsr 0x1f80 [ IM DM ZM OM UM PM ] (gdb) x 0x7ffff7dd7f90 0x7ffff7dd7f90: 0xffffffa8 (gdb) x 0xffffffa8 0xffffffa8: Cannot access memory at address 0xffffffa8 (gdb) x 0xffffffffffffffa8 0xffffffffffffffa8: Cannot access memory at address 0xffffffffffffffa8 (gdb) The fs is same with gs, but "mov %edx,%fs:(%rcx)" is not same with "mov %edx,(%rcx)". I think remove this patch from gdb-cvs-head before 7.0 branch and make the segment reg clear is better. What do you think about it? Thanks, Hui On Sat, Sep 5, 2009 at 23:37, Hui Zhu<teawater@gmail.com> wrote: > On Sat, Sep 5, 2009 at 16:14, Mark Kettenis<mark.kettenis@xs4all.nl> wrote: >>> 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? > > Thanks for you help, Michael. > >> >> The code is basically ok, but I'd like to ask Hui to avoid using >> meaningless variable names like "tmp". > > Thanks for remind me, Mark. > > I checked in this patch with change the "tmp" to "orv". > > > Hui > > >> >>> > 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 >>> > { >>> >>> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-09-06 6:52 ` Hui Zhu @ 2009-09-06 15:06 ` Hui Zhu 2009-09-07 0:07 ` Michael Snyder 0 siblings, 1 reply; 10+ messages in thread From: Hui Zhu @ 2009-09-06 15:06 UTC (permalink / raw) To: Mark Kettenis, Michael Snyder; +Cc: gdb-patches On Sun, Sep 6, 2009 at 14:52, Hui Zhu<teawater@gmail.com> wrote: > Hi guys, > > Sorry I didn't do more test for this patch on amd64 before I check it in. > > But this patch really work not very good in amd64. > > For example: > > Process record: i386_process_record addr = 0x7ffff7b13cc9 signal = 0 > Process record: add mem addr = 0xffffffffffffffa8 len = 4 to record list. > Process record: error reading memory at addr = 0xffffffffffffffa8 len = 4. > Process record: failed to record execution log. > > Program received signal SIGTRAP, Trace/breakpoint trap. > 0x00007ffff7b13cc9 in pause () from /lib/libc.so.6 > (gdb) disassemble > Dump of assembler code for function pause: > 0x00007ffff7b13c70 <pause+0>: cmpl $0x0,0x2c93d1(%rip) # 0x7ffff7ddd048 > 0x00007ffff7b13c77 <pause+7>: jne 0x7ffff7b13c89 <pause+25> > 0x00007ffff7b13c79 <pause+9>: mov $0x22,%eax > 0x00007ffff7b13c7e <pause+14>: syscall > 0x00007ffff7b13c80 <pause+16>: cmp $0xfffffffffffff001,%rax > 0x00007ffff7b13c86 <pause+22>: jae 0x7ffff7b13cbd <pause+77> > 0x00007ffff7b13c88 <pause+24>: retq > 0x00007ffff7b13c89 <pause+25>: sub $0x18,%rsp > 0x00007ffff7b13c8d <pause+29>: callq 0x7ffff7b60770 > 0x00007ffff7b13c92 <pause+34>: mov %rax,(%rsp) > 0x00007ffff7b13c96 <pause+38>: mov $0x22,%eax > 0x00007ffff7b13c9b <pause+43>: syscall > 0x00007ffff7b13c9d <pause+45>: mov (%rsp),%rdi > 0x00007ffff7b13ca1 <pause+49>: mov %rax,0x8(%rsp) > 0x00007ffff7b13ca6 <pause+54>: callq 0x7ffff7b60740 > 0x00007ffff7b13cab <pause+59>: mov 0x8(%rsp),%rax > 0x00007ffff7b13cb0 <pause+64>: add $0x18,%rsp > 0x00007ffff7b13cb4 <pause+68>: cmp $0xfffffffffffff001,%rax > 0x00007ffff7b13cba <pause+74>: jae 0x7ffff7b13cbd <pause+77> > 0x00007ffff7b13cbc <pause+76>: retq > 0x00007ffff7b13cbd <pause+77>: mov 0x2c42cc(%rip),%rcx # > 0x7ffff7dd7f90 > 0x00007ffff7b13cc4 <pause+84>: xor %edx,%edx > 0x00007ffff7b13cc6 <pause+86>: sub %rax,%rdx > 0x00007ffff7b13cc9 <pause+89>: mov %edx,%fs:(%rcx) > 0x00007ffff7b13ccc <pause+92>: or $0xffffffffffffffff,%rax > 0x00007ffff7b13cd0 <pause+96>: jmp 0x7ffff7b13cbc <pause+76> > End of assembler dump. > (gdb) info reg > rax 0xfffffffffffffffc -4 > rbx 0x4007c0 4196288 > rcx 0xffffffffffffffa8 -88 > rdx 0x4 4 > rsi 0x0 0 > rdi 0x1 1 > rbp 0x7fffffffe2e0 0x7fffffffe2e0 > rsp 0x7fffffffe2b8 0x7fffffffe2b8 > r8 0x7fffffffe210 140737488347664 > r9 0x7fffffffe170 140737488347504 > r10 0x7fffffffe040 140737488347200 > r11 0x346 838 > r12 0x400640 4195904 > r13 0x7fffffffe3b0 140737488348080 > r14 0x0 0 > r15 0x0 0 > rip 0x7ffff7b13cc9 0x7ffff7b13cc9 <pause+89> > eflags 0x313 [ CF AF TF IF ] > cs 0x33 51 > ss 0x2b 43 > ds 0x0 0 > es 0x0 0 > fs 0x0 0 > gs 0x0 0 > fctrl 0x37f 895 > fstat 0x0 0 > ftag 0xffff 65535 > fiseg 0x0 0 > fioff 0x0 0 > foseg 0x0 0 > fooff 0x0 0 > fop 0x0 0 > mxcsr 0x1f80 [ IM DM ZM OM UM PM ] > (gdb) record stop > Delete recorded log and stop recording?(y or n) y > Process record: record_close > (gdb) set disassemble-next-line on > (gdb) si > 0x00007ffff7b13ccc in pause () from /lib/libc.so.6 > 0x00007ffff7b13ccc <pause+92>: 48 83 c8 ff or $0xffffffffffffffff,%rax > (gdb) info registers > rax 0xfffffffffffffffc -4 > rbx 0x4007c0 4196288 > rcx 0xffffffffffffffa8 -88 > rdx 0x4 4 > rsi 0x0 0 > rdi 0x1 1 > rbp 0x7fffffffe2e0 0x7fffffffe2e0 > rsp 0x7fffffffe2b8 0x7fffffffe2b8 > r8 0x7fffffffe210 140737488347664 > r9 0x7fffffffe170 140737488347504 > r10 0x7fffffffe040 140737488347200 > r11 0x346 838 > r12 0x400640 4195904 > r13 0x7fffffffe3b0 140737488348080 > r14 0x0 0 > r15 0x0 0 > rip 0x7ffff7b13ccc 0x7ffff7b13ccc <pause+92> > eflags 0x313 [ CF AF TF IF ] > cs 0x33 51 > ss 0x2b 43 > ds 0x0 0 > es 0x0 0 > fs 0x0 0 > gs 0x0 0 > fctrl 0x37f 895 > fstat 0x0 0 > ftag 0xffff 65535 > fiseg 0x0 0 > fioff 0x0 0 > foseg 0x0 0 > fooff 0x0 0 > fop 0x0 0 > mxcsr 0x1f80 [ IM DM ZM OM UM PM ] > (gdb) x 0x7ffff7dd7f90 > 0x7ffff7dd7f90: 0xffffffa8 > (gdb) x 0xffffffa8 > 0xffffffa8: Cannot access memory at address 0xffffffa8 > (gdb) x 0xffffffffffffffa8 > 0xffffffffffffffa8: Cannot access memory at address 0xffffffffffffffa8 > (gdb) > > > The fs is same with gs, but "mov %edx,%fs:(%rcx)" is not same with > "mov %edx,(%rcx)". > > I think remove this patch from gdb-cvs-head before 7.0 branch and > make the segment reg clear is better. > > What do you think about it? > > Thanks, > Hui > I make a patch for it. Please help me review it. Thanks, Hui 2009-09-06 Hui Zhu <teawater@gmail.com> * i386-tdep.c (i386_record_check_override): Deleted. (i386_record_lea_modrm): Ditto. (i386_process_record): Ditto. --- i386-tdep.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-) --- a/i386-tdep.c +++ b/i386-tdep.c @@ -3148,26 +3148,6 @@ 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 orv, ds; - - regcache_raw_read_unsigned (irp->regcache, - irp->regmap[irp->override], - &orv); - regcache_raw_read_unsigned (irp->regcache, - irp->regmap[X86_RECORD_DS_REGNUM], - &ds); - if (orv != 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. */ @@ -3178,7 +3158,7 @@ i386_record_lea_modrm (struct i386_recor struct gdbarch *gdbarch = irp->gdbarch; uint64_t addr; - if (i386_record_check_override (irp)) + if (irp->override >= 0) { warning (_("Process record ignores the memory change " "of instruction at address %s because it " @@ -4060,7 +4040,7 @@ reswitch: /* mov EAX */ case 0xa2: case 0xa3: - if (i386_record_check_override (&ir)) + if (ir.override >= 0) { warning (_("Process record ignores the memory change " "of instruction at address 0x%s because " @@ -4478,8 +4458,13 @@ reswitch: ir.regmap[X86_RECORD_REDI_REGNUM], &tmpulongest); - ir.override = X86_RECORD_ES_REGNUM; - if (ir.aflag && i386_record_check_override (&ir)) + 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)) { /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */ warning (_("Process record ignores the memory " @@ -5103,7 +5088,7 @@ reswitch: opcode = opcode << 8 | ir.modrm; goto no_support; } - if (i386_record_check_override (&ir)) + if (ir.override >= 0) { warning (_("Process record ignores the memory " "change of instruction at " @@ -5154,7 +5139,7 @@ reswitch: else { /* sidt */ - if (i386_record_check_override (&ir)) + if (ir.override >= 0) { warning (_("Process record ignores the memory " "change of instruction at " ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-09-06 15:06 ` Hui Zhu @ 2009-09-07 0:07 ` Michael Snyder 2009-09-07 11:17 ` Hui Zhu 0 siblings, 1 reply; 10+ messages in thread From: Michael Snyder @ 2009-09-07 0:07 UTC (permalink / raw) To: Hui Zhu; +Cc: Mark Kettenis, gdb-patches Hui Zhu wrote: > On Sun, Sep 6, 2009 at 14:52, Hui Zhu<teawater@gmail.com> wrote: >> Hi guys, >> >> Sorry I didn't do more test for this patch on amd64 before I check it in. >> >> But this patch really work not very good in amd64. >> [...] >> I think remove this patch from gdb-cvs-head before 7.0 branch and >> make the segment reg clear is better. >> >> What do you think about it? >> >> Thanks, >> Hui >> > > I make a patch for it. Please help me review it. > > Thanks, > Hui > > 2009-09-06 Hui Zhu <teawater@gmail.com> > > * i386-tdep.c (i386_record_check_override): Deleted. > (i386_record_lea_modrm): Ditto. > (i386_process_record): Ditto. (i386_process_record): Revert the use of deleted function 'i386_record_check_override'. (i386_record_lea_modrm): Ditto. Looks to me like you've preserved what you could of the change, and removed the minimum. Good. This looks ok to commit. > > --- > i386-tdep.c | 37 +++++++++++-------------------------- > 1 file changed, 11 insertions(+), 26 deletions(-) > > --- a/i386-tdep.c > +++ b/i386-tdep.c > @@ -3148,26 +3148,6 @@ 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 orv, ds; > - > - regcache_raw_read_unsigned (irp->regcache, > - irp->regmap[irp->override], > - &orv); > - regcache_raw_read_unsigned (irp->regcache, > - irp->regmap[X86_RECORD_DS_REGNUM], > - &ds); > - if (orv != 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. */ > @@ -3178,7 +3158,7 @@ i386_record_lea_modrm (struct i386_recor > struct gdbarch *gdbarch = irp->gdbarch; > uint64_t addr; > > - if (i386_record_check_override (irp)) > + if (irp->override >= 0) > { > warning (_("Process record ignores the memory change " > "of instruction at address %s because it " > @@ -4060,7 +4040,7 @@ reswitch: > /* mov EAX */ > case 0xa2: > case 0xa3: > - if (i386_record_check_override (&ir)) > + if (ir.override >= 0) > { > warning (_("Process record ignores the memory change " > "of instruction at address 0x%s because " > @@ -4478,8 +4458,13 @@ reswitch: > ir.regmap[X86_RECORD_REDI_REGNUM], > &tmpulongest); > > - ir.override = X86_RECORD_ES_REGNUM; > - if (ir.aflag && i386_record_check_override (&ir)) > + 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)) > { > /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; */ > warning (_("Process record ignores the memory " > @@ -5103,7 +5088,7 @@ reswitch: > opcode = opcode << 8 | ir.modrm; > goto no_support; > } > - if (i386_record_check_override (&ir)) > + if (ir.override >= 0) > { > warning (_("Process record ignores the memory " > "change of instruction at " > @@ -5154,7 +5139,7 @@ reswitch: > else > { > /* sidt */ > - if (i386_record_check_override (&ir)) > + if (ir.override >= 0) > { > warning (_("Process record ignores the memory " > "change of instruction at " > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFA/prec] Make i386 handle segment register better 2009-09-07 0:07 ` Michael Snyder @ 2009-09-07 11:17 ` Hui Zhu 0 siblings, 0 replies; 10+ messages in thread From: Hui Zhu @ 2009-09-07 11:17 UTC (permalink / raw) To: Michael Snyder; +Cc: Mark Kettenis, gdb-patches On Mon, Sep 7, 2009 at 08:06, Michael Snyder<msnyder@vmware.com> wrote: > Hui Zhu wrote: >> >> On Sun, Sep 6, 2009 at 14:52, Hui Zhu<teawater@gmail.com> wrote: >>> >>> Hi guys, >>> >>> Sorry I didn't do more test for this patch on amd64 before I check it in. >>> >>> But this patch really work not very good in amd64. >>> > [...] >>> >>> I think remove this patch from gdb-cvs-head before 7.0 branch and >>> make the segment reg clear is better. >>> >>> What do you think about it? >>> >>> Thanks, >>> Hui >>> >> >> I make a patch for it. Please help me review it. >> >> Thanks, >> Hui >> >> 2009-09-06 Hui Zhu <teawater@gmail.com> >> >> * i386-tdep.c (i386_record_check_override): Deleted. >> (i386_record_lea_modrm): Ditto. >> (i386_process_record): Ditto. > > (i386_process_record): Revert the use of deleted > function 'i386_record_check_override'. > (i386_record_lea_modrm): Ditto. > > Looks to me like you've preserved what you could > of the change, and removed the minimum. Good. > > This looks ok to commit. > > Thanks. Checked in. Hui > >> >> --- >> i386-tdep.c | 37 +++++++++++-------------------------- >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> --- a/i386-tdep.c >> +++ b/i386-tdep.c >> @@ -3148,26 +3148,6 @@ 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 orv, ds; >> - >> - regcache_raw_read_unsigned (irp->regcache, >> - irp->regmap[irp->override], >> - &orv); >> - regcache_raw_read_unsigned (irp->regcache, >> - irp->regmap[X86_RECORD_DS_REGNUM], >> - &ds); >> - if (orv != 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. */ >> @@ -3178,7 +3158,7 @@ i386_record_lea_modrm (struct i386_recor >> struct gdbarch *gdbarch = irp->gdbarch; >> uint64_t addr; >> >> - if (i386_record_check_override (irp)) >> + if (irp->override >= 0) >> { >> warning (_("Process record ignores the memory change " >> "of instruction at address %s because it " >> @@ -4060,7 +4040,7 @@ reswitch: >> /* mov EAX */ >> case 0xa2: >> case 0xa3: >> - if (i386_record_check_override (&ir)) >> + if (ir.override >= 0) >> { >> warning (_("Process record ignores the memory change " >> "of instruction at address 0x%s because " >> @@ -4478,8 +4458,13 @@ reswitch: >> ir.regmap[X86_RECORD_REDI_REGNUM], >> &tmpulongest); >> >> - ir.override = X86_RECORD_ES_REGNUM; >> - if (ir.aflag && i386_record_check_override (&ir)) >> + 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)) >> { >> /* addr += ((uint32_t) read_register (I386_ES_REGNUM)) << 4; >> */ >> warning (_("Process record ignores the memory " >> @@ -5103,7 +5088,7 @@ reswitch: >> opcode = opcode << 8 | ir.modrm; >> goto no_support; >> } >> - if (i386_record_check_override (&ir)) >> + if (ir.override >= 0) >> { >> warning (_("Process record ignores the memory " >> "change of instruction at " >> @@ -5154,7 +5139,7 @@ reswitch: >> else >> { >> /* sidt */ >> - if (i386_record_check_override (&ir)) >> + if (ir.override >= 0) >> { >> warning (_("Process record ignores the memory " >> "change of instruction at " >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-09-07 11:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-08-29 16:12 [RFA/prec] Make i386 handle segment register better 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 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox