Re: [PATCH] Rewrite the codes for opcode 0x0f01 and add more instructions support

Michael Snyder
2009-10-12T02:08:56Z

Jiang Jilin wrote:
> Hi, guys
>
> I've rewrite the codes for opcode 0x0f01 with more readable, add
> xgetbv/xsetbv/rdtscp/vmcall/vmlaunch/vmresume/vmxoff instructions
> support as well.
>
> However, I'm *not* sure it's whether right or not, especially with
> the new supported instructions beginning with "vm". And I remove all
> codes to save EFLAGS register which is not specified to be saved by
> Intel's manual, so please help me review them.
>
> Luckily, there is no regression when using precord.exp board file to test.
>
> At last but not least, there is some differences in gdb.sum when
> 'make check' before and after applying this patch. I cannot make
> a decision whether it's correct, so please help me. The diff are
> as follows:

Ah well, but you see, now the change is too big to be accepted
without a copyright assignment. Do you want to start the process
of filing one?

[Cc: Tom Tromey]

>
> --- testsuite/gdb.sum.before 2009-10-10 20:02:35.000000000 +0800
> +++ testsuite/gdb.sum.after 2009-10-10 20:21:35.000000000 +0800
> @@ -1,4 +1,4 @@
> -Test Run By jiang on Sat Oct 10 19:44:30 2009
> +Test Run By jiang on Sat Oct 10 20:05:20 2009
> Native configuration is i686-pc-linux-gnu
>
> === gdb tests ===
> @@ -14113,18 +14113,18 @@
> PASS: gdb.threads/watchthreads2.exp: all threads started
> PASS: gdb.threads/watchthreads2.exp: watch x
> PASS: gdb.threads/watchthreads2.exp: set var test_ready = 1
> -KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in multithreaded app (PRMS: gdb/10116)
> +PASS: gdb.threads/watchthreads2.exp: all threads incremented x
> Running ./gdb.threads/watchthreads.exp ...
> PASS: gdb.threads/watchthreads.exp: successfully compiled posix threads test case
> PASS: gdb.threads/watchthreads.exp: watch args[0]
> PASS: gdb.threads/watchthreads.exp: watch args[1]
> PASS: gdb.threads/watchthreads.exp: disable 2
> -FAIL: gdb.threads/watchthreads.exp: threaded watch loop
> +PASS: gdb.threads/watchthreads.exp: threaded watch loop
> PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
> PASS: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit
> PASS: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread
> PASS: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread
> -FAIL: gdb.threads/watchthreads.exp: combination of threaded watchpoints = 30
> +PASS: gdb.threads/watchthreads.exp: combination of threaded watchpoints = 30
> Running ./gdb.trace/actions.exp ...
> PASS: gdb.trace/actions.exp: 5.1a: set three tracepoints, no actions
> PASS: gdb.trace/actions.exp: 5.1b: set actions for first tracepoint
> @@ -14288,8 +14288,8 @@
>
> === gdb Summary ===
>
> -# of expected passes 13512
> -# of unexpected failures 76
> +# of expected passes 13515
> +# of unexpected failures 74
>
> 2009-10-10 Jiang Jilin <freephp@gmail.com>
> * i386-tdep.c (i386_process_record): Rewrite the codes for
> opcode 0x0f01 and add more instructions support
> ---
> gdb/i386-tdep.c | 234 ++++++++++++++++++++++++------------------------------
> 1 files changed, 104 insertions(+), 130 deletions(-)
>
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index b79bcd2..2300a91 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -5075,152 +5075,126 @@ reswitch:
> case 0x0f01:
> if (i386_record_modrm (&ir))
> return -1;
> - switch (ir.reg)
> + if (ir.mod == 3)
> {
> - /* sgdt */
> - case 0:
> - {
> - uint64_t tmpu64;
> -
> - if (ir.mod == 3)
> - {
> - ir.addr -= 3;
> - opcode = opcode << 8 | ir.modrm;
> - goto no_support;
> - }
> - if (ir.override >= 0)
> - {
> - 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
> - {
> - if (i386_record_lea_modrm_addr (&ir, &tmpu64))
> - return -1;
> - if (record_arch_list_add_mem (tmpu64, 2))
> - return -1;
> - tmpu64 += 2;
> - if (ir.regmap[X86_RECORD_R8_REGNUM])
> - {
> - if (record_arch_list_add_mem (tmpu64, 8))
> - return -1;
> - }
> - else
> - {
> - if (record_arch_list_add_mem (tmpu64, 4))
> - return -1;
> - }
> - }
> - }
> - break;
> - case 1:
> - if (ir.mod == 3)
> + uint8_t reg_rm = (ir.reg << 4) | ir.rm;
> +
> + switch (reg_rm)
> {
> - switch (ir.rm)
> - {
> - /* monitor */
> - case 0:
> - break;
> - /* mwait */
> - case 1:
> - I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> - break;
> - default:
> - ir.addr -= 3;
> - opcode = opcode << 8 | ir.modrm;
> - goto no_support;
> - break;
> + /* vmcall */
> + case 0x01:
> + /* vmlaunch */
> + case 0x02:
> + /* vmresume */
> + case 0x03:
> + /* vmxoff */
> + case 0x04:
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> + break;
> + /* monitor */
> + case 0x10:
> + break;
> + /* mwait */
> + case 0x11:
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> + break;
> + /* xgetbv */
> + case 0x20:
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REAX_REGNUM);
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDX_REGNUM);
> + break;
> + /* xsetbv */
> + case 0x21:
> + break;
> + /* swapgs */
> + case 0x70:
> + if (ir.regmap[X86_RECORD_R8_REGNUM])
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_GS_REGNUM);
> + else
> + {
> + ir.addr -= 3;
> + opcode = opcode << 8 | ir.modrm;
> + goto no_support;
> + }
> + break;
> + /* rdtscp */
> + case 0x71:
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REAX_REGNUM);
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_RECX_REGNUM);
> + I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_REDX_REGNUM);
> + break;
> + default:
> + /* smsw */
> + if (ir.reg == 4)
> + {
> + I386_RECORD_ARCH_LIST_ADD_REG (ir.rm | ir.rex_b);
> + break;
> }
> + /* lmsw */
> + else if (ir.reg == 6)
> + break;
> + ir.addr -= 3;
> + opcode = opcode << 8 | ir.modrm;
> + goto no_support;
> }
> - else
> + }
> + else
> + {
> + switch (ir.reg)
> {
> - /* sidt */
> + /* sgdt */
> + case 0:
> + /* sidt */
> + case 1:
> if (ir.override >= 0)
> - {
> + {
> 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));
> + "change of instruction at "
> + "address %s because it can't get "
> + "the value of the segment "
> + "register."),
> + paddress (gdbarch, ir.orig_addr));
> }
> else
> - {
> - uint64_t tmpu64;
> + {
> + uint64_t tmpu64;
>
> - if (i386_record_lea_modrm_addr (&ir, &tmpu64))
> + /* We have to store at least (4 + 2 = 6) bytes,
> + or (8 + 2 = 10) bytes at most. */
> + if (i386_record_lea_modrm_addr (&ir, &tmpu64))
> + return -1;
> + if (record_arch_list_add_mem (tmpu64, 6))
> return -1;
> - if (record_arch_list_add_mem (tmpu64, 2))
> - return -1;
> - addr += 2;
> - if (ir.regmap[X86_RECORD_R8_REGNUM])
> - {
> - if (record_arch_list_add_mem (tmpu64, 8))
> - return -1;
> - }
> - else
> - {
> - if (record_arch_list_add_mem (tmpu64, 4))
> - return -1;
> - }
> + tmpu64 += 6;
> + if (ir.regmap[X86_RECORD_R8_REGNUM])
> + {
> + if (record_arch_list_add_mem (tmpu64, 4))
> + return -1;
> + }
> }
> - }
> - break;
> - /* lgdt */
> - case 2:
> - /* lidt */
> - case 3:
> - if (ir.mod == 3)
> - {
> - ir.addr -= 3;
> - opcode = opcode << 8 | ir.modrm;
> - goto no_support;
> - }
> - break;
> - /* smsw */
> - case 4:
> - if (ir.mod == 3)
> - {
> - if (record_arch_list_add_reg (ir.regcache, ir.rm | ir.rex_b))
> - return -1;
> - }
> - else
> - {
> + break;
> + /* lgdt */
> + case 2:
> + /* lidt */
> + case 3:
> + break;
> + /* smsw */
> + case 4:
> ir.ot = OT_WORD;
> if (i386_record_lea_modrm (&ir))
> return -1;
> + break;
> + /* lmsw */
> + case 6:
> + break;
> + /* invlpg */
> + case 7:
> + break;
> + default:
> + ir.addr -= 3;
> + opcode = opcode << 8 | ir.modrm;
> + goto no_support;
> }
> - I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> - break;
> - /* lmsw */
> - case 6:
> - I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> - break;
> - /* invlpg */
> - case 7:
> - if (ir.mod == 3)
> - {
> - if (ir.rm == 0 && ir.regmap[X86_RECORD_R8_REGNUM])
> - I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_GS_REGNUM);
> - else
> - {
> - ir.addr -= 3;
> - opcode = opcode << 8 | ir.modrm;
> - goto no_support;
> - }
> - }
> - else
> - I386_RECORD_ARCH_LIST_ADD_REG (X86_RECORD_EFLAGS_REGNUM);
> - break;
> - default:
> - ir.addr -= 3;
> - opcode = opcode << 8 | ir.modrm;
> - goto no_support;
> - break;
> }
> break;
>