Re: [RFA] amd64 displaced stepping support

[Doug Evans <dje at google dot com>]

On Tue, Jan 27, 2009 at 9:15 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> 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'd like to add the needed interfaces.  I'm just not sure how long a
process that will be and I'd like to get started on exercising amd64
non-stop functionality.  I'll pursue exporting the modrm_bytes arrays
and insn-length computation with binutils.

>
>>       (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.

ok.

>
>> +/* ??? *_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.

I assumed so, but didn't actually know.

> 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...

I've modified the comment to be a warning.

>> +/* 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." ...

The attached patch has a different wording.

>> +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?

righto.

>> +/* 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.

Ah, righto.

>> +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.

Ya, oops.  That one I intended to fix but forgot about.  Fixed.

>> +  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?

The code allocates sufficient extra space to obviate having to
continually test "are we at the end yet?".
A lot of this is copied from i386-tdep.c.  If this patch is ok, I'll
send patches for i386-tdep.c.

>
>> +
>> +  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 <test_syscall_end>
>  (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.

Ya, this is what I discovered during my experiments to understand this.

> Something in the kernel not restoring the trace flag correctly, and
> hence stepping one instruction too much?

Presumably.

>
>> +/* ??? 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.

Well, that comment refers to the insn predicates like amd64_call_p.
Having the caller skip the prefixes muddies their interface.  The
appended patch does something cleaner.

>
>> +
>> +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?

Dropped.

>
>> +
>> +  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.

Righto.

>
>> +  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?

cut-n-paste.  Deleted.

>
>> +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.

Righto.

>
>> +# Done, run program to exit.
>> +
>> +gdb_test "continue" \
>> +    ".*Program exited normally.*" \
>> +    "continue to exit"
>>
>
> Use gdb_continue_to_end instead.

Righto.

Ok to check in?
[Modulo I need to clear the opcode/i386.h additions with binutils.
I'm not sure who to get approval from first.]

2009-01-27  Doug Evans  <dje@google.com>

        * opcode/i386.h: Add multiple inclusion protection.
        (EAX_REG_NUM,ECX_REG_NUM,EDX_REGNUM,EBX_REG_NUM,ESI_REG_NUM)
        (EDI_REG_NUM): New macros.
        (MODRM_MOD_FIELD,MODRM_REG_FIELD,MODRM_RM_FIELD): New macros.
        (SIB_SCALE_FIELD,SIB_INDEX_FIELD,SIB_BASE_FIELD): New macros.
        (REG_PREFIX_P): New macro.

        * amd64-tdep.h (amd64_displaced_step_copy_insn): Declare.
        (amd64_displaced_step_fixup): Declare.
        * amd64-tdep.c: #include opcode/i386.h, dis-asm.h.
        (amd64_arch_regmap): Move out of amd64_analyze_stack_align
        and make static global.
        (amd64_arch_regmap_len): New static global.
        (amd64_arch_reg_to_regnum): New function.
        (struct amd64_insn): New struct.
        (struct displaced_step_closure): New struct.
        (onebyte_has_modrm,twobyte_has_modrm): New static globals.
        (rex_prefix_p,skip_prefixes)
        (amd64_insn_length_fprintf,amd64_insn_length_init_dis)
        (amd64_insn_length,amd64_get_unused_input_int_reg)
        (amd64_get_insn_details,fixup_riprel,fixup_displaced_copy)
        (amd64_displaced_step_copy_insn)
        (amd64_absolute_jmp_p,amd64_absolute_call_p,amd64_ret_p)
        (amd64_call_p,amd64_breakpoint_p,amd64_syscall_p)
        (amd64_displaced_step_fixup): New functions.
        * amd64-linux-tdep.c: #include arch-utils.h.
        (amd64_linux_init_abi): Install displaced stepping support.

        * gdb.arch/amd64-disp-step.S: New file.
        * gdb.arch/amd64-disp-step.exp: New file.
        * gdb.arch/i386-disp-step.S: New file.
        * gdb.arch/i386-disp-step.exp: New file.

Attachment: gdb-090128-amd64-disp-step-2.patch.txt
Description: Text document