stepi/nexti: skip signal handler if "handle nostop" signal arrives

Message ID 544EA096.5050803@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Oct. 27, 2014, 7:44 p.m. UTC
  On 10/27/2014 05:38 PM, Eli Zaretskii wrote:
>> Date: Sat, 25 Oct 2014 15:00:40 +0100
>> From: Pedro Alves <palves@redhat.com>

> Thanks, this is a huge improvement.  I have only a couple of minor
> stylistic suggestions:
> 

Thanks.

>> +@cindex stepping and signal handlers
>> +@anchor{stepping and signal handlers}
>> +
>> +@value{GDBN} optimizes for stepping the mainline code.  If a signal
>> +that has @code{handle nostop} and @code{handle pass} set arrives while
>> +a stepping command (e.g., @code{stepi}, @code{step}, @code{next}) is
>> +in progress, @value{GDBN} lets the signal handler run and then resumes
>> +stepping the mainline code once the signal handler returns.  In other
>> +words, @value{GDBN} steps over the signal handler.  If the signal has
>> +@code{handle noprint} set, then you won't even hear about it.  This
>> +prevents signals that you've specified as not interesting (with
> 
> I would suggest to use a semi-colon, not a period, before the last
> "This".  That's because the last sentence is logically an immediate
> continuation of the one before it.  By putting a full stop between
> them we create a potential for misunderstanding to what "this" refers,
> since the previous text described 2 different situations.  Using a
> semi-colon removes that danger.

Hmm, I don't think I agree with that.  I'm not really trying to
describ 2 situations.  The "this" refers to GDB stepping over the
signal handler.  The noprint issue is secondary here, and I
guess an example shows better what I'm talking about:

(gdb) list 28
28        p = 0;
29        p = 0;
30        p = 0;
(gdb) n
28        p = 0;
(gdb) info inferiors 
  Num  Description       Executable        
* 1    process 25468     /home/pedro/gdb/tests/signal
(gdb) shell kill -SIGUSR1 25468
(gdb) handle SIGUSR1 nostop pass print
Signal        Stop      Print   Pass to program Description
SIGUSR1       No        Yes     Yes             User defined signal 1
(gdb) si

Program received signal SIGUSR1, User defined signal 1.
29        p = 0;
(gdb) 

That stepped over the signal handler, from line 28 to 29, but we
still heard about the signal.

Vs:

(gdb) 
28        p = 0;
(gdb) handle SIGUSR1 nostop noprint pass
Signal        Stop      Print   Pass to program Description
SIGUSR1       No        No      Yes             User defined signal 1
(gdb) info inferiors 
  Num  Description       Executable        
* 1    process 25484     /home/pedro/gdb/tests/signal
(gdb) shell kill -SIGUSR1 25484
(gdb) si
29        p = 0;
(gdb) 

That stepped over the signal handler, from line 28 to 29, and we
didn't even hear about the signal.

So, perhaps this variant is clearer?

@value{GDBN} optimizes for stepping the mainline code.  If a signal
that has @code{handle nostop} and @code{handle pass} set arrives while
a stepping command (e.g., @code{stepi}, @code{step}, @code{next}) is
in progress, @value{GDBN} lets the signal handler run and then resumes
stepping the mainline code once the signal handler returns.  In other
words, @value{GDBN} steps over the signal handler.  This prevents
signals that you've specified as not interesting (with @code{handle
nostop}) from changing the focus of debugging unexpectedly.  Note that
the signal handler itself may still hit a breakpoint, stop for another
signal that has @code{handle stop} in effect, or for any other event
that normally results in stopping the stepping command sooner.  Also
note that @value{GDBN} still informs you that the program received a
signal if @code{handle print} is set.


> 
> For the same reason, it might be better to make "If the signal has
> 'handle noprint' ..." start a new paragraph.

Yes, though perhaps a new paragraph is unnecessary.  See above.

> 
>> +@cindex stepping into signal handlers
>> +@anchor{stepping into signal handlers}
> 
> I would remove this @cindex entry: it doesn't add anything useful to
> the previous one, and will likely point to the same page.

I'd prefer to keep it, if you don't mind.  The queue-signal reference wants
to point here directly, and I can imagine the text above expanding in
directions not relevant for that cross reference.  I'd like to
have a place where I can point at when the topic of wanting to debug
a handler without knowing exactly which function that is comes up.

