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

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

[Jan Kratochvil]

Hi,

I had some problems on various kernels all on i686 kernel (both iron and KVM).
Like:

 stepi
-0xf7fdc421 in __kernel_vsyscall ()
+Detaching after fork from child process 5426.
+0x08048362 in _start ()
 1: x/i $pc
-=> 0xf7fdc421 <__kernel_vsyscall+1>:   push   %edx
-(gdb) PASS: gdb.base/disp-step-syscall.exp: fork: single step over fork
+=> 0x8048362 <_start+2>:       pop    %esi
+(gdb) FAIL: gdb.base/disp-step-syscall.exp: fork: single step over fork

This was completely bogus because the testcase matched "__kernel_vsyscall" as
instruction "syscall".

disp-step-syscall.exp
---------------------
So first I fixed the testcase instrution check.

Then there was another problem that it could not match x86_64 addresses.
It found adress 0x7ffff77e6544 but then it failed to match it:
stepi
0x00007ffff77e6544      131       pid = ARCH_FORK ();
So I fixed that part.

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.

Fixed that.

i386-tdep.c
-----------
After fixing the testcase it started failing on x86_64 -m32 (Intel CPU)
because it was getting false PASSes by the broken testcase before.

This is because GDB did not recognize the 'sysenter' instruction in:

Dump of assembler code for function __kernel_vsyscall:
   0xf7fdc420 <+0>:     push   %ecx
   0xf7fdc421 <+1>:     push   %edx
   0xf7fdc422 <+2>:     push   %ebp
   0xf7fdc423 <+3>:     mov    %esp,%ebp
=> 0xf7fdc425 <+5>:     sysenter
   0xf7fdc427 <+7>:     nop
   0xf7fdc428 <+8>:     nop
   0xf7fdc429 <+9>:     nop
   0xf7fdc42a <+10>:    nop
   0xf7fdc42b <+11>:    nop
   0xf7fdc42c <+12>:    nop
   0xf7fdc42d <+13>:    nop
   0xf7fdc42e <+14>:    int    $0x80
=> 0xf7fdc430 <+16>:    pop    %ebp
   0xf7fdc431 <+17>:    pop    %edx
   0xf7fdc432 <+18>:    pop    %ecx
   0xf7fdc433 <+19>:    ret
End of assembler dump.

and as return PC has changed (by that 0x30-0x25 offset over those nops which
I do not understand why kernel does it all) GDB tried to relocate it back.
This created a very invalid address:

displaced: fixup (0xf7fdc425, 0x80483b2), insn = 0x0f 0x34 ...
displaced: relocated %eip from 0xf7fdc430 to 0xe7f704a3
0xe7f704a3 in ?? ()
Cannot access memory at address 0xe7f704a3

Therefore fixed GDB so that it recognizes also 'sysenter' (and put there also
'syscall' when at it).  Made the check more strict - 'int anything' ->
-> 'int 0x80'.  Are you aware of why it was not 0x80 before?


No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu
(kernel-3.2.6-3.fc16.x86_64) and for i686-fedora17-linux-gnu in KVM
(kernel-PAE-3.3.0-0.rc5.git0.3.fc18.i686).

Posting as a single patch as the testcase fix can create false regressions for
'git bisect', it is separate by file anyway.


Thanks,
Jan


gdb/
2012-02-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix disp-step-syscall.exp: fork: single step over fork.
	* i386-linux-tdep.c (-i386_linux_get_syscall_number): Rename to ...
	(i386_linux_get_syscall_number_from_regcache): ... here, change
	parameters gdbarch and ptid to regcache.  Remove parameter regcache,
	initialize gdbarch from regcache here.
	(i386_linux_get_syscall_number, i386_linux_displaced_step_copy_insn):
	New function.
	(i386_linux_init_abi): Install i386_linux_displaced_step_copy_insn
	instead.
	* i386-tdep.c (i386_syscall_p): Check also for 'sysenter' and
	'syscall'.  Make the 'int' check more strict.

gdb/testsuite/
2012-02-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix disp-step-syscall.exp: fork: single step over fork.
	* gdb.base/disp-step-syscall.exp (syscall_insn): Anchor it by
	whitespaces.
	(single step over $syscall): Remove its check.
	(single step over $syscall final pc): New check.

