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

Michael Snyder
2009-07-17T23:58:45Z

> 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