> 
>> +If the program was stopped for a signal (that is, stopped before the
>> +program sees it), due to @code{handle stop} being set, and
>> +@code{handle pass} is in effect for that signal too, and your program
>> +handles the signal, a stepping command such as for example
>> +@code{stepi} or @code{step} steps @emph{into} the signal's handler (if
>> +the target supports it).
> 
> This is a mouthful, not in the least because of excessive use of past
> tense.  How about this variant instead:
> 
>   If you set @code{handle stop} for a signal, @value{GDBN} stops your
>   program and announces the signal when it arrives, before the program
>   sees it.  

I think this part ends up being redundant with what is already
said further above, around:

"When a signal stops your program, the signal is not visible to the
program until you
continue.  Your program sees the signal then, if @code{pass} is in
effect for the signal in question @emph{at that time}. "

I'm now thinking that we can just remove that part, and use the rest of
your paragraph below:

>   If you also set @code{handle pass} for that signal, and
>   your program sets up a handler for it, then issuing a stepping
>   command, such as @code{step} or @code{stepi}, when your program is
>   stopped due to the signal will step @emph{into} the signal handler
>   (if the target supports that).

Sounds clear enough to me (with a minor tweak to make it stand on
its own).

> 
>> +Likewise, if the @code{queue-signal} command was used to queue a
>> +signal to be delivered to the current thread when execution of the
> 
> Please reword in active tens ("... if you use the @code{queue-signal}
> command to queue ...").
> 

Done.

>> +thread resumes (@pxref{Signaling, ,Giving your Program a Signal}),
>> +then a stepping command steps into the signal's handler.
> 
> Not sure I understand the sequence here.  First, I queue-signal, then
> the signal is delivered and the thread stops, and _then_ I issue si?
> I guess the "when execution of the thread resumes" confused me.

Sounds like you're thinking of "queue-signal" like "kill" from the shell,
but that's not how "queue-signal" works.  "queue-signal" instead
passes the signal to the program immediately as if the thread had
_already_ stopped for the signal.  GDB doesn't intercept
the signal anymore.

I've added a queue-signal example.  Hopefully that makes things clearer.

Let me know how this version looks.

From fd292ad2a1c10ec5a4f902535c7d3c29d9cbc1e1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 27 Oct 2014 19:27:10 +0000
Subject: [PATCH] stepi/nexti: skip signal handler if "handle nostop" signal
 arrives

I noticed that "si" behaves differently when a "handle nostop" signal
arrives while the step is in progress, depending on whether the
program was stopped at a breakpoint when "si" was entered.
Specifically, in case GDB needs to step off a breakpoint, the handler
is skipped and the program stops in the next "mainline" instruction.
Otherwise, the "si" stops in the first instruction of the signal
handler.

I was surprised the testsuite doesn't catch this difference.  Turns
out gdb.base/sigstep.exp covers a bunch of cases related to stepping
and signal handlers, but does not test stepi nor nexti, only
step/next/continue.

My first reaction was that stopping in the signal handler was the
correct thing to do, as it's where the next user-visible instruction
that is executed is.  I considered then "nexti" -- a signal handler
could be reasonably considered a subroutine call to step over, it'd
seem intuitive to me that "nexti" would skip it.

But then, I realized that signals that arrive while a plain/line
"step" is in progress _also_ have their handler skipped.  A user might
well be excused for being confused by this, given:

  (gdb) help step
  Step program until it reaches a different source line.

And the signal handler's sources will be in different source lines,
after all.

I think that having to explain that "stepi" steps into handlers, (and
that "nexti" wouldn't according to my reasoning above), while "step"
does not, is a sign of an awkward interface.

E.g., if a user truly is interested in stepping into signal handlers,
then it's odd that she has to either force the signal to "handle
stop", or recall to do "stepi" whenever such a signal might be
delivered.  For that use case, it'd seem nicer to me if "step" also
stepped into handlers.

This suggests to me that we either need a global "step-into-handlers"
setting, or perhaps better, make "handle pass/nopass stop/nostop
print/noprint" have have an additional axis - "handle
stepinto/nostepinto", so that the user could configure whether
handlers for specific signals should be stepped into.

In any case, I think it's simpler (and thus better) for all step
commands to behave the same.  This commit thus makes "si/ni" skip
handlers for "handle nostop" signals that arrive while the command was
already in progress, like step/next do.

To be clear, nothing changes if the program was stopped for a signal,
and the user enters a stepping command _then_ -- GDB still steps into
the handler.  The change concerns signals that don't cause a stop and
that arrive while the step is in progress.

Tested on x86_64 Fedora 20, native and gdbserver.

gdb/
2014-10-27  Pedro Alves  <palves@redhat.com>

	* infrun.c (handle_signal_stop): Also skip handlers when a random
	signal arrives while handling a "stepi" or a "nexti".  Set the
	thread's 'step_after_step_resume_breakpoint' flag.

gdb/doc/
2014-10-27  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Continuing and Stepping): Add cross reference to
	info on stepping and signal handlers.
	(Signals): Explain stepping and signal handlers.  Add context
	index entries, and cross references.

gdb/testsuite/
2014-10-27  Pedro Alves  <palves@redhat.com>

	* gdb.base/sigstep.c (dummy): New global.
	(main): Issue a couple writes to the new global.
	* gdb.base/sigstep.exp (get_next_pc, test_skip_handler): New
	procedures.
	(skip_over_handler): Use test_skip_handler.
	(top level): Call skip_over_handler for stepi and nexti too.
	(breakpoint_over_handler): Use test_skip_handler.
	(top level): Call breakpoint_over_handler for stepi and nexti too.
---
 gdb/doc/gdb.texinfo                | 68 +++++++++++++++++++++++++++++++++++++-
 gdb/infrun.c                       |  7 ++--
 gdb/testsuite/gdb.base/sigstep.c   |  7 ++--
 gdb/testsuite/gdb.base/sigstep.exp | 55 ++++++++++++++++++++++--------
 4 files changed, 118 insertions(+), 19 deletions(-)
  

Comments

Eli Zaretskii Oct. 27, 2014, 7:58 p.m. UTC | #1
> Date: Mon, 27 Oct 2014 19:44:22 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> So, perhaps this variant is clearer?
> 
> @value{GDBN} optimizes for stepping the mainline code.  If a signal
> that has @code{handle nostop} and @code{handle pass} set arrives while
> a stepping command (e.g., @code{stepi}, @code{step}, @code{next}) is
> in progress, @value{GDBN} lets the signal handler run and then resumes
> stepping the mainline code once the signal handler returns.  In other
> words, @value{GDBN} steps over the signal handler.  This prevents
> signals that you've specified as not interesting (with @code{handle
> nostop}) from changing the focus of debugging unexpectedly.  Note that
> the signal handler itself may still hit a breakpoint, stop for another
> signal that has @code{handle stop} in effect, or for any other event
> that normally results in stopping the stepping command sooner.  Also
> note that @value{GDBN} still informs you that the program received a
> signal if @code{handle print} is set.

