Re: [RFA/RFC Prec] Add Linux AMD64 process record support second version, (instruction set support) 1/3

[Michael Snyder <msnyder@vmware.com>]

> Thanks Michael,
> 
> I updated all the AMD64 patches.  Please help me review it.
> 
> Thanks,
> Hui
> 
> 2009-07-17  Hui Zhu  <teawater@gmail.com>
> 
>         Add AMD64 process record instruction set support.
> 
>         * i386-tdep.h (gdbarch_tdep): Add record_regmap for registers
>         because the AMD64's registers order in GDB is not same with
>         I386 instructions.
>         Add i386_syscall_record to be the syscall function handle
>         interface.
>         (record_i386_regnum): Number for record_regmap.
>         * i386-tdep.c (OT_QUAD): For 64 bits.
>         (i386_record_s): Add rex_x, rex_b, rip_offset and
>         popl_esp_hack for AMD64 instruction set. And regmap for
>         record_regmap.
>         (i386_record_lea_modrm_addr): Support AMD64 instruction set
>         64 bits lea.
>         (i386_record_lea_modrm): Ditto.
>         (i386_record_push): New function.  Record the execution log
>         of push.
>         (I386_RECORD_ARCH_LIST_ADD_REG): New macro to record the
>         register.
>         (i386_process_record): Support AMD64 instruction set.
>         amd64-tdep.c (amd64_record_regmap): For record_regmap.

Should be a "*" at the beginning of the line above.


> @@ -2864,6 +2865,7 @@ enum
> 
>  struct i386_record_s
>  {
> +  struct gdbarch *gdbarch;
>    struct regcache *regcache;
>    CORE_ADDR addr;
>    int aflag;
> @@ -2872,6 +2874,11 @@ struct i386_record_s
>    uint8_t modrm;
>    uint8_t mod, reg, rm;
>    int ot;
> +  uint8_t rex_x;
> +  uint8_t rex_b;
> +  int rip_offset;
> +  int popl_esp_hack;

popl_esp_hack is used but never initialized.
Do you still need it?  Is it for some future use?


> @@ -4611,14 +4633,13 @@ reswitch:
>      case 0x0f9d:
>      case 0x0f9e:
>      case 0x0f9f:
> +      I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
>        ir.ot = OT_BYTE;
>        if (i386_record_modrm (&ir))
>         return -1;
>        if (ir.mod == 3)
> -       {
> -         if (record_arch_list_add_reg (ir.regcache, ir.rm & 0x3))
> -           return -1;
> -       }
> +        I386_RECORD_ARCH_LIST_ADD_REG (ir.rex_b ? (ir.rm | ir.rex_b):
> +                                                  (ir.rm & 0x3));

Space before colon operator (':'), and then
the 2nd line isn't indented right.  I know it
looks prettier the way it is, but it should be
over to the left.   ;-)


> @@ -5115,21 +5225,21 @@ reswitch:
> 
>        /* arpl */
>      case 0x63:
> -      ir.ot = ir.dflag ? OT_LONG : OT_WORD;
>        if (i386_record_modrm (&ir))
>         return -1;
> -      if (ir.mod != 3)
> -       {
> -         if (i386_record_lea_modrm (&ir))
> -           return -1;
> -       }
> +      if (ir.mod == 3 || ir.regmap[X86_RECORD_R8_REGNUM])
> +        {
> +          I386_RECORD_ARCH_LIST_ADD_REG (ir.regmap[X86_RECORD_R8_REGNUM]?
> +                                           (ir.reg | rex_r) : ir.rm);

Space before question-mark, and then the indentation of the second line.


> @@ -5232,9 +5341,8 @@ reswitch:
>        break;
>      }
> 
> -/* In the future, Maybe still need to deal with need_dasm */
> -  if (record_arch_list_add_reg (ir.regcache, I386_EIP_REGNUM))
> -    return -1;
> +  /* In the future, Maybe still need to deal with need_dasm */

"maybe" lower case.  Period and two spaces at end of sentence.


Everything else in this patch looks great!

Mark Kettenis should approve it, though.

Michael