Re: [PATCH] stepi/nexti: skip signal handler if "handle nostop" signal arrives

[Pedro Alves <palves at redhat dot com>]

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

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