--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -492,10 +492,9 @@ i386_linux_record_signal (struct gdbarch *gdbarch,
 \f
 
 static LONGEST
-i386_linux_get_syscall_number (struct gdbarch *gdbarch,
-                               ptid_t ptid)
+i386_linux_get_syscall_number_from_regcache (struct regcache *regcache)
 {
-  struct regcache *regcache = get_thread_regcache (ptid);
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   /* The content of a register.  */
   gdb_byte buf[4];
@@ -512,6 +511,15 @@ i386_linux_get_syscall_number (struct gdbarch *gdbarch,
   return ret;
 }
 
+static LONGEST
+i386_linux_get_syscall_number (struct gdbarch *gdbarch,
+                               ptid_t ptid)
+{
+  struct regcache *regcache = get_thread_regcache (ptid);
+
+  return i386_linux_get_syscall_number_from_regcache (regcache);
+}
+
 /* The register sets used in GNU/Linux ELF core-dumps are identical to
    the register sets in `struct user' that are used for a.out
    core-dumps.  These are also used by ptrace(2).  The corresponding
@@ -643,6 +651,35 @@ i386_linux_core_read_description (struct gdbarch *gdbarch,
     return tdesc_i386_mmx_linux;
 }
 
+/* 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
+   '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'.  */
+
+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)
+    {
+      /* 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;
+}
+
 static void
 i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -890,7 +927,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);
+                                        i386_linux_displaced_step_copy_insn);
   set_gdbarch_displaced_step_fixup (gdbarch, i386_displaced_step_fixup);
   set_gdbarch_displaced_step_free_closure (gdbarch,
                                            simple_displaced_step_free_closure);
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -521,7 +521,12 @@ i386_call_p (const gdb_byte *insn)
 static int
 i386_syscall_p (const gdb_byte *insn, int *lengthp)
 {
-  if (insn[0] == 0xcd)
+  /* Is it 'int $0x80'?  */
+  if ((insn[0] == 0xcd && insn[1] == 0x80)
+      /* Or is it 'sysenter'?  */
+      || (insn[0] == 0x0f && insn[1] == 0x34)
+      /* Or is it 'syscall'?  */
+      || (insn[0] == 0x0f && insn[1] == 0x05))
     {
       *lengthp = 2;
       return 1;
--- 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
+	&& $syscall_insn_next_addr == $syscall_insn_next_addr_found} {
+      pass $test
+    } else {
+      fail $test
+    }
 
     # Delete breakpoint syscall insns to avoid interference to other syscalls.
     gdb_test_no_output "delete $syscall_insn_bp" "delete break $syscall insn"

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

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 5795 bytes --]

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 (齐尧)

[-- Attachment #2: 0001-i386_linux_displaced_step_fixup.patch --]
[-- Type: text/x-patch, Size: 2149 bytes --]


	* 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

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

[Jan Kratochvil]

On Tue, 28 Feb 2012 08:50:38 +0100, Yao Qi wrote:
> On 02/28/2012 03:22 AM, Jan Kratochvil wrote:
> > +   '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.

Yes, it is a hack.  I find the most correct way to have a flag in i386-linux
struct displaced_step_closure to mark it.  But we would have then to
complicate i386-linux-tdep by wrapping all the three displaced vector methods
in i386-linux-tdep which I did not find worth it.


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

Not in the first case.

0:int 0x80
2:ret

A:
PC == 0, orig_eax == -1, GDB in i386_displaced_step_copy_insn
PTRACE_SINGLESTEP, waitpid == SIGTRAP | PTRACE_EVENT_FORK
PC == 2, orig_eax == __NR_fork, GDB in i386_displaced_step_fixup

B:
PC == 2, orig_eax == __NR_fork, GDB in i386_displaced_step_copy_insn
PTRACE_SINGLESTEP, waitpid == SIGTRAP
PC == 2, orig_eax == -1, GDB in i386_displaced_step_fixup

C:
PC == 2, orig_eax == -1, initiate standard step over 'ret'
-> PC == return address

The extra flag in struct displaced_step_closure would remember in the B case
for i386_displaced_step_fixup that this step is special due to %orig_rax.
Without special i386_displaced_step_copy_insn the function
i386_displaced_step_fixup in the B case can no longer find anything unusual,
it can no longer see 'int $0x80' anywhere and %orig_eax is also normal.
Only PC did not change but that may happen for various regular instructions.


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

It works, it is a hack, I find your patch another kind of hack.


> 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 do not mind much but it makes some assumption if PC did not change it was by
a syscall without checking it really was a syscall at all.  There could be for
example some "jmp *%ebx" with %ebx == _start and it would be falsely relocated
by your patch back to its code location, ignoring its intended jump.  The
patch of mine would not relocate it as %orig_eax remained 0.

But any code messing with the entry point address may confuse this
autodetection anyway so these countercases are more hypothetical.

What do you think about the %orig_eax verification?

I do not mind in general, displaced stepping is not used by default yet.
I just had some kernel ptrace bug suspection and the FAIL was unstable while
regression testing across archs.


Regards,
Jan

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

[Yao Qi]

On 02/28/2012 04:40 PM, Jan Kratochvil wrote:
> I do not mind much but it makes some assumption if PC did not change it was by
> a syscall without checking it really was a syscall at all.  There could be for
> example some "jmp *%ebx" with %ebx == _start and it would be falsely relocated
> by your patch back to its code location, ignoring its intended jump.  The
> patch of mine would not relocate it as %orig_eax remained 0.
> 
> But any code messing with the entry point address may confuse this
> autodetection anyway so these countercases are more hypothetical.
> 
> What do you think about the %orig_eax verification?

It looks reasonable to me then, better than my approach.  It would be
better if we can add some comments to explain this fix is a hack and why
we have to do in this way.

-- 
Yao (齐尧)

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

[Jan Kratochvil]

On Tue, 28 Feb 2012 11:03:21 +0100, Yao Qi wrote:
> It would be better if we can add some comments to explain this fix is a hack
> and why we have to do in this way.

I hope it is OK this way.


Thanks,
Jan



gdb/
2012-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix disp-step-syscall.exp: fork: single step over fork.
	* i386-linux-tdep.c (-i386_linux_get_syscall_number): Rename to ...
	(i386_linux_get_syscall_number_from_regcache): ... here, new function
	comment, change parameters gdbarch and ptid to regcache.  Remove
	parameter regcache, initialize gdbarch from regcache here.
	(i386_linux_get_syscall_number, i386_linux_displaced_step_copy_insn):
	New functions.
	(i386_linux_init_abi): Install i386_linux_displaced_step_copy_insn
	instead.
	* i386-tdep.c (i386_syscall_p): Check also for 'sysenter' and
	'syscall'.  Make the 'int' check more strict.

gdb/testsuite/
2012-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix disp-step-syscall.exp: fork: single step over fork.
	* gdb.base/disp-step-syscall.exp (syscall_insn): Anchor it by
	whitespaces.
	(single step over $syscall): Remove its check.
	(single step over $syscall final pc): New check.

--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -491,11 +491,17 @@ i386_linux_record_signal (struct gdbarch *gdbarch,
 }
 \f
 
+/* Core of the implementation for gdbarch get_syscall_number.  Get pending
+   syscall number from REGCACHE.  If there is no pending syscall -1 will be
+   returned.  Pending syscall means ptrace has stepped into the syscall but
+   another ptrace call will step out.  PC is right after the int $0x80
+   / syscall / sysenter instruction in both cases, PC does not change during
+   the second ptrace step.  */
+
 static LONGEST
-i386_linux_get_syscall_number (struct gdbarch *gdbarch,
-                               ptid_t ptid)
+i386_linux_get_syscall_number_from_regcache (struct regcache *regcache)
 {
-  struct regcache *regcache = get_thread_regcache (ptid);
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   /* The content of a register.  */
   gdb_byte buf[4];
@@ -512,6 +518,18 @@ i386_linux_get_syscall_number (struct gdbarch *gdbarch,
   return ret;
 }
 
+/* Wrapper for i386_linux_get_syscall_number_from_regcache to make it
+   compatible with gdbarch get_syscall_number method prototype.  */
+
+static LONGEST
+i386_linux_get_syscall_number (struct gdbarch *gdbarch,
+                               ptid_t ptid)
+{
+  struct regcache *regcache = get_thread_regcache (ptid);
+
+  return i386_linux_get_syscall_number_from_regcache (regcache);
+}
+
 /* The register sets used in GNU/Linux ELF core-dumps are identical to
    the register sets in `struct user' that are used for a.out
    core-dumps.  These are also used by ptrace(2).  The corresponding
@@ -643,6 +661,49 @@ i386_linux_core_read_description (struct gdbarch *gdbarch,
     return tdesc_i386_mmx_linux;
 }
 
+/* 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 'int $0x80; ret' and during stepping out of the syscall
+   i386_displaced_step_fixup would keep PC at the displaced pad location.
+   As PC is pointing to the 'ret' instruction before the step
+   i386_displaced_step_fixup would expect inferior has just executed that 'ret'
+   and PC should not be adjusted.  In reality it finished syscall instead and
+   PC should get relocated back to its vDSO address.  Hide the 'ret'
+   instruction by 'nop' so that i386_displaced_step_fixup is not confused.
+   
+   It is not fully correct as the bytes in struct displaced_step_closure will
+   not match the inferior code.  But we would need some new flag in
+   displaced_step_closure otherwise to keep the state that syscall is finishing
+   for the later i386_displaced_step_fixup execution as the syscall execution
+   is already no longer detectable there.  The new flag field would mean
+   i386-linux-tdep.c needs to wrap all the displacement methods of i386-tdep.c
+   which does not seem worth it.  The same effect is achieved by patching that
+   'nop' instruction there instead.  */
+
+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)
+    {
+      /* 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;
+}
+
 static void
 i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -890,7 +951,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);
+                                        i386_linux_displaced_step_copy_insn);
   set_gdbarch_displaced_step_fixup (gdbarch, i386_displaced_step_fixup);
   set_gdbarch_displaced_step_free_closure (gdbarch,
                                            simple_displaced_step_free_closure);
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -521,7 +521,12 @@ i386_call_p (const gdb_byte *insn)
 static int
 i386_syscall_p (const gdb_byte *insn, int *lengthp)
 {
-  if (insn[0] == 0xcd)
+  /* Is it 'int $0x80'?  */
+  if ((insn[0] == 0xcd && insn[1] == 0x80)
+      /* Or is it 'sysenter'?  */
+      || (insn[0] == 0x0f && insn[1] == 0x34)
+      /* Or is it 'syscall'?  */
+      || (insn[0] == 0x0f && insn[1] == 0x05))
     {
       *lengthp = 2;
       return 1;
--- 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 +118,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
+	&& $syscall_insn_next_addr == $syscall_insn_next_addr_found} {
+      pass $test
+    } else {
+      fail $test
+    }
 
     # Delete breakpoint syscall insns to avoid interference to other syscalls.
     gdb_test_no_output "delete $syscall_insn_bp" "delete break $syscall insn"

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

[Yao Qi]

On 02/28/2012 09:42 PM, Jan Kratochvil wrote:
> +/* 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 'int $0x80; ret' and during stepping out of the syscall
> +   i386_displaced_step_fixup would keep PC at the displaced pad location.
> +   As PC is pointing to the 'ret' instruction before the step
> +   i386_displaced_step_fixup would expect inferior has just executed that 'ret'
> +   and PC should not be adjusted.  In reality it finished syscall instead and
> +   PC should get relocated back to its vDSO address.  Hide the 'ret'
> +   instruction by 'nop' so that i386_displaced_step_fixup is not confused.
> +   
> +   It is not fully correct as the bytes in struct displaced_step_closure will
> +   not match the inferior code.  But we would need some new flag in
> +   displaced_step_closure otherwise to keep the state that syscall is finishing
> +   for the later i386_displaced_step_fixup execution as the syscall execution
> +   is already no longer detectable there.  The new flag field would mean
> +   i386-linux-tdep.c needs to wrap all the displacement methods of i386-tdep.c
> +   which does not seem worth it.  The same effect is achieved by patching that
> +   'nop' instruction there instead.  */

These comments are clear.  I like them :)

-- 
Yao (齐尧)

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

[Pedro Alves]

On 02/28/2012 01:42 PM, Jan Kratochvil wrote:
> On Tue, 28 Feb 2012 11:03:21 +0100, Yao Qi wrote:
>> It would be better if we can add some comments to explain this fix is a hack
>> and why we have to do in this way.
> 
> I hope it is OK this way.

Looks good to me.

-- 
Pedro Alves

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

[Jan Kratochvil]

On Tue, 28 Feb 2012 14:55:28 +0100, Yao Qi wrote:
> These comments are clear.  I like them :)

On Tue, 28 Feb 2012 15:57:31 +0100, Pedro Alves wrote:
> Looks good to me.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-02/msg00202.html


Thanks,
Jan