This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFA] i386/amd64 displaced stepping fixes
- From: dje at google dot com (Doug Evans)
- To: gdb-patches at sourceware dot org
- Date: Sun, 1 Feb 2009 18:46:54 -0800 (PST)
- Subject: [RFA] i386/amd64 displaced stepping fixes
Hi. This patch fixes a few things in i386/amd64 displaced stepping.
1) Assuming(!) it is correct to not back up the pc when stepping over an int3,
this updates both arches' fixup routines to not do this.
2) Adds prefix scanning to the i386 support.
These prefixes are normally not present, but I think gdb shouldn't
ignore them.
3) Adds handling for more 3-byte opcodes in the amd64 port.
4) Adds new tests to the i386 and amd64 displaced stepping tests.
5) minor cleanups like constifying the `insn' parameter to the i386
instruction predicates
Question: There's both i386_skip_prefixes and amd64_skip_prefixes
functions. Arguably there should be only one, though they currently
have different signatures. If there's a strong feeling that
there should be only one, I can rewrite the patch to use only one function,
but I'm not sure where to put it. Suggestions?
Ok to check in?
2009-02-01 Doug Evans <dje@google.com>
* amd64-tdep.c (amd64_skip_prefixes): Renamed from skip_prefixes.
All callers updated.
(amd64_get_insn_details): Handle more 3-byte opcode insns.
(amd64_breakpoint_p): Delete.
(amd64_displaced_step_fixup): When fixing up after stepping an int3,
don't back up pc to the start of the int3.
* i386-tdep.c: #include opcode/i386.h.
(i386_skip_prefixes): New function.
(i386_absolute_jmp_p): Constify argument.
(i386_absolute_call_p,i386_ret_p,i386_call_p,i386_syscall_p): Ditto.
(i386_breakpoint_p): Delete.
(i386_displaced_step_fixup): Handle unnecessary or redundant prefixes.
When fixing up after stepping an int3, don't back up pc to the start
of the int3.
* gdb.arch/amd64-disp-step.S (test_int3): New test.
* gdb.arch/amd64-disp-step.exp (test_int3): New test.
* gdb.arch/i386-disp-step.S (test_prefixed_abs_jump): New test.
(test_prefixed_syscall,test_int3): New tests.
* gdb.arch/i386-disp-step.exp (test_prefixed_abs_jump): New test.
(test_prefixed_syscall,test_int3): New tests.
Index: amd64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/amd64-tdep.c,v
retrieving revision 1.57
diff -u -p -u -p -r1.57 amd64-tdep.c
--- amd64-tdep.c 29 Jan 2009 00:29:56 -0000 1.57
+++ amd64-tdep.c 2 Feb 2009 02:33:36 -0000
@@ -805,7 +805,7 @@ rex_prefix_p (gdb_byte pfx)
about falling off the end of the buffer. */
static gdb_byte *
-skip_prefixes (gdb_byte *insn)
+amd64_skip_prefixes (gdb_byte *insn)
{
while (1)
{
@@ -974,7 +974,7 @@ amd64_get_insn_details (gdb_byte *insn,
details->modrm_offset = -1;
/* Skip legacy instruction prefixes. */
- insn = skip_prefixes (insn);
+ insn = amd64_skip_prefixes (insn);
/* Skip REX instruction prefix. */
if (rex_prefix_p (*insn))
@@ -992,13 +992,21 @@ amd64_get_insn_details (gdb_byte *insn,
need_modrm = twobyte_has_modrm[*insn];
/* Check for three-byte opcode. */
- if (*insn == 0x38 || *insn == 0x3a)
+ switch (*insn)
{
+ case 0x24:
+ case 0x25:
+ case 0x38:
+ case 0x3a:
+ case 0x7a:
+ case 0x7b:
++insn;
details->opcode_len = 3;
+ break;
+ default:
+ details->opcode_len = 2;
+ break;
}
- else
- details->opcode_len = 2;
}
else
{
@@ -1217,14 +1225,6 @@ amd64_call_p (const struct amd64_insn *d
return 0;
}
-static int
-amd64_breakpoint_p (const struct amd64_insn *details)
-{
- const gdb_byte *insn = &details->raw_insn[details->opcode_offset];
-
- return insn[0] == 0xcc; /* int 3 */
-}
-
/* Return non-zero if INSN is a system call, and set *LENGTHP to its
length in bytes. Otherwise, return zero. */
@@ -1323,21 +1323,6 @@ amd64_displaced_step_fixup (struct gdbar
{
ULONGEST rip = orig_rip - insn_offset;
- /* If we have stepped over a breakpoint, set %rip to
- point at the breakpoint instruction itself.
-
- (gdbarch_decr_pc_after_break was never something the core
- of GDB should have been concerned with; arch-specific
- code should be making PC values consistent before
- presenting them to GDB.) */
- if (amd64_breakpoint_p (insn_details))
- {
- if (debug_displaced)
- fprintf_unfiltered (gdb_stdlog,
- "displaced: stepped breakpoint\n");
- rip--;
- }
-
regcache_cooked_write_unsigned (regs, AMD64_RIP_REGNUM, rip);
if (debug_displaced)
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.267
diff -u -p -u -p -r1.267 i386-tdep.c
--- i386-tdep.c 3 Jan 2009 05:57:51 -0000 1.267
+++ i386-tdep.c 2 Feb 2009 02:33:37 -0000
@@ -20,6 +20,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include "defs.h"
+#include "opcode/i386.h"
#include "arch-utils.h"
#include "command.h"
#include "dummy-frame.h"
@@ -278,9 +279,44 @@ i386_breakpoint_from_pc (struct gdbarch
/* Displaced instruction handling. */
+/* Skip the legacy instruction prefixes in INSN.
+ Not all prefixes are valid for any particular insn
+ but we needn't care, the insn will fault if it's invalid.
+ The result is a pointer to the first opcode byte,
+ or NULL if we run off the end of the buffer. */
+
+static gdb_byte *
+i386_skip_prefixes (gdb_byte *insn, size_t max_len)
+{
+ gdb_byte *end = insn + max_len;
+
+ while (insn < end)
+ {
+ switch (*insn)
+ {
+ case DATA_PREFIX_OPCODE:
+ case ADDR_PREFIX_OPCODE:
+ case CS_PREFIX_OPCODE:
+ case DS_PREFIX_OPCODE:
+ case ES_PREFIX_OPCODE:
+ case FS_PREFIX_OPCODE:
+ case GS_PREFIX_OPCODE:
+ case SS_PREFIX_OPCODE:
+ case LOCK_PREFIX_OPCODE:
+ case REPE_PREFIX_OPCODE:
+ case REPNE_PREFIX_OPCODE:
+ ++insn;
+ continue;
+ default:
+ return insn;
+ }
+ }
+
+ return NULL;
+}
static int
-i386_absolute_jmp_p (gdb_byte *insn)
+i386_absolute_jmp_p (const gdb_byte *insn)
{
/* jmp far (absolute address in operand) */
if (insn[0] == 0xea)
@@ -301,7 +337,7 @@ i386_absolute_jmp_p (gdb_byte *insn)
}
static int
-i386_absolute_call_p (gdb_byte *insn)
+i386_absolute_call_p (const gdb_byte *insn)
{
/* call far, absolute */
if (insn[0] == 0x9a)
@@ -322,7 +358,7 @@ i386_absolute_call_p (gdb_byte *insn)
}
static int
-i386_ret_p (gdb_byte *insn)
+i386_ret_p (const gdb_byte *insn)
{
switch (insn[0])
{
@@ -339,7 +375,7 @@ i386_ret_p (gdb_byte *insn)
}
static int
-i386_call_p (gdb_byte *insn)
+i386_call_p (const gdb_byte *insn)
{
if (i386_absolute_call_p (insn))
return 1;
@@ -351,16 +387,11 @@ i386_call_p (gdb_byte *insn)
return 0;
}
-static int
-i386_breakpoint_p (gdb_byte *insn)
-{
- return insn[0] == 0xcc; /* int 3 */
-}
-
/* Return non-zero if INSN is a system call, and set *LENGTHP to its
length in bytes. Otherwise, return zero. */
+
static int
-i386_syscall_p (gdb_byte *insn, ULONGEST *lengthp)
+i386_syscall_p (const gdb_byte *insn, ULONGEST *lengthp)
{
if (insn[0] == 0xcd)
{
@@ -373,6 +404,7 @@ i386_syscall_p (gdb_byte *insn, ULONGEST
/* Fix up the state of registers and memory after having single-stepped
a displaced instruction. */
+
void
i386_displaced_step_fixup (struct gdbarch *gdbarch,
struct displaced_step_closure *closure,
@@ -388,6 +420,8 @@ i386_displaced_step_fixup (struct gdbarc
/* Since we use simple_displaced_step_copy_insn, our closure is a
copy of the instruction. */
gdb_byte *insn = (gdb_byte *) closure;
+ /* The start of the insn, needed in case we see some prefixes. */
+ gdb_byte *insn_start = insn;
if (debug_displaced)
fprintf_unfiltered (gdb_stdlog,
@@ -401,6 +435,18 @@ i386_displaced_step_fixup (struct gdbarc
/* Relocate the %eip, if necessary. */
+ /* The instruction recognizers we use assume any leading prefixes
+ have been skipped. */
+ {
+ /* This is the size of the buffer in closure. */
+ size_t max_insn_len = gdbarch_max_insn_length (gdbarch);
+ gdb_byte *opcode = i386_skip_prefixes (insn, max_insn_len);
+ /* If there are too many prefixes, just ignore the insn.
+ It will fault when run. */
+ if (opcode != NULL)
+ insn = opcode;
+ }
+
/* Except in the case of absolute or indirect jump or call
instructions, or a return instruction, the new eip is relative to
the displaced instruction; make it relative. Well, signal
@@ -430,7 +476,7 @@ i386_displaced_step_fixup (struct gdbarc
it unrelocated. Goodness help us if there are PC-relative
system calls. */
if (i386_syscall_p (insn, &insn_len)
- && orig_eip != to + insn_len)
+ && orig_eip != to + (insn - insn_start) + insn_len)
{
if (debug_displaced)
fprintf_unfiltered (gdb_stdlog,
@@ -441,21 +487,6 @@ i386_displaced_step_fixup (struct gdbarc
{
ULONGEST eip = (orig_eip - insn_offset) & 0xffffffffUL;
- /* If we have stepped over a breakpoint, set the %eip to
- point at the breakpoint instruction itself.
-
- (gdbarch_decr_pc_after_break was never something the core
- of GDB should have been concerned with; arch-specific
- code should be making PC values consistent before
- presenting them to GDB.) */
- if (i386_breakpoint_p (insn))
- {
- if (debug_displaced)
- fprintf_unfiltered (gdb_stdlog,
- "displaced: stepped breakpoint\n");
- eip--;
- }
-
regcache_cooked_write_unsigned (regs, I386_EIP_REGNUM, eip);
if (debug_displaced)
@@ -493,8 +524,6 @@ i386_displaced_step_fixup (struct gdbarc
paddr_nz (retaddr));
}
}
-
-
#ifdef I386_REGNO_TO_SYMMETRY
#error "The Sequent Symmetry is no longer supported."
Index: testsuite/gdb.arch/amd64-disp-step.S
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/amd64-disp-step.S,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 amd64-disp-step.S
--- testsuite/gdb.arch/amd64-disp-step.S 29 Jan 2009 00:29:57 -0000 1.1
+++ testsuite/gdb.arch/amd64-disp-step.S 2 Feb 2009 02:33:37 -0000
@@ -23,6 +23,8 @@
main:
nop
+/***********************************************/
+
/* test call/ret */
.global test_call
@@ -33,6 +35,8 @@ test_call:
test_ret_end:
nop
+/***********************************************/
+
/* test abs-jmp/rep-ret */
test_abs_jmp_setup:
@@ -48,6 +52,8 @@ test_abs_jmp_return:
test_rep_ret_end:
nop
+/***********************************************/
+
/* test syscall */
.global test_syscall
@@ -58,6 +64,24 @@ test_syscall:
test_syscall_end:
nop
+/***********************************************/
+
+/* Test stepping over int3.
+ The prefixes are pointless, but it's possible, so we exercise it. */
+
+ nop
+ .global test_int3
+test_int3:
+ repz
+ repz
+ int3
+ nop
+ .global test_int3_end
+test_int3_end:
+ nop
+
+/***********************************************/
+
/* test rip-relative
GDB picks a spare register to hold the rip-relative address.
Exercise all the possibilities (rax-rdi, sans rsp). */
@@ -118,6 +142,8 @@ test_rip_rdi_end:
answer: .8byte 42
+/***********************************************/
+
/* all done */
done:
@@ -139,6 +165,8 @@ test_call_end:
test_ret:
ret
+/***********************************************/
+
/* subroutine to help test abs-jmp/rep-ret */
test_abs_jmp_subr:
Index: testsuite/gdb.arch/amd64-disp-step.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/amd64-disp-step.exp,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 amd64-disp-step.exp
--- testsuite/gdb.arch/amd64-disp-step.exp 29 Jan 2009 00:29:57 -0000 1.1
+++ testsuite/gdb.arch/amd64-disp-step.exp 2 Feb 2009 02:33:37 -0000
@@ -141,6 +141,29 @@ gdb_test "continue" \
##########################################
+# int3 (with prefixes)
+# These don't occur in normal code, but gdb should still DTRT.
+
+gdb_test "break test_int3" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_int3"
+gdb_test "break test_int3_end" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_int3_end"
+
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_int3 ().*" \
+ "continue to test_int3"
+# FIXME: Single stepping over an int3 currently doesn't generate a SIGTRAP.
+#gdb_test "continue" \
+# "Continuing.*SIGTRAP.*" \
+# "continue to test_int3_SIGTRAP"
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_int3_end ().*" \
+ "continue to test_int3_end"
+
+##########################################
+
# Test rip-relative.
# GDB picks a spare register to hold the rip-relative address.
# Exercise all the possibilities (rax-rdi, sans rsp).
Index: testsuite/gdb.arch/i386-disp-step.S
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/i386-disp-step.S,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 i386-disp-step.S
--- testsuite/gdb.arch/i386-disp-step.S 29 Jan 2009 00:29:57 -0000 1.1
+++ testsuite/gdb.arch/i386-disp-step.S 2 Feb 2009 02:33:37 -0000
@@ -23,8 +23,11 @@
main:
nop
-/* test call/ret */
+/***********************************************/
+
+/* Test call/ret. */
+ nop
.global test_call
test_call:
call test_call_subr
@@ -33,16 +36,72 @@ test_call:
test_ret_end:
nop
-/* test syscall */
+/***********************************************/
+
+/* Absolute jump with leading prefixes.
+ These don't occur in normal code, but gdb should still DTRT. */
+
+ nop
+ .global test_prefixed_abs_jump
+test_prefixed_abs_jump:
+ ds
+ jmp *test_prefixed_abs_jump_addr
+ .data
+test_prefixed_abs_jump_addr:
+ .4byte test_prefixed_abs_jump_target
+ .text
+test_prefixed_abs_jump_target:
+ nop
+ .global test_prefixed_abs_jump_end
+test_prefixed_abs_jump_end:
+ nop
+
+/***********************************************/
+
+/* Test syscall. */
- .global test_syscall
mov $0x14,%eax /* getpid */
+ .global test_syscall
test_syscall:
int $0x80
nop
+ .global test_syscall_end
test_syscall_end:
nop
+/***********************************************/
+
+/* Test syscall again, this time with a prefix.
+ These don't occur in normal code, but gdb should still DTRT. */
+
+ mov $0x14,%eax /* getpid */
+ .global test_prefixed_syscall
+test_prefixed_syscall:
+ repnz
+ int $0x80
+ nop
+ .global test_prefixed_syscall_end
+test_prefixed_syscall_end:
+ nop
+
+/***********************************************/
+
+/* Test stepping over int3.
+ The prefixes are pointless, but it's possible, so we exercise it. */
+
+ nop
+ .global test_int3
+test_int3:
+ repz
+ repz
+ int3
+ nop
+ .global test_int3_end
+test_int3_end:
+ nop
+
+/***********************************************/
+
/* all done */
pushl $0
Index: testsuite/gdb.arch/i386-disp-step.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/i386-disp-step.exp,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 i386-disp-step.exp
--- testsuite/gdb.arch/i386-disp-step.exp 29 Jan 2009 00:29:57 -0000 1.1
+++ testsuite/gdb.arch/i386-disp-step.exp 2 Feb 2009 02:33:37 -0000
@@ -89,6 +89,25 @@ gdb_test "continue" \
##########################################
+# Absolute jump with leading prefixes.
+# These don't occur in normal code, but gdb should still DTRT.
+
+gdb_test "break test_prefixed_abs_jump" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_prefixed_abs_jump"
+gdb_test "break test_prefixed_abs_jump_end" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_prefixed_abs_jump_end"
+
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_prefixed_abs_jump ().*" \
+ "continue to test_prefixed_abs_jump"
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_prefixed_abs_jump_end ().*" \
+ "continue to test_prefixed_abs_jump_end"
+
+##########################################
+
# Test syscall.
gdb_test "break test_syscall" \
@@ -107,6 +126,48 @@ gdb_test "continue" \
##########################################
+# Test prefixed syscall.
+# These don't occur in normal code, but gdb should still DTRT.
+
+gdb_test "break test_prefixed_syscall" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_prefixed_syscall"
+gdb_test "break test_prefixed_syscall_end" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_prefixed_syscall_end"
+
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_prefixed_syscall ().*" \
+ "continue to test_prefixed_syscall"
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_prefixed_syscall_end ().*" \
+ "continue to test_prefixed_syscall_end"
+
+##########################################
+
+# int3 (with prefixes)
+# These don't occur in normal code, but gdb should still DTRT.
+
+gdb_test "break test_int3" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_int3"
+gdb_test "break test_int3_end" \
+ "Breakpoint.*at.* file .*$srcfile, line.*" \
+ "break test_int3_end"
+
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_int3 ().*" \
+ "continue to test_int3"
+# FIXME: Single stepping over an int3 currently doesn't generate a SIGTRAP.
+#gdb_test "continue" \
+# "Continuing.*SIGTRAP.*" \
+# "continue to test_int3_SIGTRAP"
+gdb_test "continue" \
+ "Continuing.*Breakpoint.*, test_int3_end ().*" \
+ "continue to test_int3_end"
+
+##########################################
+
# Done, run program to exit.
gdb_continue_to_end "i386-disp-step"