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

Ulrich Weigand
2011-04-28T15:18:49Z

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;
+}