From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12236 invoked by alias); 7 Feb 2009 00:12:54 -0000 Received: (qmail 12226 invoked by uid 22791); 7 Feb 2009 00:12:51 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.45.13) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 07 Feb 2009 00:12:46 +0000 Received: from zps76.corp.google.com (zps76.corp.google.com [172.25.146.76]) by smtp-out.google.com with ESMTP id n170ChAA007486 for ; Fri, 6 Feb 2009 16:12:44 -0800 Received: from rv-out-0708.google.com (rvfb17.prod.google.com [10.140.179.17]) by zps76.corp.google.com with ESMTP id n170Cf75004991 for ; Fri, 6 Feb 2009 16:12:42 -0800 Received: by rv-out-0708.google.com with SMTP id b17so1104235rvf.46 for ; Fri, 06 Feb 2009 16:12:41 -0800 (PST) MIME-Version: 1.0 Received: by 10.141.193.9 with SMTP id v9mr1762374rvp.120.1233965561752; Fri, 06 Feb 2009 16:12:41 -0800 (PST) In-Reply-To: <20090202024654.8BD461C7A19@localhost> References: <20090202024654.8BD461C7A19@localhost> Date: Sat, 07 Feb 2009 00:12:00 -0000 Message-ID: Subject: Re: [RFA] i386/amd64 displaced stepping fixes From: Doug Evans To: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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-02/txt/msg00168.txt.bz2 Ping. On Sun, Feb 1, 2009 at 6:46 PM, 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. > 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 > > * 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 . */ > > #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" >