Yes, this is OK.

> >> +@cindex stepping into signal handlers
> >> +@anchor{stepping into signal handlers}
> > 
> > I would remove this @cindex entry: it doesn't add anything useful to
> > the previous one, and will likely point to the same page.
> 
> I'd prefer to keep it, if you don't mind.  The queue-signal reference wants
> to point here directly, and I can imagine the text above expanding in
> directions not relevant for that cross reference.

Both entries use the same words, so why do we need both?  There's one
paragraph of only a dozen of text lines between them.

> I'd like to have a place where I can point at when the topic of
> wanting to debug a handler without knowing exactly which function
> that is comes up.

For pointing, you have the anchor; I didn't say to delete that.  Index
entries cannot point.  In fact, some Info readers land you at the
beginning of the node, not at the place where the entry was in
Texinfo.

> I'm now thinking that we can just remove that part, and use the rest of
> your paragraph below:
> 
> >   If you also set @code{handle pass} for that signal, and
> >   your program sets up a handler for it, then issuing a stepping
> >   command, such as @code{step} or @code{stepi}, when your program is
> >   stopped due to the signal will step @emph{into} the signal handler
> >   (if the target supports that).

Fine with me.

> >> +thread resumes (@pxref{Signaling, ,Giving your Program a Signal}),
> >> +then a stepping command steps into the signal's handler.
> > 
> > Not sure I understand the sequence here.  First, I queue-signal, then
> > the signal is delivered and the thread stops, and _then_ I issue si?
> > I guess the "when execution of the thread resumes" confused me.
> 
> Sounds like you're thinking of "queue-signal" like "kill" from the shell,
> but that's not how "queue-signal" works.  "queue-signal" instead
> passes the signal to the program immediately as if the thread had
> _already_ stopped for the signal.  GDB doesn't intercept
> the signal anymore.

How is this different from what I wrote?  The program behaves as if
the signal was delivered to it, right?

