From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 0G6jHcY5LWHSeAAAWB0awg (envelope-from ) for ; Mon, 30 Aug 2021 16:04:22 -0400 Received: by simark.ca (Postfix, from userid 112) id 774AF1EE1C; Mon, 30 Aug 2021 16:04:22 -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.5 required=5.0 tests=DKIM_SIGNED, MAILING_LIST_MULTI,RDNS_DYNAMIC,T_DKIM_INVALID,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 0EF661E813 for ; Mon, 30 Aug 2021 16:04:21 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CB5A73858D35 for ; Mon, 30 Aug 2021 20:04:20 +0000 (GMT) Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by sourceware.org (Postfix) with ESMTPS id D58E03858407 for ; Mon, 30 Aug 2021 20:03:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D58E03858407 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-x32a.google.com with SMTP id d22-20020a1c1d16000000b002e7777970f0so268558wmd.3 for ; Mon, 30 Aug 2021 13:03:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=cd0NygEH49H1v+ZWg8YfckPVvqM1BdvcgzqGy3KoiCg=; b=NLUmmfQX6k/uyGVLNmzOy1FzUbP2SgeOa2/FUFOlP7pYSycMUDWp2u+fsVtN7Egybg tTZJ2KiN3oW9/Xio8iGqYUfSHPT2CIGQPKKhBit/Af8il4lnRLhqtvNoLxxfQDDwzDxc 1Q9QEugwVx+oOCGuGfIQZ93MB6q+lf2iywffWYPriMWmbTwlKEAkmGbXWd0OUACSGvbl YdK91wz6emFJMiFUZD5RxBFyBNleJKwvvSXS1FaoL2q3gfYNnfbFcTe7gbBcyAwX2q2H g/uNj17WGT+1mFsf5qfsomrYafxV+IXEA4R7TO8zEYFApp9H9qO2bZuzkoTC7Nl7oQwA ycEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=cd0NygEH49H1v+ZWg8YfckPVvqM1BdvcgzqGy3KoiCg=; b=VuugP/MIeb8HbpZdTfcFkJeD66ozwXszsGLGSzMWBoLXEew7NoAirL8BQHJ60SxeQH XrdHR4VtTsDjppvLlKvCZUoAZff+nPeRkhwnF3E3WBw37Wvyg6sDjA+PLbI53oR2TDVO +bTE3c/RYnxHk2i6kTRubPgbKv84bAZwOYZAccI1A5DUaYEnVn031b0emESp6cZyKZP1 x/Lgq42a2HSF64sQUI8eUPY+x0eTpwGv7H4bm22Ql6+R6BYdpD1mA6VVH2mMHeILWYaZ 69U4LjIJ5YJYz2FHgmaU6zSqUbVpup2yC7jiHFHzkL1bj1GdNA1fvYxEtBH2U38P04J5 4SbA== X-Gm-Message-State: AOAM533nV4WbaOdD4SIg4C9dep48vsUfn59X/EjGjUfRdBKtcsZHGbLG e3Jb9Gtob2VQWV6urJdhhhysnqPe4WQIfg== X-Google-Smtp-Source: ABdhPJwMcxpAKt5N32X2k+3z2maOmy60RekUxANc5CbV2ZlIuRRCYm+FIRik991hGOlS6C4bPR+HzA== X-Received: by 2002:a7b:c843:: with SMTP id c3mr701408wml.76.1630353797489; Mon, 30 Aug 2021 13:03:17 -0700 (PDT) Received: from localhost (host86-188-49-44.range86-188.btcentralplus.com. [86.188.49.44]) by smtp.gmail.com with ESMTPSA id v28sm16215242wrv.93.2021.08.30.13.03.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Aug 2021 13:03:17 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCH 3/3] gdb: make thread_info executing and resumed state more consistent Date: Mon, 30 Aug 2021 21:03:11 +0100 Message-Id: <0491ec20c3c5f489f625383f756d3853a8e48074.1630353626.git.andrew.burgess@embecosm.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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" This commit was inspired by this comment from Simon: https://sourceware.org/pipermail/gdb-patches/2021-July/180694.html The comment is about two thread_info flags m_executing and m_resumed. The m_resumed flag indicates that at the infrun level GDB has set the thread running, while the m_executing flag indicates that the thread is actually running at the target level. It is very common that a thread can be m_resumed, but not m_executing, that is, core GDB thinks the thread is, or should be, running, but at the target level the thread isn't currently running. The comment Simon made was in reply to a patch I posted where I found a case where a thread was marked m_executing, but not m_resumed, that is, at the infrun level GDB thought the thread was stopped, but at the target level the thread was actually running. Simon suggests that this feels like an invalid state, and after thinking about it, I agree. So, this commit adds some new asserts to GDB. The core idea here is that the resumed and executing flags should only change in the following pattern, to begin with everything is set false: Resumed: false Executing: false Then infrun marks the thread resumed: Resumed: true Executing: false Then a target starts the thread executing: Resumed: true Executing: true The thread stops, the target spots this and marks the thread no longer executing: Resumed: true Executing: false And finally, infrun acknowledges that the thread is now no longer resumed: Resumed: false Executing: false Notice that at no point is resumed false, while executing is true. And so we can add some asserts: 1. The resumed flag should only change state when the executing flag is false, and 2. The executing flag should only change state when the resumed flag is true. I added these asserts and .... .... it turns out these rules are broken all over the place in GDB, we have problems like: (a) When new threads appear they are marked executing, but not resumed, and (b) In some places we mark a single thread as resumed, but then actually set multiple threads executing. For (a) it could be argued that this is a legitimate state - this is actually the problem I addressed in the original patch that Simon was replying too, however, we don't need to support this as a separate state, so if we can make this case follow the expected set of state transitions then it allows us to reduce the number of states that GDB can be in, which I think is a good thing. Case (b) seems to just be a bug to me. The interesting changes in this commit then are: * gdbthread.h (set_stop_pc): Add assert that the stop_pc can only be set when a thread is neither resumed, or executing. * infcmd.c (post_create_inferior): Ensure thread is non-resumed before clearing the stop_pc. * infrun.c (struct scoped_mark_thread_resumed): This new class is used to ensure that all the required threads are marked resumed when required, this addresses issue (b) above. I make use of this new class in... (do_target_resume): Use scoped_mark_thread_resumed to mark all threads resumed prior to actually calling into the target to resume the threads. Placing this call here allows me to remove some calls to thread_info::set_resumed() in other places... (resume_1): Remove a call to thread_info::set_resumed() from here. (handle_inferior_event): Mark the thread as non-resumed prior to setting the stop_pc, this thread is stopping. Additionally, when we need to place a thread back into the resumed state so that we can later find its pending event, we must mark the thread resumed after setting the stop_pc, see the asserts added to set_stop_pc for why. (keep_going_stepped_thread): Remove a call to set_resumed thanks to our changes in do_target_resume. * remote.c (remote_target::remote_add_thread): Mark the new thread as resumed. (remote_target::process_initial_stop_replies): Mark the thread as non-resumed. * thread.c (add_thread_silent): New threads are now created in the resumed state. This reflects the reality of how GDB thinks about new threads, they appear in a running state, and we then process a stop event from (or about) the thread, and bring the thread to a stop. As we most often first see the thread while processing the stop event then it makes sense (I think) that the thread starts as resumed, but not executing, as core GDB should consider the thread resumed at a high level, but in reality the thread is most likely stopped due to the event we are currently processing. (thread_info::set_executing): Add an early exit patch like we already have in thread_info::set_executing(). Also add the assertion that is one of the two core asserts for this patch. (thread_info::set_resumed): Add the other core assert for this patch. This series has been tested on X86-64 GNU/Linux with the unix, native-gdbserver, and native-extended-gdbserver boards. --- gdb/gdbthread.h | 2 ++ gdb/infcmd.c | 1 + gdb/infrun.c | 70 ++++++++++++++++++++++++++++++++++++++++++------- gdb/remote.c | 4 ++- gdb/thread.c | 14 ++++++++++ 5 files changed, 81 insertions(+), 10 deletions(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 245445a859b..120315ca25f 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -336,6 +336,8 @@ class thread_info : public refcounted_object, void set_stop_pc (CORE_ADDR stop_pc) { + gdb_assert (!m_resumed); + gdb_assert (!m_executing); m_suspend.stop_pc = stop_pc; } diff --git a/gdb/infcmd.c b/gdb/infcmd.c index d948f4bafc5..34924e17abe 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -249,6 +249,7 @@ post_create_inferior (int from_tty) missing registers info), ignore it. */ thread_info *thr = inferior_thread (); + thr->set_resumed (false); thr->clear_stop_pc (); try { diff --git a/gdb/infrun.c b/gdb/infrun.c index 5554523a049..07847ade325 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2100,6 +2100,52 @@ internal_resume_ptid (int user_step) return user_visible_resume_ptid (user_step); } +/* Before calling into target code to set inferior threads executing we + must mark all threads as resumed. If an exception is thrown while + trying to set the threads executing then we should mark the threads as + non-resumed. + + Create an instance of this struct before */ +struct scoped_mark_thread_resumed +{ + /* Constructor. All threads matching PTID will be marked as resumed. */ + scoped_mark_thread_resumed (process_stratum_target *targ, ptid_t ptid) + : m_target (targ), m_ptid (ptid) + { + gdb_assert (m_target != nullptr); + set_resumed (m_target, m_ptid, true); + } + + /* Destructor. If M_TARGET is not nullptr then mark all threads matching + M_PTID as no longer being resumed. The expectation is that on the + exception path this will be called with M_TARGET still set to a valid + target. If however, the threads were successfully set executing then + this->commit() will have been called, and M_TARGET will now be + nullptr. */ + ~scoped_mark_thread_resumed () + { + if (m_target != nullptr) + set_resumed (m_target, m_ptid, false); + } + + /* Called once all of the threads have successfully be set executing (by + calling into the target code). Clears M_TARGET as an indication that, + when this object is destructed, we should leave all matching threads + as being marked resumed. */ + void commit () + { + m_target = nullptr; + } + +private: + + /* The target used for marking threads as resumed or non-resumed. */ + process_stratum_target *m_target; + + /* The thread (or threads) to mark as resumed. */ + ptid_t m_ptid; +}; + /* Wrapper for target_resume, that handles infrun-specific bookkeeping. */ @@ -2108,6 +2154,11 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig) { struct thread_info *tp = inferior_thread (); + /* Create a scoped_mark_thread_resumed to mark all threads matching + RESUME_PTID as resumed. */ + process_stratum_target *curr_target = current_inferior ()->process_target (); + scoped_mark_thread_resumed scoped_resume (curr_target, resume_ptid); + gdb_assert (!tp->stop_requested); /* Install inferior's terminal modes. */ @@ -2146,6 +2197,9 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig) if (target_can_async_p ()) target_async (1); + + /* Call commit so SCOPED_RESUME leaves threads marked as resumed. */ + scoped_resume.commit (); } /* Resume the inferior. SIG is the signal to give the inferior @@ -2305,7 +2359,6 @@ resume_1 (enum gdb_signal sig) resume_ptid = internal_resume_ptid (user_step); do_target_resume (resume_ptid, false, GDB_SIGNAL_0); - tp->set_resumed (true); return; } } @@ -2514,7 +2567,6 @@ resume_1 (enum gdb_signal sig) } do_target_resume (resume_ptid, step, sig); - tp->set_resumed (true); } /* Resume the inferior. SIG is the signal to give the inferior @@ -5616,6 +5668,9 @@ handle_inferior_event (struct execution_control_state *ecs) execd thread for that case (this is a nop otherwise). */ ecs->event_thread = inferior_thread (); + /* If we did select a new thread, make sure its non-resumed. */ + ecs->event_thread->set_resumed (false); + ecs->event_thread->set_stop_pc (regcache_read_pc (get_thread_regcache (ecs->event_thread))); @@ -5865,12 +5920,6 @@ finish_step_over (struct execution_control_state *ecs) /* Record the event thread's event for later. */ save_waitstatus (tp, &ecs->ws); - /* This was cleared early, by handle_inferior_event. Set it - so this pending event is considered by - do_target_wait. */ - tp->set_resumed (true); - - gdb_assert (!tp->executing ()); regcache = get_thread_regcache (tp); tp->set_stop_pc (regcache_read_pc (regcache)); @@ -5881,6 +5930,10 @@ finish_step_over (struct execution_control_state *ecs) target_pid_to_str (tp->ptid).c_str (), currently_stepping (tp)); + /* This was cleared early, by handle_inferior_event. Set it so + this pending event is considered by do_target_wait. */ + tp->set_resumed (true); + /* This in-line step-over finished; clear this so we won't start a new one. This is what handle_signal_stop would do, if we returned false. */ @@ -7530,7 +7583,6 @@ keep_going_stepped_thread (struct thread_info *tp) get_frame_address_space (frame), tp->stop_pc ()); - tp->set_resumed (true); resume_ptid = internal_resume_ptid (tp->control.stepping_command); do_target_resume (resume_ptid, false, GDB_SIGNAL_0); } diff --git a/gdb/remote.c b/gdb/remote.c index b6da6b086a2..0cfed0bdf65 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -2546,8 +2546,9 @@ remote_target::remote_add_thread (ptid_t ptid, bool running, bool executing) when we process a matching stop reply. */ get_remote_thread_info (thread)->set_resumed (); - set_executing (this, ptid, executing); set_running (this, ptid, running); + set_resumed (this, ptid, running); + set_executing (this, ptid, executing); return thread; } @@ -4586,6 +4587,7 @@ remote_target::process_initial_stop_replies (int from_tty) evthread->set_pending_waitstatus (ws); set_executing (this, event_ptid, false); + set_resumed (this, event_ptid, false); set_running (this, event_ptid, false); get_remote_thread_info (evthread)->set_not_resumed (); } diff --git a/gdb/thread.c b/gdb/thread.c index 10c3dcd6991..aceb233be80 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -262,6 +262,13 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid) tp = new_thread (inf, ptid); gdb::observers::new_thread.notify (tp); + /* All threads are created in a resumed state, that is as soon as GDB + sees a new thread it is expected to be running as far as the core of + GDB is concerned. At a target level the thread is probably stopped + right now, hence the executing flag is left initialized to false. */ + tp->set_resumed (true); + gdb_assert (!tp->executing ()); + return tp; } @@ -322,6 +329,11 @@ thread_info::deletable () const void thread_info::set_executing (bool executing) { + if (executing == m_executing) + return; + + gdb_assert (m_resumed); + m_executing = executing; if (executing) this->clear_stop_pc (); @@ -335,6 +347,8 @@ thread_info::set_resumed (bool resumed) if (resumed == m_resumed) return; + gdb_assert (!m_executing); + process_stratum_target *proc_target = this->inf->process_target (); /* If we transition from resumed to not resumed, we might need to remove -- 2.25.4