[patch, nios2] fix stepping past unwritable kernel helper

[patch, nios2] fix stepping past unwritable kernel helper

[Sandra Loosemore]

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

OK to commit the attached patch?

-Sandra

[-- Attachment #2: gdb-cmpxchg.patch --]
[-- Type: text/x-patch, Size: 8360 bytes --]

commit 3d57ff560dd3ecd40db70a548f158a05426f0ef7
Author: Sandra Loosemore <sandra@codesourcery.com>
Date:   Wed Mar 27 20:53:25 2019 -0700

    Fix stepping past unwritable kernel helper on nios2-linux-gnu.
    
    This patch fixes a problem on nios2-linux-gnu with stepping past the
    kernel helper __kuser_cmpxchg, which was exposed by the testcase
    gdb.threads/watchpoint-fork.exp.  The kernel maps this function into
    user space on an unwritable page.  In this testcase, the cmpxchg
    helper is invoked indirectly from the setbuf call in the test program.
    Since this target lacks hardware breakpoint/watchpoint support, GDB
    tries to single-step through the program by setting software
    breakpoints, and was just giving an error when it reached the function
    on the unwritable page.
    
    The solution here is to always step over the call instead of stepping
    into it; cmpxchg is supposed to be an atomic operation so this
    behavior seems reasonable.  The hook in nios2_get_next_pc is somewhat
    generic, but at present cmpxchg is the only helper provided by the
    Linux kernel that is invoked by an ordinary function call.  (Signal
    return trampolines also go through the unwritable page but not by a
    function call.)
    
    Fixing this issue also revealed that the testcase needs a much larger
    timeout factor when software single-stepping is used.  That has also
    been fixed in this patch.
    
    gdb/ChangeLog
    
    2019-03-27  Sandra Loosemore  <sandra@codesourcery.com>
    
    	* nios2-tdep.h (struct gdbarch_tdep): Add is_kernel_helper.
    	* nios2-tdep.c (nios2_get_next_pc): Skip over kernel helpers.
    	* nios2-linux-tdep.c (nios2_linux_is_kernel_helper): New.
    	(nios2_linux_init_abi): Install it.
    
    gdb/testsuite/ChangeLog
    
    2019-03-27  Sandra Loosemore  <sandra@codesourcery.com>
    
    	* gdb.threads/watchpoint-fork.exp (test): Use large timeout
    	factor when no hardware watchpoint support.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ebc2fd9..874f446 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-03-27  Sandra Loosemore  <sandra@codesourcery.com>
+
+	* nios2-tdep.h (struct gdbarch_tdep): Add is_kernel_helper.
+	* nios2-tdep.c (nios2_get_next_pc): Skip over kernel helpers.
+	* nios2-linux-tdep.c (nios2_linux_is_kernel_helper): New.
+	(nios2_linux_init_abi): Install it.
+
 2019-03-23  Tom Tromey  <tom@tromey.com>
 
 	* varobj.c (varobj_create): Update.
diff --git a/gdb/nios2-linux-tdep.c b/gdb/nios2-linux-tdep.c
index e4482d7..4702b6b 100644
--- a/gdb/nios2-linux-tdep.c
+++ b/gdb/nios2-linux-tdep.c
@@ -200,6 +200,17 @@ nios2_linux_syscall_next_pc (struct frame_info *frame,
   return pc + op->size;
 }
 
+/* Return true if PC is a kernel helper, a function mapped by the kernel
+   into user space on an unwritable page.  Currently the only such function
+   is __kuser_cmpxchg at 0x1004.  See arch/nios2/kernel/entry.S in the Linux
+   kernel sources and sysdeps/unix/sysv/linux/nios2/atomic-machine.h in
+   GLIBC.  */
+static int
+nios2_linux_is_kernel_helper (CORE_ADDR pc)
+{
+  return pc == 0x1004;
+}
+
 /* Hook function for gdbarch_register_osabi.  */
 
 static void
@@ -230,6 +241,7 @@ nios2_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 				  &nios2_r1_linux_rt_sigreturn_tramp_frame);
 
   tdep->syscall_next_pc = nios2_linux_syscall_next_pc;
+  tdep->is_kernel_helper = nios2_linux_is_kernel_helper;
 
   /* Index of target address word in glibc jmp_buf.  */
   tdep->jb_pc = 10;
diff --git a/gdb/nios2-tdep.c b/gdb/nios2-tdep.c
index ee45db9..0866454 100644
--- a/gdb/nios2-tdep.c
+++ b/gdb/nios2-tdep.c
@@ -2169,13 +2169,32 @@ nios2_get_next_pc (struct regcache *regcache, CORE_ADDR pc)
 	}
     }
 
