Re: [RFA/prec] Make i386 handle segment register better

[Michael Snyder <msnyder@vmware.com>]

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