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]

Re: [RFA] i386/amd64 displaced stepping fixes


Ping.

On Sun, Feb 1, 2009 at 6:46 PM, Doug Evans <dje@google.com> 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  <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"
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]