On Sun, Aug 30, 2009 at 05:21, Michael Snyder 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   >> >>        * 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 * 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 {