> I've added a queue-signal example.  Hopefully that makes things clearer.

It does, thanks.

> Let me know how this version looks.

LGTM.
  

Patch

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a1b8ac7..9b32217 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -5079,7 +5079,9 @@  line of source code, or one machine instruction (depending on what
 particular command you use).  Either when continuing or when stepping,
 your program may stop even sooner, due to a breakpoint or a signal.  (If
 it stops due to a signal, you may want to use @code{handle}, or use
-@samp{signal 0} to resume execution.  @xref{Signals, ,Signals}.)
+@samp{signal 0} to resume execution (@pxref{Signals, ,Signals}),
+or you may step into the signal's handler (@pxref{stepping and signal
+handlers}).)
 
 @table @code
 @kindex continue
@@ -5573,6 +5575,67 @@  a result of the fatal signal once it saw the signal.  To prevent this,
 you can continue with @samp{signal 0}.  @xref{Signaling, ,Giving your
 Program a Signal}.
 
+@cindex stepping and signal handlers
+@anchor{stepping and signal handlers}
+
+@value{GDBN} optimizes for stepping the mainline code.  If a signal
+that has @code{handle nostop} and @code{handle pass} set arrives while
+a stepping command (e.g., @code{stepi}, @code{step}, @code{next}) is
+in progress, @value{GDBN} lets the signal handler run and then resumes
+stepping the mainline code once the signal handler returns.  In other
+words, @value{GDBN} steps over the signal handler.  This prevents
+signals that you've specified as not interesting (with @code{handle
+nostop}) from changing the focus of debugging unexpectedly.  Note that
+the signal handler itself may still hit a breakpoint, stop for another
+signal that has @code{handle stop} in effect, or for any other event
+that normally results in stopping the stepping command sooner.  Also
+note that @value{GDBN} still informs you that the program received a
+signal if @code{handle print} is set.
+
+@cindex stepping into signal handlers
+@anchor{stepping into signal handlers}
+
+If you set @code{handle pass} for a signal, and your program sets up a
+handler for it, then issuing a stepping command, such as @code{step}
+or @code{stepi}, when your program is stopped due to the signal will
+step @emph{into} the signal handler (if the target supports that).
+
+Likewise, if you use the @code{queue-signal} command to queue a signal
+to be delivered to the current thread when execution of the thread
+resumes (@pxref{Signaling, ,Giving your Program a Signal}), then a
+stepping command will step into the signal handler.
+
+Here's an example, using @code{stepi} to step to the first instruction
+of @code{SIGUSR1}'s handler:
+
+@smallexample
+(@value{GDBP}) handle SIGUSR1
+Signal        Stop      Print   Pass to program Description
+SIGUSR1       Yes       Yes     Yes             User defined signal 1
+(@value{GDBP}) c
+Continuing.
+
+Program received signal SIGUSR1, User defined signal 1.
+main () sigusr1.c:28
+28        p = 0;
+(@value{GDBP}) si
+sigusr1_handler () at sigusr1.c:9
+9       @{
+@end smallexample
+
+The same, but using @code{queue-signal} instead of waiting for the
+program to receive the signal first:
+
+@smallexample
+(@value{GDBP}) n
+28        p = 0;
+(@value{GDBP}) queue-signal SIGUSR1
+(@value{GDBP}) si
+sigusr1_handler () at sigusr1.c:9
+9       @{
+(@value{GDBP})
+@end smallexample
+
 @cindex extra signal information
 @anchor{extra signal information}
 
@@ -16654,6 +16717,9 @@  be used to pass a signal whose handling state has been set to @code{nopass}
 @end table
 @c @end group
 
+@xref{stepping into signal handlers}, for information on how stepping
+commands behave when the thread has a signal queued.
+
 @node Returning
 @section Returning from a Function
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 90a3123..df053e2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4463,9 +4463,9 @@  handle_signal_stop (struct execution_control_state *ecs)
 	  return;
 	}
 
-      if (ecs->event_thread->control.step_range_end != 0
-	  && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_0
-	  && pc_in_thread_step_range (stop_pc, ecs->event_thread)
+      if (ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_0
+	  && (pc_in_thread_step_range (stop_pc, ecs->event_thread)
+	      || ecs->event_thread->control.step_range_end == 1)
 	  && frame_id_eq (get_stack_frame_id (frame),
 			  ecs->event_thread->control.step_stack_frame_id)
 	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
@@ -4485,6 +4485,7 @@  handle_signal_stop (struct execution_control_state *ecs)
                                 "single-step range\n");
 
 	  insert_hp_step_resume_breakpoint_at_frame (frame);
+	  ecs->event_thread->step_after_step_resume_breakpoint = 1;
 	  /* Reset trap_expected to ensure breakpoints are re-inserted.  */
 	  ecs->event_thread->control.trap_expected = 0;
 	  keep_going (ecs);
diff --git a/gdb/testsuite/gdb.base/sigstep.c b/gdb/testsuite/gdb.base/sigstep.c
index aa2384a..cc69184 100644
--- a/gdb/testsuite/gdb.base/sigstep.c
+++ b/gdb/testsuite/gdb.base/sigstep.c
@@ -24,6 +24,7 @@ 
 #include <errno.h>
 
 static volatile int done;
+static volatile int dummy;
 
 static void
 handler (int sig)
@@ -74,8 +75,10 @@  main ()
 	      return 1;
 	    }
 	}
