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

[Pedro Alves <palves@redhat.com>]

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.

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

-- 
1.9.3


I've now folded that into the patch that teaches non-stop about
in-line step overs.


>> +	      keep_going (ecs);
>> +
>> +	      /* We've canceled the step-over temporarily while the
>> +		 signal handler executes.  Let other threads run,
>> +		 according to schedlock.  */
> 
> IMO, we don't need to run other threads according to schedlock.  GDB
> resumes some threads here and operations should be invisible to user,
> schedlock, as a user visible option, shouldn't affect what threads
> should be resumed.  On the other hand, restart_threads is to undo the
> changes done by stop_all_threads, so no user preference (schedlock)
> should be considered here.

Yes, you're right, thanks for noticing this.  The code in restart_threads does
not in fact look at schedlock (through e.g., user_visible_resume_ptid) at
all, it's the comment that is stale from a previous attempt, from
before https://sourceware.org/ml/gdb-patches/2015-03/msg00293.html.
It was issues like that led to that schedlock patch, even.