This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On Tue, Jan 27, 2009 at 9:15 AM, Pedro Alves <pedro@codesourcery.com> wrote: > On Monday 26 January 2009 23:00:12, Doug Evans wrote: >> Hi. I took a crack at implementing displaced stepping support for amd64. > > Great! Thank you so much for this. > >> I think the GNU tools need a general-purpose library of ISA-related tools. > > Right, opcodes <-> gdb could cooperate better, in several ways. > >> Until then, I went with the disassembler. The code is laid out such that >> when a better implementation of computing insn lengths comes along, it >> can be easily dropped in. > > Err, well, did you consider adding the needed interfaces to libopcodes? > It's what we use to implement the disassembler anyway... I'd like to add the needed interfaces. I'm just not sure how long a process that will be and I'd like to get started on exercising amd64 non-stop functionality. I'll pursue exporting the modrm_bytes arrays and insn-length computation with binutils. > >> (rex_prefix_p,amd64_insn_length_fprintf,amd64_insn_length_init_dis, >> amd64_insn_length,amd64_get_unused_input_int_reg,fixup_riprel, > > On the nitpicking department, the standard tells us to split these lines > as: > > (rex_prefix_p,amd64_insn_length_fprintf,amd64_insn_length_init_dis) > (amd64_insn_length,amd64_get_unused_input_int_reg,fixup_riprel) > (foo): Base. > > Emacs loves this form better too. > >> amd64_displaced_step_fixup): New fns. > > Are you really that lazy? :-) Please spell out "fns", and avoid everyone > else the extra cycles to expand that. ok. > >> +/* ??? *_has_modrm are copies of the tables in ../opcodes/i386-dis.c. >> + It might be nice if we could use them. >> + ??? This differs from the kernel version, is one of them out of date? */ > > Yes, looks like it. It is safe to assume that the opcodes version is more > up to date, as it gets updated to reflect new cpus frequently, by the Intel > and AMD folks, at least. I assumed so, but didn't actually know. > In any case, comments like this are no good, as they're doomed to get out of > date at some point themselves too. Better say, "please keep this in > sync with ... foo". If not addressing this duplication yourself, please > do open a PR about it. This will surelly end up out of date at > some point... I've modified the comment to be a warning. >> +/* Return the length in bytes of INSN. >> + MAX_LEN is the size of the buffer containing INSN >> + ??? The GNU tools need a library of basic ISA-related support. >> + Until then we use the disassembler. */ > > ... Why not say something like "Unfortunately, libopcodes doesn't export > a simpler way to query the length of instruction at a given address. We > use the disassembler interface to do that." ... The attached patch has a different wording. >> +static int >> +amd64_insn_length (const gdb_byte *insn, int max_len, CORE_ADDR addr) >> +{ >> + struct disassemble_info di; >> + struct gdbarch *gdbarch = current_gdbarch; > > Ouch, CURRENT_GDBARCH red alert. From what I can tell, you have a gdbarch > available to use. amd64_displaced_step_copy_insn gets a gdbarch* passed in > as parameter, so you can pass it to fixup_displaced_copy, and then > to fixup_riprel, which then can pass it here. Can you do that please? righto. >> +/* Return an integer register (other than RSP) that is unused as an input >> + operand in INSN. >> + MAX_LEN is the size of the buffer containing INSN. >> + OPCODE_OFFSET is the offset of the first opcode byte. >> + OPCODE_LEN is the number of opcode bytes. >> + MODRM_OFFSET is the offset of the ModRM byte or -1 if not present. >> + In order to not require adding a rex prefix if the insn doesn't already >> + have one, the result is restricted to RAX ... RDI, sans RSP. >> + The register numbering of the result follows architecture ordering, >> + e.g. RDI = 7. >> + >> + ??? The GNU tools need a library of basic ISA-related support. */ > > >> + /* We shouldn't get here. */ >> + gdb_assert (0); > > Use internal_error instead, please. Ah, righto. >> +static void >> +fixup_riprel (struct displaced_step_closure *dsc, CORE_ADDR from, CORE_ADDR to, >> + struct regcache *regs) >> +{ >> + int modrm_offset = dsc->modrm_offset; >> + gdb_byte *insn = dsc->insn_buf + modrm_offset; >> + CORE_ADDR rip_base; >> + int32_t disp; >> + int insn_length; >> + int arch_tmp_regno, tmp_regno; >> + ULONGEST orig_value; >> + >> + /* %rip+disp32 addressing mode, displacement follows ModRM byte. */ >> + ++insn; >> + > >> + /* Compute the rip-relative address. */ >> + disp = *(int32_t*) insn; // FIXME: target-fetch-value > > Err, that will cause unaligned traps on some hosts (this is a tdep file, not > a nat file). Please fix that up. Ya, oops. That one I intended to fix but forgot about. Fixed. >> + insn_length = amd64_insn_length (dsc->insn_buf, dsc->max_len, from); >> + rip_base = from + insn_length; >> + >> + /* We need a register to hold the address. >> + Pick one not used in the insn. >> + NOTE: arch_tmp_regno uses architecture ordering, e.g. RDI = 7. */ >> + arch_tmp_regno = amd64_get_unused_input_int_reg (dsc->insn_buf, dsc->max_len, >> + dsc->opcode_offset, >> + dsc->opcode_len, >> + dsc->modrm_offset); >> + tmp_regno = amd64_arch_reg_to_regnum (arch_tmp_regno); >> + >> + /* REX.B should be unset as we were using rip-relative addressing, >> + but ensure it's unset anyway, tmp_regno is not r8-r15. */ >> + if (dsc->rex_offset != -1) >> + dsc->insn_buf[dsc->rex_offset] &= ~1; >> + >> + regcache_cooked_read_unsigned (regs, tmp_regno, &orig_value); >> + dsc->tmp_regno = tmp_regno; >> + dsc->tmp_save = orig_value; >> + dsc->tmp_used = 1; >> + >> + /* Convert the ModRM field to be base+disp. */ >> + dsc->insn_buf[modrm_offset] &= ~0xc7; >> + dsc->insn_buf[modrm_offset] |= 0x80 + arch_tmp_regno; >> + >> + regcache_cooked_write_unsigned (regs, tmp_regno, rip_base); >> + >> + if (debug_displaced) >> + fprintf_unfiltered (gdb_stdlog, "displaced: %%rip-relative addressing used.\n" >> + "displaced: using temp reg %d, old value 0x%s, new value 0x%s\n", >> + dsc->tmp_regno, paddr_nz (dsc->tmp_save), >> + paddr_nz (rip_base)); >> +} >> + >> +static void >> +fixup_displaced_copy (struct displaced_step_closure *dsc, >> + CORE_ADDR from, CORE_ADDR to, struct regcache *regs) >> +{ >> + gdb_byte *insn = dsc->insn_buf; >> + int len = dsc->max_len; >> + gdb_byte *start = insn; >> + gdb_byte *end = insn + len; >> + int need_modrm; >> + >> + /* Skip legacy instruction prefixes. >> + While the kernel's kprobes support can assume the instruction is valid, >> + we can't. Don't crash if we see an invalid insn. */ > > Yeah, we'd better warn/error out to the user than to do something silly > with instructions we don't know what to do with. Should we be checking > if you've reached >= end somewhere? The code allocates sufficient extra space to obviate having to continually test "are we at the end yet?". A lot of this is copied from i386-tdep.c. If this patch is ok, I'll send patches for i386-tdep.c. > >> + >> + while (insn < end) >> + { >> + switch (*insn) >> + { >> + case 0x66: >> + case 0x67: >> + case 0x2e: >> + case 0x3e: >> + case 0x26: >> + case 0x64: >> + case 0x65: >> + case 0x36: >> + case 0xf0: >> + case 0xf3: >> + case 0xf2: >> + ++insn; >> + continue; >> + } >> + break; >> + } >> + >> + /* Skip REX instruction prefix. */ >> + if (rex_prefix_p (*insn)) >> + { >> + dsc->rex_offset = insn - start; >> + ++insn; >> + } >> + >> + dsc->opcode_offset = insn - start; >> + >> + if (*insn == 0x0f) >> + { >> + /* Two or three-byte opcode. */ >> + ++insn; >> + need_modrm = twobyte_has_modrm[*insn]; >> + >> + /* Check for three-byte opcode. */ >> + if (*insn == 0x38 || *insn == 0x3a) >> + { >> + ++insn; >> + dsc->opcode_len = 3; >> + } >> + else >> + dsc->opcode_len = 2; >> + } >> + else >> + { >> + /* One-byte opcode. */ >> + need_modrm = onebyte_has_modrm[*insn]; >> + dsc->opcode_len = 1; >> + } >> + >> + if (need_modrm) >> + { >> + gdb_byte modrm = *++insn; >> + >> + dsc->modrm_offset = insn - start; >> + >> + if ((modrm & 0xc7) == 0x05) >> + { >> + /* The insn uses rip-relative addressing. >> + Deal with it. */ >> + fixup_riprel (dsc, from, to, regs); >> + } >> + } >> +} >> + > > >> + /* GDB may get control back after the insn after the syscall. >> + If this is a syscall, make sure there's a nop afterwards. */ >> + { >> + int syscall_length; >> >> + if (amd64_syscall_p (buf, &syscall_length)) >> + buf[syscall_length] = 0x90; >> + } > > Weird. Isn't this a kernel bug? It only happens when single-stepping > the syscall insn. E.g., from your testcase, without displaced stepping: > > 58 test_syscall: > 59 syscall > 60 nop > 61 test_syscall_end: > 62 nop > > (gdb) b amd64-disp-step.S:60 > Breakpoint 1 at 0x4004fa: file ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S, line 60. > (gdb) r > Starting program: /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.arch/amd64-disp-step > > Breakpoint 1, test_syscall () at ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S:60 > 60 nop > Current language: auto; currently asm > > But: > > (gdb) c > Continuing. > > Program exited normally. > (gdb) b 59 > Breakpoint 2 at 0x4004f8: file ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S, line 59. > (gdb) r > Starting program: /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.arch/amd64-disp-step > > Breakpoint 2, test_syscall () at ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S:59 > 59 syscall > (gdb) c > Continuing. > > Program exited normally. > (gdb) > > GDB did a single-step to get over the "syscall", and missed the breakpoint > at 0x4004fa. > > Again, doing a manual stepi: > > (gdb) r > Starting program: /home/pedro/gdb/baseline/build/gdb/testsuite/gdb.arch/amd64-disp-step > > Breakpoint 2, test_syscall () at ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S:59 > 59 syscall > (gdb) del 2 > (gdb) set debug infrun 1 > (gdb) stepi > infrun: clear_proceed_status_thread (process 4322) > infrun: proceed (addr=0xffffffffffffffff, signal=144, step=1) > During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x7ffff781b0f3. > infrun: resume (step=1, signal=0), trap_expected=0 > infrun: wait_for_inferior (treat_exec_as_sigtrap=0) > infrun: infwait_normal_state > infrun: TARGET_WAITKIND_STOPPED > infrun: stop_pc = 0x4004fb > infrun: stepi/nexti > infrun: stop_stepping > test_syscall_end () at ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S:62 > 62 nop > (gdb) stepi > test_syscall_end () at ../../../src/gdb/testsuite/gdb.arch/amd64-disp-step.S:62 > 62 nop > (gdb) p $pc > $1 = (void (*)()) 0x4004fb <test_syscall_end> > (gdb) > > If you replace the nop with something wider, e.g., try stepping over > this: > > mov $0x27,%eax /* getpid */ > syscall > mov $0xff,%eax > nop > > You'll see that the move after the syscall is completely > stepped over, and that is was executed correctly. Ya, this is what I discovered during my experiments to understand this. > Something in the kernel not restoring the trace flag correctly, and > hence stepping one instruction too much? Presumably. > >> +/* ??? Do we need to check for rex prefix in these predicates, or their >> + callers? */ > > The caller should (amd64_displaced_step_fixup), I believe? IIUC, you also > store the prefixes in dsc->insn_buf. Well, that comment refers to the insn predicates like amd64_call_p. Having the caller skip the prefixes muddies their interface. The appended patch does something cleaner. > >> + >> +static int >> +amd64_absolute_jmp_p (gdb_byte *insn) >> +{ >> + /* jmp far (absolute address in operand) */ >> + /* ??? undefined insn actually */ >> + if (insn[0] == 0xea) >> + return 1; > > Indeed, invalid in 64-bit mode. Any reason not to drop it? Dropped. > >> + >> + if (insn[0] == 0xff) >> + { >> + /* jump near, absolute indirect (/4) */ >> + if ((insn[1] & 0x38) == 0x20) >> + return 1; >> + >> + /* jump far, absolute indirect (/5) */ >> + if ((insn[1] & 0x38) == 0x28) >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> +static int >> +amd64_absolute_call_p (gdb_byte *insn) >> +{ >> + /* call far, absolute */ >> + /* ??? undefined insn actually */ >> + if (insn[0] == 0x9a) >> + return 1; >> + > > Same as above. Righto. > >> + if (insn[0] == 0xff) >> + { >> + /* Call near, absolute indirect (/2) */ >> + if ((insn[1] & 0x38) == 0x10) >> + return 1; >> + >> + /* Call far, absolute indirect (/3) */ >> + if ((insn[1] & 0x38) == 0x18) >> + return 1; >> + } >> + >> + return 0; >> +} >> + > >> Index: testsuite/gdb.arch/amd64-disp-step.S >> =================================================================== >> RCS file: testsuite/gdb.arch/amd64-disp-step.S >> diff -N testsuite/gdb.arch/amd64-disp-step.S > >> + Please email any bugs, comments, and/or additions to this file to: >> + bug-gdb@gnu.org > > Hmmm, shoudn't we be stopping using this address? cut-n-paste. Deleted. > >> +gdb_test "set non-stop on" "" >> +gdb_test "show non-stop" "Controlling the inferior in non-stop mode is on.*" > > Please use "set displaced-stepping on" instead. Righto. > >> +# Done, run program to exit. >> + >> +gdb_test "continue" \ >> + ".*Program exited normally.*" \ >> + "continue to exit" >> > > Use gdb_continue_to_end instead. Righto. Ok to check in? [Modulo I need to clear the opcode/i386.h additions with binutils. I'm not sure who to get approval from first.] 2009-01-27 Doug Evans <dje@google.com> * opcode/i386.h: Add multiple inclusion protection. (EAX_REG_NUM,ECX_REG_NUM,EDX_REGNUM,EBX_REG_NUM,ESI_REG_NUM) (EDI_REG_NUM): New macros. (MODRM_MOD_FIELD,MODRM_REG_FIELD,MODRM_RM_FIELD): New macros. (SIB_SCALE_FIELD,SIB_INDEX_FIELD,SIB_BASE_FIELD): New macros. (REG_PREFIX_P): New macro. * amd64-tdep.h (amd64_displaced_step_copy_insn): Declare. (amd64_displaced_step_fixup): Declare. * amd64-tdep.c: #include opcode/i386.h, dis-asm.h. (amd64_arch_regmap): Move out of amd64_analyze_stack_align and make static global. (amd64_arch_regmap_len): New static global. (amd64_arch_reg_to_regnum): New function. (struct amd64_insn): New struct. (struct displaced_step_closure): New struct. (onebyte_has_modrm,twobyte_has_modrm): New static globals. (rex_prefix_p,skip_prefixes) (amd64_insn_length_fprintf,amd64_insn_length_init_dis) (amd64_insn_length,amd64_get_unused_input_int_reg) (amd64_get_insn_details,fixup_riprel,fixup_displaced_copy) (amd64_displaced_step_copy_insn) (amd64_absolute_jmp_p,amd64_absolute_call_p,amd64_ret_p) (amd64_call_p,amd64_breakpoint_p,amd64_syscall_p) (amd64_displaced_step_fixup): New functions. * amd64-linux-tdep.c: #include arch-utils.h. (amd64_linux_init_abi): Install displaced stepping support. * gdb.arch/amd64-disp-step.S: New file. * gdb.arch/amd64-disp-step.exp: New file. * gdb.arch/i386-disp-step.S: New file. * gdb.arch/i386-disp-step.exp: New file.
Attachment:
gdb-090128-amd64-disp-step-2.patch.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |