From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id tQjmDydRN2EtMQAAWB0awg (envelope-from ) for ; Tue, 07 Sep 2021 07:46:47 -0400 Received: by simark.ca (Postfix, from userid 112) id 2EAF01EE22; Tue, 7 Sep 2021 07:46:47 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.4 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id F0FF61EDF0 for ; Tue, 7 Sep 2021 07:46:44 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7B81B3858423 for ; Tue, 7 Sep 2021 11:46:44 +0000 (GMT) Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id 9895E3858415 for ; Tue, 7 Sep 2021 11:46:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9895E3858415 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wm1-x335.google.com with SMTP id u26-20020a05600c441a00b002f66b2d8603so1565280wmn.4 for ; Tue, 07 Sep 2021 04:46:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=XKypm3fR7kK1TmRQvG3Zj+oHD4OenOocFe6QAIg7dA0=; b=bDDWDl58+AcrSnrbjGOX7BPMTTsVDQqubfCKqdJbbnTZtz3XZGcz1CpRDWAd6Sx+cx TG85vhohqFPpbzxbRpsMI2coPOIhhjzJ/TsIGNGqZ34ndV9L2S27MCADipU0DQTiVAOd Q9fHkVjGGjkp2eKsLF+pf4nxmCPu2oVO1tcrss8ymvqTYgWLJu6TX8dCaRJ+wMdW5CsR 5p3d+6TleUR+g0CkAfwQP9ta6PHd9ZpBjTqNvOEde0ApiT0pqNXArLH1o9aYBOXCipCT EgrC+Epo6NcXFIQGS29VKHK7jYkdvcqQzqcdxJs+L7c948+3QgwyoyEJQ/45pXs1OCdf 2l0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=XKypm3fR7kK1TmRQvG3Zj+oHD4OenOocFe6QAIg7dA0=; b=Wdf4JfTM16SqKVtfE6aGXcQdUieKxfDhDPwrIInA5LT7X9VsgpVZ//IfbhZhIryraI RIj5hrP9isv1pO8YnRY2BltRck9vIHNhyQTCk3YvXfhfoaQ4ZaothqlxN3QngwM6PpiY DtxoH1e7ykzp26YO0cQNofmemJHcCJbIMCxwbVaHt7fxslAmVt7PV7fSM84fQXDyt5ve p2ZJt77mHb/7/uzg/hwoP4eMU8AYOHfJMnep/vMZSoGzEJ6j6fI491TECNkAcAvbh3lt IsUZUNILspQu8Bj4SFEUzH+z7PlFerY5PAN1FfwVqF0+7LZ07JFyCjhC1CQUVWUAu5Ca 2jxA== X-Gm-Message-State: AOAM5321ZVm5+NWS+Y4XqHspsz/H0KXYk9UVbUK2hpJFi3hmPjPt2FFT ApNP95L0ONEcaccUFRIg/d7oZUKTfT0TTw== X-Google-Smtp-Source: ABdhPJx57TvCqtoY9xr9xg66SraJxQ/zKhueo7q3yImMnJnAcfDIkdLsN7PluqWxLmg8qxNKt0cSJw== X-Received: by 2002:a7b:c052:: with SMTP id u18mr3493326wmc.105.1631015192135; Tue, 07 Sep 2021 04:46:32 -0700 (PDT) Received: from localhost (host86-191-239-77.range86-191.btcentralplus.com. [86.191.239.77]) by smtp.gmail.com with ESMTPSA id x11sm10776374wro.83.2021.09.07.04.46.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Sep 2021 04:46:31 -0700 (PDT) Date: Tue, 7 Sep 2021 12:46:30 +0100 From: Andrew Burgess To: gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] gdb: make thread_info::executing private Message-ID: <20210907114630.GP2581@embecosm.com> References: <0fc4a9e754ad23dad99afdbeafa1388791135408.1630353626.git.andrew.burgess@embecosm.com> <1b625bf0-9a43-801c-4a5a-a880de986106@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1b625bf0-9a43-801c-4a5a-a880de986106@polymtl.ca> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 12:45:01 up 21 days, 41 min, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" * Simon Marchi [2021-09-01 09:53:31 -0400]: > > @@ -841,24 +851,11 @@ set_running (process_stratum_target *targ, ptid_t ptid, bool running) > > gdb::observers::target_resumed.notify (ptid); > > } > > > > - > > -/* Helper for set_executing. Set's the thread's 'executing' field > > - from EXECUTING, and if EXECUTING is true also clears the thread's > > - stop_pc. */ > > That comment should probably be moved to the set_executing > declaration, in the thread_info class. The part about clearing stop_pc > is relevant to have there, I think. Thanks. I made that change, and also added a comment onto the declaration of set_resumed. I pushed just this patch from this series, as this seems like a reasonable cleanup on its own. Below is what I pushed. Thanks, Andrew --- commit 611841bb1afc689becfab6dd490e1799aabf547d Author: Andrew Burgess Date: Tue Aug 10 11:20:44 2021 +0100 gdb: make thread_info::executing private Rename thread_info::executing to thread_info::m_executing, and make it private. Add a new get/set member functions, and convert GDB to make use of these. The only real change of interest in this patch is in thread.c where I have deleted the helper function set_executing_thread, and now just use the new set function thread_info::set_executing. However, the old helper function set_executing_thread included some code to reset the thread's stop_pc, so I moved this code into the new function thread_info::set_executing. However, I don't believe there is anywhere that this results in a change of behaviour, previously the executing flag was always set true through a call to set_executing_thread anyway. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f6c9683aecf..10b28c97be7 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1661,7 +1661,7 @@ watchpoint_in_thread_scope (struct watchpoint *b) return (b->pspace == current_program_space && (b->watchpoint_thread == null_ptid || (inferior_ptid == b->watchpoint_thread - && !inferior_thread ()->executing))); + && !inferior_thread ()->executing ()))); } /* Set watchpoint B to disp_del_at_next_stop, even including its possible @@ -4512,7 +4512,7 @@ get_bpstat_thread () return NULL; thread_info *tp = inferior_thread (); - if (tp->state == THREAD_EXITED || tp->executing) + if (tp->state == THREAD_EXITED || tp->executing ()) return NULL; return tp; } diff --git a/gdb/frame.c b/gdb/frame.c index 4d7505f7ae3..d28944075ed 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1789,7 +1789,7 @@ has_stack_frames () return false; /* ... or from a spinning thread. */ - if (tp->executing) + if (tp->executing ()) return false; } diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 9c178f531d9..e6f383cca61 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -287,15 +287,19 @@ class thread_info : public refcounted_object, if the thread does not have a user-given name. */ char *name = NULL; - /* True means the thread is executing. Note: this is different - from saying that there is an active target and we are stopped at - a breakpoint, for instance. This is a real indicator whether the - thread is off and running. */ - bool executing = false; + bool executing () const + { return m_executing; } + + /* Set the thread's 'm_executing' field from EXECUTING, and if EXECUTING + is true also clears the thread's stop_pc. */ + void set_executing (bool executing); bool resumed () const { return m_resumed; } + /* Set the thread's 'm_resumed' field from RESUMED. The thread may also + be added to (when RESUMED is true), or removed from (when RESUMED is + false), the list of threads with a pending wait status. */ void set_resumed (bool resumed); /* Frontend view of the thread state. Note that the THREAD_RUNNING/ @@ -488,6 +492,12 @@ class thread_info : public refcounted_object, the thread run. */ bool m_resumed = false; + /* True means the thread is executing. Note: this is different + from saying that there is an active target and we are stopped at + a breakpoint, for instance. This is a real indicator whether the + thread is off and running. */ + bool m_executing = false; + /* State of inferior thread to restore after GDB is done with an inferior call. See `struct thread_suspend_state'. */ thread_suspend_state m_suspend; diff --git a/gdb/infcmd.c b/gdb/infcmd.c index c183b60e81a..e6ee49bc43d 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -2371,7 +2371,7 @@ proceed_after_attach (inferior *inf) scoped_restore_current_thread restore_thread; for (thread_info *thread : inf->non_exited_threads ()) - if (!thread->executing + if (!thread->executing () && !thread->stop_requested && thread->stop_signal () == GDB_SIGNAL_0) { @@ -2644,7 +2644,7 @@ notice_new_inferior (thread_info *thr, bool leave_running, int from_tty) /* When we "notice" a new inferior we need to do all the things we would normally do if we had just attached to it. */ - if (thr->executing) + if (thr->executing ()) { struct inferior *inferior = current_inferior (); diff --git a/gdb/inflow.c b/gdb/inflow.c index 74dda702a8a..bcab66487b0 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -525,7 +525,7 @@ child_interrupt (struct target_ops *self) thread_info *resumed = NULL; for (thread_info *thr : all_non_exited_threads ()) { - if (thr->executing) + if (thr->executing ()) { resumed = thr; break; diff --git a/gdb/infrun.c b/gdb/infrun.c index 694bbe665f4..8e778576eab 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -862,7 +862,7 @@ static void proceed_after_vfork_done (thread_info *thread) { if (thread->state == THREAD_RUNNING - && !thread->executing + && !thread->executing () && !thread->stop_requested && thread->stop_signal () == GDB_SIGNAL_0) { @@ -1885,7 +1885,7 @@ start_step_over (void) if (tp->control.trap_expected || tp->resumed () - || tp->executing) + || tp->executing ()) { internal_error (__FILE__, __LINE__, "[%s] has inconsistent state: " @@ -1893,7 +1893,7 @@ start_step_over (void) target_pid_to_str (tp->ptid).c_str (), tp->control.trap_expected, tp->resumed (), - tp->executing); + tp->executing ()); } infrun_debug_printf ("resuming [%s] for step-over", @@ -3197,7 +3197,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) { infrun_debug_printf ("[%s] resumed", target_pid_to_str (tp->ptid).c_str ()); - gdb_assert (tp->executing || tp->has_pending_waitstatus ()); + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); continue; } @@ -3329,7 +3329,7 @@ infrun_thread_stop_requested (ptid_t ptid) { if (tp->state != THREAD_RUNNING) continue; - if (tp->executing) + if (tp->executing ()) continue; /* Remove matching threads from the step-over queue, so @@ -3778,7 +3778,7 @@ prepare_for_detach (void) { if (thr->displaced_step_state.in_progress ()) { - if (thr->executing) + if (thr->executing ()) { if (!thr->stop_requested) { @@ -4806,7 +4806,7 @@ handle_one (const wait_one_event &event) t = add_thread (event.target, event.ptid); t->stop_requested = 0; - t->executing = 0; + t->set_executing (false); t->set_resumed (false); t->control.may_range_step = 0; @@ -4947,7 +4947,7 @@ stop_all_threads (void) if (!target_is_non_stop_p ()) continue; - if (t->executing) + if (t->executing ()) { /* If already stopping, don't request a stop again. We just haven't seen the notification yet. */ @@ -5091,7 +5091,7 @@ handle_no_resumed (struct execution_control_state *ecs) for (thread_info *thread : all_non_exited_threads ()) { - if (swap_terminal && thread->executing) + if (swap_terminal && thread->executing ()) { if (thread->inf != curr_inf) { @@ -5104,7 +5104,7 @@ handle_no_resumed (struct execution_control_state *ecs) } if (!ignore_event - && (thread->executing || thread->has_pending_waitstatus ())) + && (thread->executing () || thread->has_pending_waitstatus ())) { /* Either there were no unwaited-for children left in the target at some point, but there are now, or some target @@ -5722,7 +5722,7 @@ restart_threads (struct thread_info *event_thread) { infrun_debug_printf ("restart threads: [%s] resumed", target_pid_to_str (tp->ptid).c_str ()); - gdb_assert (tp->executing || tp->has_pending_waitstatus ()); + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); continue; } @@ -5869,7 +5869,7 @@ finish_step_over (struct execution_control_state *ecs) do_target_wait. */ tp->set_resumed (true); - gdb_assert (!tp->executing); + gdb_assert (!tp->executing ()); regcache = get_thread_regcache (tp); tp->set_stop_pc (regcache_read_pc (regcache)); @@ -7419,7 +7419,7 @@ restart_after_all_stop_detach (process_stratum_target *proc_target) /* If we have any thread that is already executing, then we don't need to resume the target -- it is already been resumed. */ - if (thr->executing) + if (thr->executing ()) return; /* If we have a pending event to process, skip resuming the diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index e9433b2206b..29ca62d2bc1 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -1280,7 +1280,7 @@ get_detach_signal (struct lwp_info *lp) { struct thread_info *tp = find_thread_ptid (linux_target, lp->ptid); - if (target_is_non_stop_p () && !tp->executing) + if (target_is_non_stop_p () && !tp->executing ()) { if (tp->has_pending_waitstatus ()) signo = tp->pending_waitstatus ().value.sig; diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c index eb2aa5ac409..245939b6400 100644 --- a/gdb/linux-thread-db.c +++ b/gdb/linux-thread-db.c @@ -1641,7 +1641,7 @@ thread_db_target::update_thread_list () continue; thread_info *thread = any_live_thread_of_inferior (inf); - if (thread == NULL || thread->executing) + if (thread == NULL || thread->executing ()) continue; /* It's best to avoid td_ta_thr_iter if possible. That walks diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 85e357e604a..e2b9866d68a 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -1998,7 +1998,7 @@ get_thread_current_frame_id (struct thread_info *tp) For the former, EXECUTING is true and we're in wait, about to move the thread. Since we need to recompute the stack, we temporarily set EXECUTING to false. */ - executing = tp->executing; + executing = tp->executing (); set_executing (proc_target, inferior_ptid, false); id = null_frame_id; diff --git a/gdb/target.c b/gdb/target.c index ae2d659583e..d1c1bf523ed 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -3815,7 +3815,7 @@ target_pass_ctrlc (void) { /* A thread can be THREAD_STOPPED and executing, while running an infcall. */ - if (thr->state == THREAD_RUNNING || thr->executing) + if (thr->state == THREAD_RUNNING || thr->executing ()) { /* We can get here quite deep in target layers. Avoid switching thread context or anything that would diff --git a/gdb/thread.c b/gdb/thread.c index a82fb49140a..c95a9186681 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -319,6 +319,16 @@ thread_info::deletable () const /* See gdbthread.h. */ +void +thread_info::set_executing (bool executing) +{ + m_executing = executing; + if (executing) + this->set_stop_pc (~(CORE_ADDR) 0); +} + +/* See gdbthread.h. */ + void thread_info::set_resumed (bool resumed) { @@ -625,13 +635,13 @@ any_live_thread_of_inferior (inferior *inf) curr_tp = inferior_thread (); if (curr_tp->state == THREAD_EXITED) curr_tp = NULL; - else if (!curr_tp->executing) + else if (!curr_tp->executing ()) return curr_tp; } for (thread_info *tp : inf->non_exited_threads ()) { - if (!tp->executing) + if (!tp->executing ()) return tp; tp_executing = tp; @@ -841,24 +851,11 @@ set_running (process_stratum_target *targ, ptid_t ptid, bool running) gdb::observers::target_resumed.notify (ptid); } - -/* Helper for set_executing. Set's the thread's 'executing' field - from EXECUTING, and if EXECUTING is true also clears the thread's - stop_pc. */ - -static void -set_executing_thread (thread_info *thr, bool executing) -{ - thr->executing = executing; - if (executing) - thr->set_stop_pc (~(CORE_ADDR) 0); -} - void set_executing (process_stratum_target *targ, ptid_t ptid, bool executing) { for (thread_info *tp : all_non_exited_threads (targ, ptid)) - set_executing_thread (tp, executing); + tp->set_executing (executing); /* It only takes one running thread to spawn more threads. */ if (executing) @@ -895,7 +892,7 @@ finish_thread_state (process_stratum_target *targ, ptid_t ptid) bool any_started = false; for (thread_info *tp : all_non_exited_threads (targ, ptid)) - if (set_running_thread (tp, tp->executing)) + if (set_running_thread (tp, tp->executing ())) any_started = true; if (any_started) @@ -922,7 +919,7 @@ validate_registers_access (void) at the prompt) when a thread is not executing for some internal reason, but is marked running from the user's perspective. E.g., the thread is waiting for its turn in the step-over queue. */ - if (tp->executing) + if (tp->executing ()) error (_("Selected thread is running.")); } @@ -940,7 +937,7 @@ can_access_registers_thread (thread_info *thread) return false; /* ... or from a spinning thread. FIXME: see validate_registers_access. */ - if (thread->executing) + if (thread->executing ()) return false; return true; @@ -1997,7 +1994,7 @@ update_threads_executing (void) for (thread_info *tp : inf->non_exited_threads ()) { - if (tp->executing) + if (tp->executing ()) { targ->threads_executing = true; return;