[PATCH 3/4] Make "set scheduler-locking step" depend on user intention, only
Pedro Alves
palves@redhat.com
Wed Mar 18 20:06:00 GMT 2015
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
>
More information about the Gdb-patches
mailing list