Re: [PATCH 3/4] Make "set scheduler-locking step" depend on user intention, only

[Pedro Alves <palves at redhat dot com>]

Hi Eli,

Looks like nobody disagreed with this, so far.

Does the documentation change look OK?

Thanks,
Pedro Alves

On 03/11/2015 02:39 PM, Pedro Alves wrote:
> Currently, "set scheduler-locking step" is a bit odd.  The manual
> documents it as being optimized for stepping, so that focus of
> debugging does not change unexpectedly, but then it says that
> sometimes other threads may run, and thus focus may indeed change
> unexpectedly...  A user can then be excused to get confused and wonder
> why does GDB behave like this.
> 
> I don't think a user should have to know about details of how "next"
> or whatever other run control command is implemented internally to
> understand when does the "scheduler-locking step" setting take effect.
> 
> This patch completes a transition that the code has been moving
> towards for a while.  It makes "set scheduler-locking step" hold
> threads depending on whether the _command_ the user entered was a
> stepping command [step/stepi/next/nexti], or not.
> 
> Before, GDB could end up locking threads even on "continue" if for
> some reason run control decides a thread needs to be single stepped
> (e.g., for a software watchpoint).
> 
> After, if a "continue" happens to need to single-step for some reason,
> we won't lock threads (unless when stepping over a breakpoint,
> naturally).  And if a stepping command wants to continue a thread for
> bit, like when skipping a function to a step-resume breakpoint, we'll
> still lock threads, so focus of debugging doesn't change.
> 
> In order to make this work, we need to record in the thread structure
> whether what set it running was a stepping command.
> 
> (A follow up patch will remove the "step" parameters of 'proceed' and 'resume')
> 
> FWIW, Fedora GDB, which defaults to "scheduler-locking step" (mainline
> defaults to "off") carries a different patch that goes in this
> direction as well.
> 
> Tested on x86_64 Fedora 20, native and gdbserver.
> 
> gdb/ChangeLog:
> 2015-03-11  Pedro Alves  <palves@redhat.com>
> 
> 	* gdbthread.h (struct thread_control_state) <stepping_command>:
> 	New field.
> 	* infcmd.c (step_once): Pass step=1 to clear_proceed_status.  Set
> 	the thread's stepping_command field.
> 	* infrun.c (resume): Check the thread's stepping_command flag to
> 	determine which threads should be resumed.  Rename 'entry_step'
> 	local to user_step.
> 	(clear_proceed_status_thread): Clear 'stepping_command'.
> 	(schedlock_applies): Change parameter type to struct thread_info
> 	pointer.  Adjust.
> 	(find_thread_needs_step_over): Remove 'step' parameter.  Adjust.
> 	(switch_back_to_stepped_thread): Adjust calls to
> 	'schedlock_applies'.
> 	(_initialize_infrun): Adjust "set scheduler-locking step" help.
> 
> gdb/testsuite/ChangeLog:
> 2015-03-11  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.threads/schedlock.exp (test_step): No longer expect that
> 	"set scheduler-locking step" with "next" over a function call runs
> 	threads unlocked.
> 
> gdb/doc/ChangeLog:
> 2015-03-11  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.texinfo (test_step) <set scheduler-locking step>: No longer
> 	mention that threads may sometimes run unlocked.
> ---
>  gdb/doc/gdb.texinfo                     |  5 ++--
>  gdb/gdbthread.h                         |  5 ++++
>  gdb/infcmd.c                            |  3 ++-
>  gdb/infrun.c                            | 43 +++++++++++++++------------------
>  gdb/testsuite/gdb.threads/schedlock.exp | 11 ++++-----
>  5 files changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 4b76ce9..d0e99fd 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -5846,9 +5846,8 @@ current thread may run when the inferior is resumed.  The @code{step}
>  mode optimizes for single-stepping; it prevents other threads 
>  from preempting the current thread while you are stepping, so that 
>  the focus of debugging does not change unexpectedly.
> -Other threads only rarely (or never) get a chance to run
> -when you step.  They are more likely to run when you @samp{next} over a
> -function call, and they are completely free to run when you use commands
> +Other threads never get a chance to run when you step, and they are
> +completely free to run when you use commands
>  like @samp{continue}, @samp{until}, or @samp{finish}.  However, unless another
>  thread hits a breakpoint during its timeslice, @value{GDBN} does not change
>  the current thread away from the thread that you are debugging.
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index ce4f76f..bb15717 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -138,6 +138,11 @@ struct thread_control_state
>       thread was resumed as a result of a command applied to some other
>       thread (e.g., "next" with scheduler-locking off).  */
>    struct interp *command_interp;
> +
> +  /* Whether the command that started the thread was a stepping
> +     command.  This is used to decide whether "set scheduler-locking
> +     step" behaves like "on" or "off".  */
> +  int stepping_command;
>  };
>  
>  /* Inferior thread specific part of `struct infcall_suspend_state'.
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 0211b5d..eddbbff 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1047,7 +1047,7 @@ step_once (int skip_subroutines, int single_inst, int count, int thread)
>  	 THREAD is set.  */
>        struct thread_info *tp = inferior_thread ();
>  
> -      clear_proceed_status (!skip_subroutines);
> +      clear_proceed_status (1);
>        set_step_frame ();
>  
>        if (!single_inst)
> @@ -1121,6 +1121,7 @@ step_once (int skip_subroutines, int single_inst, int count, int thread)
>  	tp->control.step_over_calls = STEP_OVER_ALL;
>  
>        tp->step_multi = (count > 1);
> +      tp->control.stepping_command = 1;
>        proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT, 1);
>  
>        /* For async targets, register a continuation to do any
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index be1cc74..9ad7480 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2038,7 +2038,6 @@ user_visible_resume_ptid (int step)
>     we get a SIGINT random_signal, but for remote debugging and perhaps
>     other targets, that's not true).
>  
> -   STEP nonzero if we should step (zero to continue instead).
>     SIG is the signal to give the inferior (zero for none).  */
>  void
>  resume (int step, enum gdb_signal sig)
> @@ -2050,13 +2049,10 @@ resume (int step, enum gdb_signal sig)
>    CORE_ADDR pc = regcache_read_pc (regcache);
>    struct address_space *aspace = get_regcache_aspace (regcache);
>    ptid_t resume_ptid;
> -  /* From here on, this represents the caller's step vs continue
> -     request, while STEP represents what we'll actually request the
> -     target to do.  STEP can decay from a step to a continue, if e.g.,
> -     we need to implement single-stepping with breakpoints (software
> -     single-step).  When deciding whether "set scheduler-locking step"
> -     applies, it's the callers intention that counts.  */
> -  const int entry_step = step;
> +  /* This represents the user's step vs continue request.  When
> +     deciding whether "set scheduler-locking step" applies, it's the
> +     user's intention that counts.  */
> +  const int user_step = tp->control.stepping_command;
>  
>    tp->stepped_breakpoint = 0;
>  
> @@ -2165,7 +2161,7 @@ resume (int step, enum gdb_signal sig)
>  	      target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
>  	      /* ... and safe to let other threads run, according to
>  		 schedlock.  */
> -	      resume_ptid = user_visible_resume_ptid (entry_step);
> +	      resume_ptid = user_visible_resume_ptid (user_step);
>  	      target_resume (resume_ptid, 0, GDB_SIGNAL_0);
>  	      discard_cleanups (old_cleanups);
>  	      return;
> @@ -2207,7 +2203,7 @@ resume (int step, enum gdb_signal sig)
>  	     Unless we're calling an inferior function, as in that
>  	     case we pretend the inferior doesn't run at all.  */
>  	  if (!tp->control.in_infcall)
> -	    set_running (user_visible_resume_ptid (entry_step), 1);
> +	    set_running (user_visible_resume_ptid (user_step), 1);
>  	  discard_cleanups (old_cleanups);
>  	  return;
>  	}
> @@ -2280,7 +2276,7 @@ resume (int step, enum gdb_signal sig)
>    /* Decide the set of threads to ask the target to resume.  Start
>       by assuming everything will be resumed, than narrow the set
>       by applying increasingly restricting conditions.  */
> -  resume_ptid = user_visible_resume_ptid (entry_step);
> +  resume_ptid = user_visible_resume_ptid (user_step);
>  
>    /* Even if RESUME_PTID is a wildcard, and we end up resuming less
>       (e.g., we might need to step over a breakpoint), from the
> @@ -2413,6 +2409,7 @@ clear_proceed_status_thread (struct thread_info *tp)
>    tp->control.proceed_to_finish = 0;
>  
>    tp->control.command_interp = NULL;
> +  tp->control.stepping_command = 0;
>  
>    /* Discard any remaining commands or status from previous stop.  */
>    bpstat_clear (&tp->control.stop_bpstat);
> @@ -2492,21 +2489,19 @@ thread_still_needs_step_over (struct thread_info *tp)
>     we're about to do a step/next-like command to a thread.  */
>  
>  static int
> -schedlock_applies (int step)
> +schedlock_applies (struct thread_info *tp)
>  {
>    return (scheduler_mode == schedlock_on
>  	  || (scheduler_mode == schedlock_step
> -	      && step));
> +	      && tp->control.stepping_command));
>  }
>  
>  /* Look a thread other than EXCEPT that has previously reported a
>     breakpoint event, and thus needs a step-over in order to make
> -   progress.  Returns NULL is none is found.  STEP indicates whether
> -   we're about to step the current thread, in order to decide whether
> -   "set scheduler-locking step" applies.  */
> +   progress.  Returns NULL is none is found.  */
>  
>  static struct thread_info *
> -find_thread_needs_step_over (int step, struct thread_info *except)
> +find_thread_needs_step_over (struct thread_info *except)
>  {
>    struct thread_info *tp, *current;
>  
> @@ -2517,7 +2512,7 @@ find_thread_needs_step_over (int step, struct thread_info *except)
>  
>    /* If scheduler locking applies, we can avoid iterating over all
>       threads.  */
> -  if (schedlock_applies (step))
> +  if (schedlock_applies (except))
>      {
>        if (except != current
>  	  && thread_still_needs_step_over (current))
> @@ -2652,7 +2647,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
>  
>  	 Look for a thread other than the current (TP) that reported a
>  	 breakpoint hit and hasn't been resumed yet since.  */
> -      step_over = find_thread_needs_step_over (step, tp);
> +      step_over = find_thread_needs_step_over (tp);
>        if (step_over != NULL)
>  	{
>  	  if (debug_infrun)
> @@ -5592,7 +5587,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
>  	 current thread is stepping.  If some other thread not the
>  	 event thread is stepping, then it must be that scheduler
>  	 locking is not in effect.  */
> -      if (schedlock_applies (0))
> +      if (schedlock_applies (ecs->event_thread))
>  	return 0;
>  
>        /* Look for the stepping/nexting thread, and check if any other
> @@ -5628,7 +5623,7 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
>  		 stepping, then scheduler locking can't be in effect,
>  		 otherwise we wouldn't have resumed the current event
>  		 thread in the first place.  */
> -	      gdb_assert (!schedlock_applies (currently_stepping (tp)));
> +	      gdb_assert (!schedlock_applies (tp));
>  
>  	      stepping_thread = tp;
>  	    }
> @@ -7875,9 +7870,9 @@ Set mode for locking scheduler during execution."), _("\
>  Show mode for locking scheduler during execution."), _("\
>  off  == no locking (threads may preempt at any time)\n\
>  on   == full locking (no thread except the current thread may run)\n\
> -step == scheduler locked during every single-step operation.\n\
> -	In this mode, no other thread may run during a step command.\n\
> -	Other threads may run while stepping over a function call ('next')."), 
> +step == scheduler locked during stepping commands (step, next, stepi, nexti).\n\
> +	In this mode, other threads may run with other commands.\n\
> +	Other threads may run during other commands."),
>  			set_schedlock_func,	/* traps on target vector */
>  			show_scheduler_mode,
>  			&setlist, &showlist);
> diff --git a/gdb/testsuite/gdb.threads/schedlock.exp b/gdb/testsuite/gdb.threads/schedlock.exp
> index b47af77..54e847e 100644
> --- a/gdb/testsuite/gdb.threads/schedlock.exp
> +++ b/gdb/testsuite/gdb.threads/schedlock.exp
> @@ -285,8 +285,7 @@ proc test_step { schedlock cmd call_function } {
>  
>      step_ten_loops $cmd
>  
> -    # "next" lets other threads run while stepping over functions.
> -    if { $schedlock == "on" || ($schedlock == "step" && !$call_function) } {
> +    if { $schedlock == "on" || $schedlock == "step" } {
>  	set locked 1
>      } else {
>  	set locked 0
> @@ -302,10 +301,10 @@ foreach schedlock {"off" "step" "on"} {
>  	    test_step $schedlock "step" 0
>  	}
>  	with_test_prefix "cmd=next" {
> -	    # With "next", and schedlock "step", threads run unlocked
> -	    # when stepping over a function call.  This exercises both
> -	    # with and without a function call.  Without a function
> -	    # call "next" should behave just like "step".
> +	    # In GDB <= 7.9, with schedlock "step", "next" would
> +	    # unlock threads when stepping over a function call.  This
> +	    # exercises "next" with and without a function call.  WRT
> +	    # "schedlock step", "next" should behave just like "step".
>  	    foreach call_function {0 1} {
>  		with_test_prefix "call_function=$call_function" {
>  		    test_step $schedlock "next" $call_function
>