From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8382 invoked by alias); 27 Jan 2009 17:15:03 -0000 Received: (qmail 8207 invoked by uid 22791); 27 Jan 2009 17:15:00 -0000 X-SWARE-Spam-Status: No, hits=-1.1 required=5.0 tests=AWL,BAYES_20,J_CHICKENPOX_44,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Jan 2009 17:14:51 +0000 Received: (qmail 18526 invoked from network); 27 Jan 2009 17:14:48 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 27 Jan 2009 17:14:48 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA] amd64 displaced stepping support Date: Tue, 27 Jan 2009 17:27:00 -0000 User-Agent: KMail/1.9.10 Cc: Doug Evans References: <20090126230013.5C7F01C72DE@localhost> In-Reply-To: <20090126230013.5C7F01C72DE@localhost> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <200901271715.49852.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-01/txt/msg00519.txt.bz2 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 don't like these spread around we-should-have-this-super-library references. Please, if not proposing new interfaces yourself, centralize the complain, if you must, and when this goes in, open a PR about it. > This also includes a testcase! :-) > Plus I added a testcase for the i386 case. Yay! > > Ok to check in? Close! > (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. > +/* ??? *_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. 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... > +/* 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." ... > +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? > +/* 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. > +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. > + 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? > + > + 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 (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. Something in the kernel not restoring the trace flag correctly, and hence stepping one instruction too much? > +/* ??? 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. > + > +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? > + > + 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. > + 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? > +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. > +# Done, run program to exit. > + > +gdb_test "continue" \ > + ".*Program exited normally.*" \ > + "continue to exit" > Use gdb_continue_to_end instead. -- Pedro Alves