[RFA] i386/amd64 displaced stepping fixes
Pedro Alves
pedro@codesourcery.com
Sat Feb 7 01:18:00 GMT 2009
On Monday 02 February 2009 02:46:54, Doug Evans wrote:
> 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.
Who knows what in inferior with a SIGTRAP signal handler may
be doing? It may be handling breakpoints and backing off the pc
itself. We can mostly consider SIGTRAP ours, but if we can
avoid changing the inferiors' behaviour vs what would happen
without a debugger, we should. Also, if it doesn't happen without
displaced stepping, it shouldn't also with.
> 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?
We'll you could put it in i386-tdep.c, and have amd64-tdep.c call it.
Not sure if it's worth it for such little sharing. I do have the
feeling we'll be merging the 64-bit and 32-bit displaced stepping
implementations into i386-tdep.c at some point.
> 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.
I trust you, so I didn't go open up the manuals to check the opcodes.
> (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.
>
Looks OK to me, thanks. Minor comments inline below.
> 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--;
> - }
> -
Now that you talk about it, this assumed that
gdbarch_decr_pc_after_break was 1, but it could not be. E.g., i386-nto-tdep.c
resets it to 0. Granted, it doesn't use this (at least yet)...
> 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;
No preference either way, but another option here would be
for i386_skip_prefixes to return INSN unmodified if the
insn was bogus. Then you'd not need a NULL check.
> + }
> +
> /* 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"
This isn't related to displaced stepping, is it? This is
happening in standard stepping too... So, it looks to me that
if you really really really want to add a fixme for this, we
should have a PR about it, and this should be a KFAILed
test (not commented out), and there should be a similar
test somewhere else not using displaced stepping.
(this exp itself could be running two times, once with and
another without displaced stepping)
> +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"
>
--
Pedro Alves
More information about the Gdb-patches
mailing list