-  else if (nios2_match_jmpi (insn, op, mach, &uimm)
-	   || nios2_match_calli (insn, op, mach, &uimm))
+  else if (nios2_match_jmpi (insn, op, mach, &uimm))
     pc = (pc & 0xf0000000) | uimm;
+  else if (nios2_match_calli (insn, op, mach, &uimm))
+    {
+      CORE_ADDR callto = (pc & 0xf0000000) | uimm;
+      if (tdep->is_kernel_helper != NULL
+	  && tdep->is_kernel_helper (callto))
+	/* Step over call to kernel helper, which we cannot debug
+	   from user space.  */
+	pc += op->size;
+      else
+	pc = callto;
+    }
 
-  else if (nios2_match_jmpr (insn, op, mach, &ra)
-	   || nios2_match_callr (insn, op, mach, &ra))
+  else if (nios2_match_jmpr (insn, op, mach, &ra))
     pc = regcache_raw_get_unsigned (regcache, ra);
+  else if (nios2_match_callr (insn, op, mach, &ra))
+    {
+      CORE_ADDR callto = regcache_raw_get_unsigned (regcache, ra);
+      if (tdep->is_kernel_helper != NULL
+	  && tdep->is_kernel_helper (callto))
+	/* Step over call to kernel helper.  */
+	pc += op->size;
+      else
+	pc = callto;
+    }
 
   else if (nios2_match_ldwm (insn, op, mach, &uimm, &ra, &imm, &wb, &id, &ret)
 	   && ret)
diff --git a/gdb/nios2-tdep.h b/gdb/nios2-tdep.h
index a2f0163..3524819 100644
--- a/gdb/nios2-tdep.h
+++ b/gdb/nios2-tdep.h
@@ -74,6 +74,9 @@ struct gdbarch_tdep
   CORE_ADDR (*syscall_next_pc) (struct frame_info *frame,
 				const struct nios2_opcode *op);
 
+  /* Returns true if PC points to a kernel helper function.  */
+  int (*is_kernel_helper) (CORE_ADDR pc);
+
   /* Offset to PC value in jump buffer.
      If this is negative, longjmp support will be disabled.  */
   int jb_pc;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 76ffd632..d651335 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-03-27  Sandra Loosemore  <sandra@codesourcery.com>
+
+	* gdb.threads/watchpoint-fork.exp (test): Use large timeout
+	factor when no hardware watchpoint support.
+
 2019-03-22  Alan Hayward  <alan.hayward@arm.com>
 
 	* README: Add pie options.