-      /* Wait.  */
-      while (!done);
+      /* Wait.  Issue a couple writes to a dummy volatile var to be
+	 reasonably sure our simple "get-next-pc" logic doesn't
+	 stumble on branches.  */
+      dummy = 0; dummy = 0; while (!done);
       done = 0;
     }
   return 0;
diff --git a/gdb/testsuite/gdb.base/sigstep.exp b/gdb/testsuite/gdb.base/sigstep.exp
index 184d46e..53152b8 100644
--- a/gdb/testsuite/gdb.base/sigstep.exp
+++ b/gdb/testsuite/gdb.base/sigstep.exp
@@ -269,9 +269,38 @@  proc skip_to_handler_entry { i } {
     gdb_test "clear *handler" ".*" "$prefix; clear handler"
 }
 
-skip_to_handler_entry step
-skip_to_handler_entry next
-skip_to_handler_entry continue
+foreach cmd {"stepi" "nexti" "step" "next" "continue"} {
+    skip_to_handler_entry $cmd
+}
+
+# Get the address of where a single-step should land.
+
+proc get_next_pc {test} {
+    global gdb_prompt
+    global hex
+
+    set next ""
+    gdb_test_multiple "x/2i \$pc" $test {
+	-re "$hex .*:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
+	    set next $expect_out(1,string)
+	    pass $test
+	}
+    }
+
+    return $next
+}
+
+# Test that the command skipped over the handler.
+
+proc test_skip_handler {prefix i} {
+    if {$i == "stepi" || $i == "nexti"} {
+	set next_pc [get_next_pc "$prefix; get next PC"]
+	gdb_test "$i" "dummy = 0.*" "$prefix; performing $i"
+	gdb_test "p /x \$pc" " = $next_pc" "$prefix; advanced"
+    } else {
+	gdb_test "$i" "done = 0.*" "$prefix; performing $i"
+    }
+}
 
 # Try stepping when there's a signal pending but no breakpoints.
 # Should skip the handler advancing to the next line.
@@ -295,13 +324,13 @@  proc skip_over_handler { i } {
 
     # Make the signal pending
     sleep 1
-    
-    gdb_test "$i" "done = 0.*" "$prefix; performing $i"
+
+    test_skip_handler $prefix $i
 }
 
-skip_over_handler step
-skip_over_handler next
-skip_over_handler continue
+foreach cmd {"stepi" "nexti" "step" "next" "continue"} {
+    skip_over_handler $cmd
+}
 
 # Try stepping when there's a signal pending, a pre-existing
 # breakpoint at the current instruction, and a breakpoint in the
@@ -385,7 +414,7 @@  breakpoint_to_handler_entry continue
 
 # Try stepping when there's a signal pending, and a pre-existing
 # breakpoint at the current instruction, and no breakpoint in the
-# handler.  Should advance to the next line.
+# handler.  Should advance to the next line/instruction.
 
 proc breakpoint_over_handler { i } {
     global gdb_prompt
@@ -409,10 +438,10 @@  proc breakpoint_over_handler { i } {
     # Make the signal pending
     sleep 1
     
-    gdb_test "$i" "done = 0.*" "$prefix; performing $i"
+    test_skip_handler $prefix $i
     gdb_test "clear $infinite_loop" ".*" "$prefix; clear infinite loop"
 }
 
-breakpoint_over_handler step
-breakpoint_over_handler next
-breakpoint_over_handler continue
+foreach cmd {"stepi" "nexti" "step" "next" "continue"} {
+    breakpoint_over_handler $cmd
+}