This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[patch v2] Re: [commit] Re: [rfc][1/2] Signal delivery + software single-step is broken


Pedro Alves wrote:

> (I wish there was no need to undo stuff, and do the decision
> before actually inserting sss breakpoints, but I see why you
> did it.  We need to call gdbarch_software_single_step to
> know whether sss breakpoints will be inserted, which, actually
> inserts them.)

Fully agreed ...

> I'm trying to think whether this works okay with nested
> signals.
[...]
> Since you'll already have one step-resume breakpoint set (back in the
> main code), and you can't set another -- you'd hit the
> assert in insert_step_resume_breakpoint_at_sal trying to insert
> a second one.

Hmm, good point.  Indeed I was able to trigger that assert.

However, this is easily fixable just the same as in handle_inferior_event:
if we already have a step-resume breakpoint, we don't need to insert
another one, but just continue running until we hit the original one.

Fixed in the patch below, together with a test case that triggers
the assert with the original patch.

Retested on armv7l-unknown-linux-gnueabi and i386-linux.

Does this look OK to you?

Thanks,
Ulrich


ChangeLog:

	gdb/
	* infrun.c (proceed): Revert previous change.
	(resume): Instead, handle the case of signal delivery while stepping
	off a breakpoint location here, and only if software single-stepping
	is used.  Handle nested signals.

	gdb/testsuite/
	* gdb.base/signest.exp: New file.
	* gdb.base/signest.c: Likewise.


Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.476
diff -u -p -r1.476 infrun.c
--- gdb/infrun.c	27 Apr 2011 17:08:41 -0000	1.476
+++ gdb/infrun.c	28 Apr 2011 14:24:44 -0000
@@ -1703,6 +1703,51 @@ a command like `return' or `jump' to con
   else if (step)
     step = maybe_software_singlestep (gdbarch, pc);
 
