[v3,10/17] Implement all-stop on top of a target running non-stop mode

Message ID 5538017B.9040907@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves April 22, 2015, 8:15 p.m. UTC
  On 04/21/2015 12:08 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> gdb/ChangeLog:
>> 2015-04-17  Pedro Alves  <palves@redhat.com>
>>
>> 	* NEWS: Mention "maint set/show target-non-stop".
>> 	* breakpoint.c (update_global_location_list): Check
>> 	target_is_non_stop_p instead of non_stop.
>> 	* infcmd.c (attach_command_post_wait, attach_command): Likewise.
>> 	* infrun.c (show_can_use_displaced_stepping)
>> 	(can_use_displaced_stepping_p, start_step_over_inferior):
>> 	Likewise.
>> 	(resume): Always resume a single thread if the target is in
>> 	non-stop mode.
>> 	(proceed): Check target_is_non_stop_p instead of non_stop.  If in
>> 	all-mode but the target is always in non-stop mode, start all the
>         ^^^^^^^^
> 
> all-stop mode.

Thanks, fixed.

> 
>> 	(handle_signal_stop) <random signal>: If we get a signal while
>> 	stepping over a breakpoint, and the target is always in non-stop
>> 	mode, restart all threads.
> 
>> @@ -5614,6 +5666,17 @@ handle_signal_stop (struct execution_control_state *ecs)
>>  	  /* Reset trap_expected to ensure breakpoints are re-inserted.  */
>>  	  ecs->event_thread->control.trap_expected = 0;
>>  
>> +	  if (!non_stop && target_is_non_stop_p ())
>> +	    {
> 
> This path is about the case that a signal is got while in in-line
> stepping, isn't?  If so, non_stop should be an invariant false.  We
> don't need to check it.

Hmm, not sure what you mean:

 - We need to do this with displaced stepping too, because we can't
   deliver signals while doing a displaced step.  See comments at the
   top of displaced_step_prepare and in do_target_resume.

 - We can certainly get a signal while doing an in-line step-over.
   The simplest would be, trying to step-over a breakpoint here:

      *(volatile int *)0 = 1;

   which usually results SIGSEGV.

 - We can step past breakpoints in-line in non-stop mode too.

However, I agree there's something wrong here.  If we get here
with "set non-stop on", and we were doing an in-line step-over,
then we also need to restart threads, not just
in all-stop + "target always-non-stop".  I've applied this change
on top now, and it tested OK on x86-64, native with
both displaced on/off, and gdbserver.
  

Comments

Yao Qi April 24, 2015, 7:39 a.m. UTC | #1
Pedro Alves <palves@redhat.com> writes:

>> This path is about the case that a signal is got while in in-line
>> stepping, isn't?  If so, non_stop should be an invariant false.  We
>> don't need to check it.
>
> Hmm, not sure what you mean:
>

Let me ask it in another way, when we get here, it means a signal
arrived, the code some lines above is:

          if (debug_infrun)
            fprintf_unfiltered (gdb_stdlog,
                                "infrun: signal arrived while stepping over "
                                "breakpoint\n");

GDB just did the breakpoint step over, via in-line stepping or
out-of-line stepping, right? as your patch below shows.

>  - We need to do this with displaced stepping too, because we can't
>    deliver signals while doing a displaced step.  See comments at the
>    top of displaced_step_prepare and in do_target_resume.

The first sentence is contradictory, or you mean we *can* do either
out-of-line stepping or in-line stepping, but we can't deliver a signal
while doing a displaced stepping...

>
>  - We can certainly get a signal while doing an in-line step-over.
>    The simplest would be, trying to step-over a breakpoint here:
>
>       *(volatile int *)0 = 1;
>
>    which usually results SIGSEGV.

... while we can deliver a signal in in-line stepping.  Is it correct?
  
Pedro Alves May 19, 2015, 6:08 p.m. UTC | #2
Hi Yao,

I had not realized I missed answering this.

On 04/24/2015 08:39 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> This path is about the case that a signal is got while in in-line
>>> stepping, isn't?  If so, non_stop should be an invariant false.  We
>>> don't need to check it.
>>
>> Hmm, not sure what you mean:
>>
> 
> Let me ask it in another way, when we get here, it means a signal
> arrived, the code some lines above is:
> 
>           if (debug_infrun)
>             fprintf_unfiltered (gdb_stdlog,
>                                 "infrun: signal arrived while stepping over "
>                                 "breakpoint\n");
> 
> GDB just did the breakpoint step over, via in-line stepping or
> out-of-line stepping, right? as your patch below shows.
> 
>>  - We need to do this with displaced stepping too, because we can't
>>    deliver signals while doing a displaced step.  See comments at the
>>    top of displaced_step_prepare and in do_target_resume.
> 
> The first sentence is contradictory, or you mean we *can* do either
> out-of-line stepping or in-line stepping, but we can't deliver a signal
> while doing a displaced stepping...

By "need to do this" I meant that we need to cancel the step over in
progress, and insert the step-resume at the signal handlers return.

> 
>>
>>  - We can certainly get a signal while doing an in-line step-over.
>>    The simplest would be, trying to step-over a breakpoint here:
>>
>>       *(volatile int *)0 = 1;
>>
>>    which usually results SIGSEGV.
> 
> ... while we can deliver a signal in in-line stepping.  Is it correct?
> 

Not really.

We can't deliver a signal while a displaced step is in
progress, because we wouldn't be able to tell whether the thread stepped
to the handler or to main line code, so we wouldn't know whether to apply
the displaced step fixup.  Also, if the thread stops for some reason while
in the handler, we'd end up with the scratch pad still in the frame chain,
so later when the thread is re-resumed, it'd return to the scratch pad, but
the scratch pad would have already have been overwritten.

And we can't deliver a signal while stepping over a breakpoint in-line
either, because that requires removing the breakpoint, and the signal handler
can recurse and call the same code that had the breakpoint we were
stepping over -- which would mean we'd miss trapping on that breakpoint.
(this is tested by signest.exp).

So the only difference between stepping over a breakpoint in-line or
out-of-line here, is that with the former, threads had been previously
paused, so we re-resume them while the signal handler runs (to avoid
debugger-induced inferior deadlock).

Let me know if that answered your question.

Thanks,
Pedro Alves
  
Yao Qi May 21, 2015, 9:17 a.m. UTC | #3
Pedro Alves <palves@redhat.com> writes:

> So the only difference between stepping over a breakpoint in-line or
> out-of-line here, is that with the former, threads had been previously
> paused, so we re-resume them while the signal handler runs (to avoid
> debugger-induced inferior deadlock).

Looks in-line stepping and out-of-line stepping behaves similarly under
this context.

>
> Let me know if that answered your question.

Yes, that is what I want to know, thanks.
  

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e236510..3e60313 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5732,6 +5732,8 @@  handle_signal_stop (struct execution_control_state *ecs)
 	  && ecs->event_thread->control.trap_expected
 	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
 	{
+	  int was_in_line;
+
 	  /* We were just starting a new sequence, attempting to
 	     single-step off of a breakpoint and expecting a SIGTRAP.
 	     Instead this signal arrives.  This signal will take us out
@@ -5747,20 +5749,29 @@  handle_signal_stop (struct execution_control_state *ecs)
                                 "infrun: signal arrived while stepping over "
                                 "breakpoint\n");

+	  was_in_line = step_over_info_valid_p ();
 	  clear_step_over_info ();
 	  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;

-	  if (!non_stop && target_is_non_stop_p ())
+	  if (target_is_non_stop_p ())
 	    {
 	      keep_going (ecs);

-	      /* We've canceled the step-over temporarily while the
-		 signal handler executes.  Let other threads run,
-		 according to schedlock.  */
-	      restart_threads (ecs->event_thread);
+	      /* The step-over has been canceled temporarily while the
+		 signal handler executes.  */
+	      if (was_in_line)
+		{
+		  /* We had paused all threads, restart them.  */
+		  restart_threads (ecs->event_thread);
+		}
+	      else if (debug_infrun)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "infrun: no need to restart threads\n");
+		}
 	      return;
 	    }