diff --git a/gdb/testsuite/gdb.threads/watchpoint-fork.exp b/gdb/testsuite/gdb.threads/watchpoint-fork.exp
index 878f784..28f3956 100644
--- a/gdb/testsuite/gdb.threads/watchpoint-fork.exp
+++ b/gdb/testsuite/gdb.threads/watchpoint-fork.exp
@@ -39,6 +39,12 @@ proc test {type symbol} {
 	    if [target_info exists gdb,no_hardware_watchpoints] {
 		# The software watchpoint functionality is in GDB an unrelated test.
 		gdb_test_no_output "set can-use-hw-watchpoints 0"
+		# Software watchpoints can be quite slow on remote targets
+		# on this test because it ends up single-stepping through
+		# code to initialize dynamic libraries, etc.  
+		set factor 20
+	    } else {
+		set factor 1
 	    }
 
 	    gdb_test "show detach-on-fork" "Whether gdb will detach the child of a fork is on\\."
@@ -63,19 +69,21 @@ proc test {type symbol} {
 
 	    gdb_breakpoint "mark_exit"
 
-	    gdb_test "continue" \
-		"reakpoint \[0-9\]+, marker.*" "hardware breakpoints work"
-	    gdb_test "continue" \
-		"atchpoint \[0-9\]+: var.*Old value = 0.*New value = 1.*forkoff *\\(1\\).*" "watchpoints work"
-	    gdb_test "continue" \
-		"reakpoint \[0-9\]+, marker.*" "breakpoint after the first fork"
-	    gdb_test "continue" \
-		"atchpoint \[0-9\]+: var.*Old value = 1.*New value = 2.*forkoff *\\(2\\).*" "watchpoint after the first fork"
-	    gdb_test "continue" \
-		"reakpoint \[0-9\]+, marker.*" "breakpoint after the second fork"
-	    gdb_test "continue" \
-		"atchpoint \[0-9\]+: var.*Old value = 2.*New value = 3.*mark_exit \\(\\);" "watchpoint after the second fork"
-	    gdb_test "continue" "Continuing\\..*\r\n(Thread .* hit )?Breakpoint \[0-9\]+, mark_exit .*" "finish"
+	    with_timeout_factor $factor {
+		gdb_test "continue" \
+		    "reakpoint \[0-9\]+, marker.*" "hardware breakpoints work"
+		gdb_test "continue" \
+		    "atchpoint \[0-9\]+: var.*Old value = 0.*New value = 1.*forkoff *\\(1\\).*" "watchpoints work"
+		gdb_test "continue" \
+		    "reakpoint \[0-9\]+, marker.*" "breakpoint after the first fork"
+		gdb_test "continue" \
+		    "atchpoint \[0-9\]+: var.*Old value = 1.*New value = 2.*forkoff *\\(2\\).*" "watchpoint after the first fork"
+		gdb_test "continue" \
+		    "reakpoint \[0-9\]+, marker.*" "breakpoint after the second fork"
+		gdb_test "continue" \
+		    "atchpoint \[0-9\]+: var.*Old value = 2.*New value = 3.*mark_exit \\(\\);" "watchpoint after the second fork"
+		gdb_test "continue" "Continuing\\..*\r\n(Thread .* hit )?Breakpoint \[0-9\]+, mark_exit .*" "finish"
+	    }
 	}
 
 	# threads

Re: [patch, nios2] fix stepping past unwritable kernel helper

[Kevin Buettner]

On Wed, 27 Mar 2019 22:47:44 -0600
Sandra Loosemore <sandra@codesourcery.com> wrote:

>     Fix stepping past unwritable kernel helper on nios2-linux-gnu.
>     
>     This patch fixes a problem on nios2-linux-gnu with stepping past the
>     kernel helper __kuser_cmpxchg, which was exposed by the testcase
>     gdb.threads/watchpoint-fork.exp.  The kernel maps this function into
>     user space on an unwritable page.  In this testcase, the cmpxchg
>     helper is invoked indirectly from the setbuf call in the test program.
>     Since this target lacks hardware breakpoint/watchpoint support, GDB
>     tries to single-step through the program by setting software
>     breakpoints, and was just giving an error when it reached the function
>     on the unwritable page.
>     
>     The solution here is to always step over the call instead of stepping
>     into it; cmpxchg is supposed to be an atomic operation so this
>     behavior seems reasonable.  The hook in nios2_get_next_pc is somewhat
>     generic, but at present cmpxchg is the only helper provided by the
>     Linux kernel that is invoked by an ordinary function call.  (Signal
>     return trampolines also go through the unwritable page but not by a
>     function call.)
>     
>     Fixing this issue also revealed that the testcase needs a much larger
>     timeout factor when software single-stepping is used.  That has also
>     been fixed in this patch.
>     
>     gdb/ChangeLog
>     
>     2019-03-27  Sandra Loosemore  <sandra@codesourcery.com>
>     
>     	* nios2-tdep.h (struct gdbarch_tdep): Add is_kernel_helper.
>     	* nios2-tdep.c (nios2_get_next_pc): Skip over kernel helpers.
>     	* nios2-linux-tdep.c (nios2_linux_is_kernel_helper): New.
>     	(nios2_linux_init_abi): Install it.
>     
>     gdb/testsuite/ChangeLog
>     
>     2019-03-27  Sandra Loosemore  <sandra@codesourcery.com>
>     
>     	* gdb.threads/watchpoint-fork.exp (test): Use large timeout
>     	factor when no hardware watchpoint support.

Thanks for the explanation, above.

The one thing that concerned me when I first saw it was the use of the
constant 0x1004 as the address for __kuser_cmpxchg.  But after checking
the glibc sources at the path indicated in the comment, I saw that it's
hardcoded using the same constant there as well.

So... this patch is okay.

Kevin

Re: [patch, nios2] fix stepping past unwritable kernel helper

[Sandra Loosemore]

On 3/28/19 5:39 AM, Pedro Alves wrote:
> On 03/28/2019 04:47 AM, Sandra Loosemore wrote:
>> +static int
>> +nios2_linux_is_kernel_helper (CORE_ADDR pc)
>> +{
>> +  return pc == 0x1004;
>> +}
>> +
> 
>> +  /* Returns true if PC points to a kernel helper function.  */
>> +  int (*is_kernel_helper) (CORE_ADDR pc);
> 
> Nit: please make these return bool instead of int.

Thanks for the speedy review.  I've pushed the patch with that tweak as
commit f489207efde922e436b1b420d4de071927e3b9d5.

-Sandra