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

[Doug Evans <dje@google.com>]

Pedro Alves 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
  > 	other threads that are implicitly resumed too.
  > 	(for_each_just_stopped_thread, fetch_inferior_event)
  > 	(adjust_pc_after_break, stop_all_threads): Check
  > 	target_is_non_stop_p instead of non_stop.
  > 	(handle_inferior_event): Likewise.  Handle detach-fork in all-stop
  > 	with the target always in non-stop mode.
  > 	(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.
  > 	(switch_back_to_stepped_thread): Check target_is_non_stop_p
  > 	instead of non_stop.
  > 	(keep_going_stepped_thread): Always resume a single thread if the
  > 	target is in non-stop mode.
  > 	(stop_waiting): If in all-stop mode, and the target is in non-stop
  > 	mode, stop all threads.
  > 	(keep_going_pass): Likewise, when starting a new in-line step-over
  > 	sequence.
  > 	* linux-nat.c (get_pending_status, select_event_lwp)
  > 	(linux_nat_filter_event, linux_nat_wait_1, linux_nat_wait): Check
  > 	target_is_non_stop_p instead of non_stop.
  > 	(linux_nat_always_non_stop_p): New function.
  > 	(linux_nat_stop): Check target_is_non_stop_p instead of non_stop.
  > 	(linux_nat_add_target): Install linux_nat_always_non_stop_p.
  > 	* target-delegates.c: Regenerate.
  > 	* target.c (target_is_non_stop_p): New function.
  > 	(target_non_stop_enabled, target_non_stop_enabled_1): New globals.
  > 	(maint_set_target_non_stop_command)
  > 	(maint_show_target_non_stop_command): New functions.
  > 	(_initilize_target): Install "maint set/show target-non-stop"
  > 	commands.
  > 	* target.h (struct target_ops) <to_always_non_stop_p>: New field.
  > 	(target_non_stop_enabled): New declaration.
  > 	(target_is_non_stop_p): New declaration.
  >
  > gdb/doc/ChangeLog:
  > 2015-04-07  Pedro Alves  <palves@redhat.com>
  >
  > 	* gdb.texinfo (Maintenance Commands): Document "maint set/show
  > 	target-non-stop".
  >
> ...
  > diff --git a/gdb/infrun.c b/gdb/infrun.c
  > index 8859b9f..f6d3e07 100644
  > --- a/gdb/infrun.c
  > +++ b/gdb/infrun.c
  > @@ -1632,7 +1632,7 @@ show_can_use_displaced_stepping (struct ui_file  
*file, int from_tty,
  >      fprintf_filtered (file,
  >  		      _("Debugger's willingness to use displaced stepping "
  >  			"to step over breakpoints is %s (currently %s).\n"),
  > -		      value, non_stop ? "on" : "off");
  > +		      value, target_is_non_stop_p () ? "on" : "off");
  >    else
  >      fprintf_filtered (file,
  >  		      _("Debugger's willingness to use displaced stepping "
  > @@ -1645,7 +1645,8 @@ show_can_use_displaced_stepping (struct ui_file  
*file, int from_tty,
  >  static int
  >  use_displaced_stepping (struct gdbarch *gdbarch)
  >  {
  > -  return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO && non_stop)
  > +  return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
  > +	    && target_is_non_stop_p ())
  >  	   || can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
  >  	  && gdbarch_displaced_step_copy_insn_p (gdbarch)
  >  	  && find_record_target () == NULL);
  > @@ -2016,7 +2017,7 @@ start_step_over (void)
  >  	 wouldn't be able to resume anything else until the target
  >  	 stops again.  In non-stop, the resume always resumes only TP,
  >  	 so it's OK to let the thread resume freely.  */
  > -      if (!non_stop && !step_what)
  > +      if (!target_is_non_stop_p () && !step_what)
  >  	continue;
  >
  >        switch_to_thread (tp->ptid);
  > @@ -2035,7 +2036,7 @@ start_step_over (void)
  >  	  return 1;
  >  	}
  >
  > -      if (!non_stop)
  > +      if (!target_is_non_stop_p ())
  >  	{
  >  	  /* On all-stop, shouldn't have resumed unless we needed a
  >  	     step over.  */
  > @@ -2383,7 +2384,10 @@ resume (enum gdb_signal sig)
  >  	      insert_single_step_breakpoint (gdbarch, aspace, pc);
  >  	      insert_breakpoints ();
  >
  > -	      resume_ptid = user_visible_resume_ptid (user_step);
  > +	      if (target_is_non_stop_p ())
  > +		resume_ptid = inferior_ptid;
  > +	      else
  > +		resume_ptid = user_visible_resume_ptid (user_step);

Hi.
For my own education, why is this change needed?

  >  	      do_target_resume (resume_ptid, 0, GDB_SIGNAL_0);
  >  	      discard_cleanups (old_cleanups);
  >  	      tp->resumed = 1;
  > @@ -2498,8 +2502,14 @@ resume (enum gdb_signal sig)
  >    resume_ptid = user_visible_resume_ptid (user_step);
  >
  >    /* Maybe resume a single thread after all.  */
  > -  if ((step || thread_has_single_step_breakpoints_set (tp))
  > -      && tp->control.trap_expected)
  > +  if (target_is_non_stop_p ())
  > +    {
  > +      /* If non-stop mode, threads are always controlled
  > +	 individually.  */
  > +      resume_ptid = inferior_ptid;
  > +    }
  > +  else if ((step || thread_has_single_step_breakpoints_set (tp))
  > +	   && tp->control.trap_expected)
  >      {
  >        /* We're allowing a thread to run past a breakpoint it has
  >  	 hit, by single-stepping the thread with the breakpoint
  > @@ -2935,11 +2945,51 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
  >  	 other thread was already doing one.  In either case, don't
  >  	 resume anything else until the step-over is finished.  */
  >      }
  > -  else if (started && !non_stop)
  > +  else if (started && !target_is_non_stop_p ())
  >      {
  >        /* A new displaced stepping sequence was started.  In all-stop,
  >  	 we can't talk to the target anymore until it next stops.  */
  >      }
  > +  else if (!non_stop && target_is_non_stop_p ())

This takes a bit of reading to grok (the difference
b/w non_stop and target_is_non_stop_p).
Can you copy the comment below (marked XYZ) here?

  > +    {
  > +      /* Start all other threads that are implicitly resumed too.  */
  > +      ALL_NON_EXITED_THREADS (tp)
  > +        {
  > +	  /* Ignore threads of processes we're not resuming.  */
  > +	  if (!ptid_match (tp->ptid, resume_ptid))
  > +	    continue;
  > +
  > +	  if (tp->resumed)
  > +	    {
  > +	      if (debug_infrun)
  > +		fprintf_unfiltered (gdb_stdlog,
  > +				    "infrun: proceed: [%s] resumed\n",
  > +				    target_pid_to_str (tp->ptid));
  > +	      gdb_assert (tp->executing || tp->suspend.waitstatus_pending_p);
  > +	      continue;
  > +	    }
  > +
  > +	  if (thread_is_in_step_over_chain (tp))
  > +	    {
  > +	      if (debug_infrun)
  > +		fprintf_unfiltered (gdb_stdlog,
  > +				    "infrun: proceed: [%s] needs step-over\n",
  > +				    target_pid_to_str (tp->ptid));
  > +	      continue;
  > +	    }
  > +
  > +	  if (debug_infrun)
  > +	    fprintf_unfiltered (gdb_stdlog,
  > +				"infrun: proceed: resuming %s\n",
  > +				target_pid_to_str (tp->ptid));
  > +
  > +	  reset_ecs (ecs, tp);
  > +	  switch_to_thread (tp->ptid);
  > +	  keep_going_pass (ecs);
  > +	  if (!ecs->wait_some_more)
  > +	    error ("Command aborted.");
  > +	}
  > +    }
  >    else if (!tp->resumed && !thread_is_in_step_over_chain (tp))
  >      {
  >        /* The thread wasn't started, and isn't queued, run it now.  */
> ...
  > @@ -5037,7 +5089,7 @@ finish_step_over (struct execution_control_state  
*ecs)
  >  	clear_step_over_info ();
  >      }
  >
  > -  if (!non_stop)
  > +  if (!target_is_non_stop_p ())
  >      return 0;
  >
  >    /* Start a new step-over in another thread if there's one that
  > @@ -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 ())

Ditto. Copy XYZ here?

  > +	    {
  > +	      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);
  > +	      return;
  > +	    }
  > +
  >  	  /* If we were nexting/stepping some other thread, switch to
  >  	     it, so that we don't continue it, losing control.  */
  >  	  if (!switch_back_to_stepped_thread (ecs))
> ...
  > @@ -7153,6 +7220,11 @@ stop_waiting (struct execution_control_state *ecs)
  >
  >    /* Let callers know we don't want to wait for the inferior anymore.   
*/
  >    ecs->wait_some_more = 0;
  > +
  > +  /* If all-stop, but the target is always in non-stop mode, stop all
  > +     threads now that we're presenting the stop to the user.  */

XYZ^^^
"If all-stop, but the target is always in non-stop mode, ..."

  > +  if (!non_stop && target_is_non_stop_p ())
  > +    stop_all_threads ();
  >  }
  >
  >  /* Like keep_going, but passes the signal to the inferior, even if the
  > @@ -7267,7 +7339,7 @@ keep_going_pass (struct execution_control_state  
*ecs)
  >  	 insert_breakpoints below, because that removes the breakpoint
  >  	 we're about to step over, otherwise other threads could miss
  >  	 it.  */
  > -      if (step_over_info_valid_p () && non_stop)
  > +      if (step_over_info_valid_p () && target_is_non_stop_p ())
  >  	stop_all_threads ();
  >
  >        /* Stop stepping if inserting breakpoints fails.  */