Re: [patch] Fix disp-step-syscall.exp on some i386 targets

[Yao Qi <yao at codesourcery dot com>]

On 02/28/2012 03:22 AM, Jan Kratochvil wrote:
> i386-linux-tdep.c
> -----------------
> Then there was FAIL on native i686: This is because its vDSO is unusual and GDB
> gets confusied by 'ret' while still only the second part of 'int $0x80' gets
> stepped.  More in the patch comment.
> 
> Dump of assembler code for function __kernel_vsyscall:
> => 0xb7fff414 <+0>:	int    $0x80
>    0xb7fff416 <+2>:	ret    
> End of assembler dump.

> +/* Linux kernel shows PC value after the 'int $0x80' instruction even if
> +   inferior is still inside the syscall.  On next PTRACE_SINGLESTEP it will
> +   finish the syscall but PC will not change.  Some vDSOs contain

The problem statement looks right to me.  The debugging log can reveal
this problem,

=> 0x40000830:  cd 80   int    $0x80^M
   0x40000832:  c3      ret
(gdb) PASS: gdb.base/disp-step-syscall.exp: fork: set debug infrun 1
stepi^M
infrun: clear_proceed_status_thread (process 2393)^M
infrun: proceed (addr=0xffffffff, signal=144, step=1)^M
infrun: resume (step=1, signal=0), trap_expected=1, current thread
[process 2393] at 0x40000830^M
displaced: stepping process 2393 now^M
displaced: saved 0x8048392: 5e 89 e1 83 e4 f0 50 54 52 68 b0 84 04 08 68
c0 ^M
displaced: copy 0x40000830->0x8048392: cd 80 90 8d b6 00 00 00 00 8d bc
27 00 00 00 00 ^M
displaced: displaced pc to 0x8048392^M
displaced: run 0x8048392: cd 80 90 8d ^M
infrun: wait_for_inferior ()^M
infrun: target_wait (-1, status) =^M
infrun:   2393 [process 2393],^M
infrun:   status->kind = forked^M
infrun: infwait_normal_state^M
infrun: TARGET_WAITKIND_FORKED^M
displaced: restored process 2393 0x8048392
displaced: fixup (0x40000830, 0x8048392), insn = 0xcd 0x80 ...^M
displaced: relocated %eip from 0x8048394 to 0x40000832
                                            ^^^^^^^^^^
displaced: restore scratchpad for child^M
displaced: restored process 2398 0x8048392^M
displaced: write child pc from 0x8048394 to 0x40000832

In TARGET_WAITKIND_FORKED, pc value has been incremented by 2, to
0x40000832.

> +   'int $0x80; ret' and i386_displaced_step_fixup would keep PC at the displaced
> +   location expecting it just executed 'ret' despite it finished the syscall.
> +   Hide the 'ret' instruction by 'nop'.  */
> +

We write 'ret' into scratchpad, but return 'nop' in closure.  I am
afraid it is a little risky.

> +struct displaced_step_closure *
> +i386_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
> +				     CORE_ADDR from, CORE_ADDR to,
> +				     struct regcache *regs)
> +{
> +  struct displaced_step_closure *closure;
> +  
> +  closure = i386_displaced_step_copy_insn (gdbarch, from, to, regs);
> +
> +  if (i386_linux_get_syscall_number_from_regcache (regs) != -1)

I don't understand this.  If we are doing displaced stepping on
arbitrary instruction, the condition is always true?, I guess.

> +    {
> +      /* Since we use simple_displaced_step_copy_insn, our closure is a
> +	 copy of the instruction.  */
> +      gdb_byte *insn = (gdb_byte *) closure;
> +
> +      /* Fake nop.  */
> +      insn[0] = 0x90;
> +    }
> +
> +  return closure;
> +}

I am afraid this fix is not correct.

Let us continue to look at the debugging log first.  After
TARGET_WAITKIND_FORKED, pc has been updated to 0x40000832, and we
single-step inferior,

infrun: resume (step=1, signal=0), trap_expected=1, current thread
[process 2393] at 0x40000832^M
displaced: stepping process 2393 now^M
displaced: saved 0x8048392: 5e 89 e1 83 e4 f0 50 54 52 68 b0 84 04 08 68
c0 ^M
displaced: copy 0x40000832->0x8048392: c3 8d b6 00 00 00 00 8d bc 27 00
00 00 00 8b 1c ^M
displaced: displaced pc to 0x8048392^M
displaced: run 0x8048392: c3 8d b6 00 ^M
infrun: prepare_to_wait^M
infrun: target_wait (-1, status) =^M
infrun:   2393 [process 2393],^M
infrun:   status->kind = stopped, signal = SIGTRAP^M
infrun: infwait_normal_state^M
infrun: TARGET_WAITKIND_STOPPED^M
displaced: restored process 2393 0x8048392^M
displaced: fixup (0x40000832, 0x8048392), insn = 0xc3 0x8d ...^M
infrun: stop_pc = 0x8048392