+  /* Currently, our software single-step implementation leads to different
+     results than hardware single-stepping in one situation: when stepping
+     into delivering a signal which has an associated signal handler,
+     hardware single-step will stop at the first instruction of the handler,
+     while software single-step will simply skip execution of the handler.
+
+     For now, this difference in behavior is accepted since there is no
+     easy way to actually implement single-stepping into a signal handler
+     without kernel support.
+
+     However, there is one scenario where this difference leads to follow-on
+     problems: if we're stepping off a breakpoint by removing all breakpoints
+     and then single-stepping.  In this case, the software single-step
+     behavior means that even if there is a *breakpoint* in the signal
+     handler, GDB still would not stop.
+
+     Fortunately, we can at least fix this particular issue.  We detect
+     here the case where we are about to deliver a signal while software
+     single-stepping with breakpoints removed.  In this situation, we
+     revert the decisions to remove all breakpoints and insert single-
+     step breakpoints, and instead we install a step-resume breakpoint
+     at the current address, deliver the signal without stepping, and
+     once we arrive back at the step-resume breakpoint, actually step
+     over the breakpoint we originally wanted to step over.  */
+  if (singlestep_breakpoints_inserted_p
+      && tp->control.trap_expected && sig != TARGET_SIGNAL_0)
+    {
+      /* If we have nested signals or a pending signal is delivered
+	 immediately after a handler returns, might might already have
+	 a step-resume breakpoint set on the earlier handler.  We cannot
+	 set another step-resume breakpoint; just continue on until the
+	 original breakpoint is hit.  */
+      if (tp->control.step_resume_breakpoint == NULL)
+	{
+	  insert_step_resume_breakpoint_at_frame (get_current_frame ());
+	  tp->step_after_step_resume_breakpoint = 1;
+	}
+
+      remove_single_step_breakpoints ();
+      singlestep_breakpoints_inserted_p = 0;
+
+      insert_breakpoints ();
+      tp->control.trap_expected = 0;
+    }
+
   if (should_resume)
     {
       ptid_t resume_ptid;
@@ -2064,6 +2109,24 @@ proceed (CORE_ADDR addr, enum target_sig
   /* prepare_to_proceed may change the current thread.  */
   tp = inferior_thread ();
 
+  if (oneproc)
+    {
+      tp->control.trap_expected = 1;
+      /* If displaced stepping is enabled, we can step over the
+	 breakpoint without hitting it, so leave all breakpoints
+	 inserted.  Otherwise we need to disable all breakpoints, step
+	 one instruction, and then re-add them when that step is
+	 finished.  */
+      if (!use_displaced_stepping (gdbarch))
+	remove_breakpoints ();
+    }
+
+  /* We can insert breakpoints if we're not trying to step over one,
+     or if we are stepping over one but we're using displaced stepping
+     to do so.  */
+  if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
+    insert_breakpoints ();
+
   if (!non_stop)
     {
       /* Pass the last stop signal to the thread we're resuming,
@@ -2133,42 +2196,6 @@ proceed (CORE_ADDR addr, enum target_sig
   /* Reset to normal state.  */
   init_infwait_state ();
 
-  /* Stepping over a breakpoint while at the same time delivering a signal
-     has a problem: we cannot use displaced stepping, but we also cannot
-     use software single-stepping, because we do not know where execution
-     will continue if a signal handler is installed.
-
-     On the other hand, if there is a signal handler we'd have to step
-     over it anyway.  So what we do instead is to install a step-resume
-     handler at the current address right away, deliver the signal without
-     stepping, and once we arrive back at the step-resume breakpoint, step
-     once more over the original breakpoint we wanted to step over.  */
-  if (oneproc && tp->suspend.stop_signal != TARGET_SIGNAL_0
-      && execution_direction != EXEC_REVERSE)
-    {
-      insert_step_resume_breakpoint_at_frame (get_current_frame ());
-      tp->step_after_step_resume_breakpoint = 1;
-      oneproc = 0;
-    }
-
-  if (oneproc)
-    {
-      tp->control.trap_expected = 1;
-      /* If displaced stepping is enabled, we can step over the
-	 breakpoint without hitting it, so leave all breakpoints
-	 inserted.  Otherwise we need to disable all breakpoints, step
-	 one instruction, and then re-add them when that step is
-	 finished.  */
-      if (!use_displaced_stepping (gdbarch))
-	remove_breakpoints ();
-    }
-
-  /* We can insert breakpoints if we're not trying to step over one,
-     or if we are stepping over one but we're using displaced stepping
-     to do so.  */
-  if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
-    insert_breakpoints ();
-
   /* Resume inferior.  */
   resume (oneproc || step || bpstat_should_step (), tp->suspend.stop_signal);
 
--- /dev/null	2011-04-28 13:38:53.488264001 +0200
+++ gdb/testsuite/gdb.base/signest.exp	2011-04-28 16:15:16.000000000 +0200
@@ -0,0 +1,67 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile "signest"
+set srcfile ${testfile}.c
+
+if [target_info exists gdb,nosignals] {
+    verbose "Skipping ${testfile}.exp because of nosignals."
+    return -1
+}
+
+if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug}] {
+    untested ${testfile}.exp
+    return -1
+}
+
+if ![runto_main] then {
+    untested ${testfile}.exp
+    return -1
+}
+
+# If we can examine what's at memory address 0, it is possible that we
+# could also execute it.  This could probably make us run away,
+# executing random code, which could have all sorts of ill effects,
+# especially on targets without an MMU.  Don't run the tests in that
+# case.
+
+gdb_test_multiple "x 0" "memory at address 0" {
+    -re "0x0:.*Cannot access memory at address 0x0.*$gdb_prompt $" { }
+    -re "0x0:.*Error accessing memory address 0x0.*$gdb_prompt $" { }
+    -re ".*$gdb_prompt $" {
+	untested "Memory at address 0 is possibly executable"
+	return -1
+    }
+}
+
+# Run until we hit the SIGSEGV (or SIGBUS on some platforms).
+gdb_test "continue" \
+	 ".*Program received signal (SIGBUS|SIGSEGV).*bowler.*" \
+         "continue to fault"
+
+# Insert conditional breakpoint at faulting instruction
+gdb_test "break if 0" ".*" "set conditional breakpoint"
+
+# Set SIGSEGV/SIGBUS to pass+nostop
+gdb_test "handle SIGSEGV nostop print pass" ".*" "pass SIGSEGV"
+gdb_test "handle SIGBUS nostop print pass" ".*" "pass SIGBUS"
+
+# Step off the faulting instruction into the handler, triggering nested faults
+gdb_test "continue" \
+         ".*Program received signal (SIGBUS|SIGSEGV).*Program received signal (SIGBUS|SIGSEGV).*exited normally.*" \
+	 "run through nested faults"
+
--- /dev/null	2011-04-28 13:38:53.488264001 +0200
+++ gdb/testsuite/gdb.base/signest.c	2011-04-28 16:04:52.000000000 +0200
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+volatile char *p = NULL;
+
+extern long
+bowler (void)
+{
+  return *p;
+}
+
+extern void
+keeper (int sig)
+{
+  static int recurse = 0;
+  if (++recurse < 3)
+    bowler ();
+
+  _exit (0);
+}
+
+int
+main (void)
+{
+  struct sigaction act;
+  memset (&act, 0, sizeof act);
+  act.sa_handler = keeper;
+  act.sa_flags = SA_NODEFER;
+  sigaction (SIGSEGV, &act, NULL);
+  sigaction (SIGBUS, &act, NULL);
+
+  bowler ();
+  return 0;
+}

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]