From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 65335 invoked by alias); 18 Mar 2015 20:06:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 65245 invoked by uid 89); 18 Mar 2015 20:06:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 18 Mar 2015 20:06:23 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 25CF1C6A1B; Wed, 18 Mar 2015 20:06:22 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2IK6JB0008014; Wed, 18 Mar 2015 16:06:20 -0400 Message-ID: <5509DABB.9030906@redhat.com> Date: Wed, 18 Mar 2015 20:06:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Eli Zaretskii CC: GDB Patches Subject: Re: [PATCH 3/4] Make "set scheduler-locking step" depend on user intention, only References: <1426084798-1032-1-git-send-email-palves@redhat.com> <1426084798-1032-4-git-send-email-palves@redhat.com> In-Reply-To: <1426084798-1032-4-git-send-email-palves@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00563.txt.bz2 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 > > * gdbthread.h (struct thread_control_state) : > 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 > > * 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 > > * gdb.texinfo (test_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 >