IIUC, this SIGTRAP is reported for single-step `int 0x80', and that is
why pc is not changed.  GDB is confused by this unchanged pc.

So, the proper fix would be defining i386_linux_displaced_step_fixup in
i386-linux-tdep.c to check whether pc is changed (equal to the address
of scratchpad).  If pc is not changed, this means pc has been updated in
previous syscall event, and restore pc to its original place.
Otherwise, call i386_displaced_step_fixup.   The attached patch is to do
what I said here.  WDYT?  I didn't run this patch in the whole test
suite yet.

> --- a/gdb/testsuite/gdb.base/disp-step-syscall.exp
> +++ b/gdb/testsuite/gdb.base/disp-step-syscall.exp
> @@ -25,7 +25,7 @@ set syscall_insn ""
>  # Define the syscall instruction for each target.
>  
>  if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
> -    set syscall_insn "(int|syscall|sysenter)"
> +    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
>  } else {
>      return -1
>  }
> @@ -118,7 +117,16 @@ proc disp_step_cross_syscall { syscall } { with_test_prefix "$syscall" {
>      gdb_test_no_output "set displaced-stepping on"
>  
>      # Check the address of next instruction of syscall.
> -    gdb_test "stepi" ".*$syscall_insn_next_addr.*" "single step over $syscall"
> +    gdb_test "stepi" ".*" "single step over $syscall"
> +    set syscall_insn_next_addr_found [get_hexadecimal_valueof "\$pc" "0"]
> +
> +    set test "single step over $syscall final pc"
> +    if {$syscall_insn_next_addr != 0

Is this check really necessary?  Looks $syscall_insn_next_addr is always
non-zero.

> +	&& $syscall_insn_next_addr == $syscall_insn_next_addr_found} {
> +      pass $test
> +    } else {
> +      fail $test
> +    }
>  

-- 
Yao (éå)

	* i386-linux-tdep.c (i386_linux_displaced_step_fixup): New.
	(i386_linux_init_abi): Install i386_linux_displaced_step_fixup.
---
 gdb/i386-linux-tdep.c |   32 +++++++++++++++++++++++++++++++-
 1 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 61800b4..cbfe857 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -644,6 +644,36 @@ i386_linux_core_read_description (struct gdbarch *gdbarch,
 }
 
 static void
+i386_linux_displaced_step_fixup (struct gdbarch *gdbarch,
+				 struct displaced_step_closure *closure,
+				 CORE_ADDR from, CORE_ADDR to,
+				 struct regcache *regs)
+{
+  ULONGEST orig_eip;
+
+  regcache_cooked_read_unsigned (regs, I386_EIP_REGNUM, &orig_eip);
+
+  if (orig_eip == to)
+    {
+      /* The inferior is stopped without any pc value updates.  This usually
+	 means pc value has been incremented in previous syscall event.  In
+	 this case, set pc value to its original place.  */
+      ULONGEST eip = (orig_eip - (to - from)) & 0xffffffffUL;
+
+      regcache_cooked_write_unsigned (regs, I386_EIP_REGNUM, eip);
+
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "displaced: "
+			    "relocated %%eip from %s to %s\n",
+			    paddress (gdbarch, orig_eip),
+			    paddress (gdbarch, eip));
+    }
+  else
+    i386_displaced_step_fixup (gdbarch, closure, from, to, regs);
+}
+
+static void
 i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
@@ -891,7 +921,7 @@ i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   /* Displaced stepping.  */
   set_gdbarch_displaced_step_copy_insn (gdbarch,
                                         i386_displaced_step_copy_insn);
-  set_gdbarch_displaced_step_fixup (gdbarch, i386_displaced_step_fixup);
+  set_gdbarch_displaced_step_fixup (gdbarch, i386_linux_displaced_step_fixup);
   set_gdbarch_displaced_step_free_closure (gdbarch,
                                            simple_displaced_step_free_closure);
   set_gdbarch_displaced_step_location (gdbarch,
-- 
1.7.0.4