Re: [RFA] amd64 displaced stepping support

[Pedro Alves <pedro at codesourcery dot com>]

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 don't like these spread around we-should-have-this-super-library
references.  Please, if not proposing new interfaces yourself,
centralize the complain, if you must, and when this goes in,
open a PR about it.

> This also includes a testcase! :-)
> Plus I added a testcase for the i386 case.

Yay!

> 
> Ok to check in?

Close!

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Use gdb_continue_to_end instead.

-- 
Pedro Alves