From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id +A8XMcERS2FKbAAAWB0awg (envelope-from ) for ; Wed, 22 Sep 2021 07:21:37 -0400 Received: by simark.ca (Postfix, from userid 112) id C12291EE25; Wed, 22 Sep 2021 07:21:37 -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=unavailable 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 DCA201EDF0 for ; Wed, 22 Sep 2021 07:21:35 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 341B33858422 for ; Wed, 22 Sep 2021 11:21:35 +0000 (GMT) Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) by sourceware.org (Postfix) with ESMTPS id DA4233858D39 for ; Wed, 22 Sep 2021 11:21:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DA4233858D39 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-wr1-x42b.google.com with SMTP id t18so5875493wrb.0 for ; Wed, 22 Sep 2021 04:21:17 -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=wdEKh5W96F8FFzUpxtCYSXxrreA8rETW6iU4lwbmy3I=; b=C+cPXMxDmQpZd3TsAnggCNvPWUQYkFMn0LWMxEgNYaxXCrOpHXCPNNcZP/6p/Qo/Nj KaRM/MyHZzf6/4O/OgMPh6lcB7wSDLXt4nFs4dQIc8KuZ0/KWW8PdGBJ/eCKik/Exf9C P9yRqAjTig/w4AFR7UQJJ5NWzwFee907wMNFAcOIc8+lymszQDB699Q2Ye8a5VzBIGub UiVw2SHTJe+xVkd5V8sGnW9fHSDxDKKZBHdXM4nUOfvbAXOeVSQ1LJzF2Q7psuyO1SHs DIZqEmc5Pq2+RLxiQSSZy37ohBqX5QmmrDCKcRtG77AFTRlg4134+ytn8XH2hiCERDlN SxLg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wdEKh5W96F8FFzUpxtCYSXxrreA8rETW6iU4lwbmy3I=; b=Y+xXOPrkgW1aVubdtSy+jnWM6tTZ9JY5J2SyRM327CO0qKhkufeCzKVQtUTT4nLZNC ETSjmY8GXMphjueOzEc3FGwG+coF2ixIFysG6Ii2rFVYLqP+ERw0orUZzCGaiAiZJsy1 tl8XMRI/8A3WLlgKVwoIohooGgO8i60lSAd5Fbz+Cqf8+rgogzgKSGK/M5EbfxXXPlhG 3Un7ghALjD2VueLJB8jeUpxt6jpaUsy6oDPWspe/IsIelssWpF1A/PI67oL6GwFFQEHw TuLwGeJr8heTGKl5Z8iREncEyQEO03RADe2pdkWzxDYZ1nO9y7vAUInmZTZuKkOOa7Vw 3wjg== X-Gm-Message-State: AOAM532nYrTBJDZjg1gJ07PLVkfcsm+FOvMB+AgpQTtylnEM/Z+P5QXr TTftb8ntaq2JTI/Xc/DO6ZU/+D4M/zQQfw== X-Google-Smtp-Source: ABdhPJy1rWDqsF7alkF2MRtyJQhyear1dSXxtBe17zi6ck1Y6HjG7zI4oFLZJ79I4qRQTz0+xSWcPg== X-Received: by 2002:a1c:2289:: with SMTP id i131mr9837094wmi.113.1632309675498; Wed, 22 Sep 2021 04:21:15 -0700 (PDT) Received: from localhost (host86-153-58-62.range86-153.btcentralplus.com. [86.153.58.62]) by smtp.gmail.com with ESMTPSA id z17sm1618416wml.24.2021.09.22.04.21.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Sep 2021 04:21:15 -0700 (PDT) Date: Wed, 22 Sep 2021 12:21:14 +0100 From: Andrew Burgess To: gdb-patches@sourceware.org Subject: Re: [PATCH 3/3] gdb: make thread_info executing and resumed state more consistent Message-ID: <20210922112114.GF4950@embecosm.com> References: <0491ec20c3c5f489f625383f756d3853a8e48074.1630353626.git.andrew.burgess@embecosm.com> <68c1d1c6-940c-f38d-4f95-5cb863f75938@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <68c1d1c6-940c-f38d-4f95-5cb863f75938@polymtl.ca> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 11:57:16 up 3 days, 2:06, 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 11:09:31 -0400]: > > > On 2021-08-30 4:03 p.m., Andrew Burgess wrote: > > 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. > > Did you consider collapsing the two fields (resumed and executing) into > a single one, with three states? The names are silly, but as an > example: > > enum class thread_resumption_state > { > NOT_RESUMED, > RESUMED_BUT_NOT_TARGET_RESUMED, > RESUMED_AND_TARGET_RESUMED, > }; > > I think that would end up simpler than two booleans with rules. But > since it would probably be a pretty big change, I will accept "nope" as > an answer. I did think about this, but I guess I'm not quite sure how this ends up being any simpler than the two booleans with rules. My assumption is that we'd want to keep the API as executing/set_executing and resumed/set_resumed as this represents the questions we want to ask. As such something like set_executing would become: void thread_info::set_executing (bool e) { if (m_state == RESUMED_AND_TARGET_RESUMED) return; gdb_assert (m_state == RESUMED_BUT_NOT_TARGET_RESUMED); m_state = RESUMED_AND_TARGET_RESUMED; } Which doesn't feel any less complex to me. Of course, we could change the API so the user sets the state directly, like: void thread_info_::set_executing_resumed_state (enum state_type s) { /* ... */ } But then we'd _still_ end up having to assert the correct state transitions. Maybe I'm just not understanding what you're suggesting though... > > > > > 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. > > Should clear_stop_pc have the same assertions as set_stop_pc? I > guess you tried that and it didn't work? I've added these in the new patch. > > > @@ -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); > > + > > This one is in the TARGET_WAITKIND_EXECD handling... I'm wondering if it > should be target_ops::follow_exec's contract that any new thread of the > new inferior should be not resumed. Because follow_exec is always > called in a context where the event thread on entry is not resumed. So > it's not like new threads that are added in target_ops::wait, for > example. For those, it makes sense to be added as resumed. > > I think it would just mean adding a set_resumed (false) call in > process_stratum_target::follow_exec. And possibly some assertions in > the follow_exec free function. For example, I would add to the existing > assertions: > > /* If a new inferior was created, the target must have added at least > one thread. If the same inferior was used, we have left one > thread. */ > gdb_assert (!inf->thread_list.empty ()); > > /* Any added thread must have been added in the not resumed state. */ > for (thread_info *thread : inf->threads ()) > gdb_assert (!thread->resumed ()); > > PS: if taking into consideration my comment about the observer seeing > the correct resumed state, in add_thread_silent (below), then perhaps we > would need add_thread_silent to take a "resumed" parameter, so it can > set the right resumed state before calling the observer. I think I've addressed all of this in the new patch below. The thread coming out of ::follow_exec should now be non-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 ()); > > I don't know if it matters, but should resumed be set before calling the > new_thread observer? So that observers see the thread in the state we > intend it to be in? I checked, and as far as I can tell it doesn't currently matter, but I agree we should try to do the right thing here, so I've changed this in the new patch version. Below is V2 of this patch. There's a lot of change since V1. - The add_thread* calls now take a flag, and threads can be started as resumed or non-resumed. - Some of the state changes, like in ::follow_exec, are now pushed back to where the thread is created, as a result, some places where in V1 I was changing the state, I'm now asserting the state. - Added some asserts to clear_stop_pc One big consequence of this patch is that we now expect that threads are created in the "correct" resumed/non-resumed state. A place where this is definitely now wrong is during the initial creation of a process to debug - threads are created in the default resumed state, which is not correct. I've fixed this for the native Linux and remote targets, but for all other targets this will likely be incorrect. As I'm not able to test these targets I made the choice to, instead of asserting that GDB has the correct state, fix the state. This can be seen in gdb_startup_inferior (fork-child.c) and one in run_command_1 (infcmd.c), where I've added comments about this. I don't really see a way around this. If I make the default thread state be non-resumed then the inferior creation code would work, but any target specific code to handle creating new threads would potentially be wrong (as threads would be created non-resumed when they should be resumed). I could potentially just add the asserts and let all the other targets break; then offer help as/when these issues are reported. Thoughts on this welcome. Thanks, Andrew ---- commit 32b05d0b4f46070df5583bc8e411034da8dc4d5a Author: Andrew Burgess Date: Mon Aug 16 12:09:45 2021 +0100 gdb: make thread_info executing and resumed state more consistent 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, the goal of this commit is to add some 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 these 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. My initial approach was to retain the idea that threads are always created in the non-executing, non-resumed state, and then find all the places where new threads are created (which should be in the resumed state), and mark the thread resumed in that case. However, after looking at the changes for a while, it felt like it would be simpler to create threads as resumed by default and instead force the threads back to non-resumed in the few cases where this was appropriate. The appropriate case being (as far as I can tell), just the initial threads created as part of starting up a new inferior. So, that's what this patch does. The thread creation routines now take a flag to indicate if the new thread should be created resumed or not. Threads created as part of the ::create_inferior process should be created non-resumed, while all other threads are created resumed. The only problem is that this change requires updates to all targets that supply ::create_inferior, and I'm mostly only able to test GNU/Linux targets. And so, I have two changes, one in gdb_startup_inferior (fork-child.c) and one in run_command_1 (infcmd.c) where, instead of asserting that a thread is in a particular resumed/non-resumed state, I am setting the thread back to non-resumed. I believe that this should fix up GDB's state for any target that is not (yet) doing the correct thing when creating its initial threads. Here is a description of all the individual changes: * corelow.c (add_to_thread_list): When adding threads as part of setting up a core target, the threads should not be resumed. (core_target_open): When adding threads as part of setting up a core target, the threads should not be resumed. For sanity, assert that the threads of a core target are not resumed once the initial target has been setup. * fork-child.c (gdb_startup_inferior): Upon entry to this function no threads in the inferior should be resumed, however, I suspect not all targets comply with this assertion, so for now I don't actually assert anything. As we're about to resume the inferior though, mark all threads as resumed. * gdbthread.h (set_stop_pc): Add assert that the stop_pc can only be set when a thread is neither resumed, or executing. (clear_stop_pc): The stop_pc is cleared as part of setting the thread executing. And, as a thread is only set executing after it is marked resumed, we can add some asserts to this function to validate GDB's state. (add_thread): Update header comment, add extra 'resumed_p' parameter. (add_thread_silent): Add extra 'resumed_p' parameter. (add_thread_with_info): Add extra 'resumed_p' parameter. * inf-ptrace.c (inf_ptrace_target::create_inferior): The initial thread is created non-resumed. * infcmd.c (post_create_inferior): Only attempt to set the stop_pc once per-stop. I've updated the comment, and re-indented the code under the new if condition. (run_command_1): After creating the inferior all threads should be non-resumed, however, I suspect that not all targets comply with this assertion, so, instead of asserting this, I set all threads back to non-resumed to save any badly behaved targets. * infrun.c (follow_fork_inferior): After a fork all threads in the child should already have been marked as non-resumed. (follow_exec): Upon entry the thread performing the exec should already be marked as non-resumed. After the exec the selected thread should also be non-resumed. (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): Assert that the thread is not resumed when handling an exec event. (finish_step_over): 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. * process-stratum-target.c (process_stratum_target::follow_exec): If creating a new thread to handle the exec in, then it should be created non-resumed. (process_stratum_target::follow_fork): If creating a new thread to follow the child of a fork, the new thread should be non-resumed. * remote.c (remote_target::add_current_inferior_and_thread): Add resumed_p parameter. Use this parameter to decide if the new thread should be started resumed or not. (remote_target::remote_add_thread): Mark the thread resumed before marking it executed. (remote_target::process_initial_stop_replies): Mark the thread as non-resumed. (remote_target::start_remote): Create the initial thread in a resumed state. (extended_remote_target::create_inferior): Create the initial thread in a non-resumed state. * thread.c (add_thread_silent): Take a new resumed_p parameter, use this to set if the new thread is resumed or not. All new threads are created non-executing, so assert this. (add_thread_with_info): Pass through the new resumed_p parameter. (add_thread): Pass through the new resumed_p parameter. (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. Mark the thread executing after clearing the stop_pc, this allows the assertion we added to thread_info::clear_stop_pc above to work. (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. diff --git a/gdb/corelow.c b/gdb/corelow.c index 711e86c4cd4..d6846b8f609 100644 --- a/gdb/corelow.c +++ b/gdb/corelow.c @@ -347,7 +347,8 @@ add_to_thread_list (asection *asect, asection *reg_sect) ptid_t ptid (pid, lwpid); - thread_info *thr = add_thread (inf->process_target (), ptid); + /* Pass false to indicate the thread should be added non-resumed. */ + thread_info *thr = add_thread (inf->process_target (), ptid, false); /* Warning, Will Robinson, looking at BFD private data! */ @@ -505,7 +506,7 @@ core_target_open (const char *arg, int from_tty) if (thread == NULL) { inferior_appeared (current_inferior (), CORELOW_PID); - thread = add_thread_silent (target, ptid_t (CORELOW_PID)); + thread = add_thread_silent (target, ptid_t (CORELOW_PID), false); } switch_to_thread (thread); @@ -514,6 +515,10 @@ core_target_open (const char *arg, int from_tty) if (current_program_space->exec_bfd () == nullptr) locate_exec_from_corefile_build_id (core_bfd, from_tty); + /* Threads in a core file are not resumed. */ + for (thread_info *thread : current_inferior ()->threads ()) + gdb_assert (!thread->resumed ()); + post_create_inferior (from_tty); /* Now go through the target stack looking for threads since there diff --git a/gdb/fork-child.c b/gdb/fork-child.c index 3ce7d64b855..8890ea41e05 100644 --- a/gdb/fork-child.c +++ b/gdb/fork-child.c @@ -126,10 +126,24 @@ gdb_startup_inferior (pid_t pid, int num_traps) inferior *inf = current_inferior (); process_stratum_target *proc_target = inf->process_target (); + for (thread_info *thread : inf->threads ()) + { + /* Ideally, all targets would created their initial threads in the + non-resumed state, and only when we get here would we mark the + threads as resumed. However, this is not currently the case, some + targets create their initial threads in the resumed state (for no + particular reason other than historical), and so we can't assert + anything about the state of the inferior threads at this point. + + What we'd like to say is: gdb_assert (!thread->resumed ()); */ + thread->set_resumed (true); + } + ptid_t ptid = startup_inferior (proc_target, pid, num_traps, NULL, NULL); /* Mark all threads non-executing. */ set_executing (proc_target, ptid, false); + set_resumed (proc_target, ptid, false); return ptid; } diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 4b271943d50..9ffe9233c77 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -345,6 +345,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; } @@ -352,6 +354,8 @@ class thread_info : public refcounted_object, void clear_stop_pc () { + gdb_assert (m_resumed); + gdb_assert (!m_executing); m_suspend.stop_pc.reset (); } @@ -547,22 +551,25 @@ using inferior_ref /* Create an empty thread list, or empty the existing one. */ extern void init_thread_list (void); -/* Add a thread to the thread list, print a message - that a new thread is found, and return the pointer to - the new thread. Caller my use this pointer to - initialize the private thread data. */ +/* Add a thread to the thread list, print a message that a new thread is + found, and return the pointer to the new thread. Caller my use this + pointer to initialize the private thread data. When RESUMED_P is true + the thread is created in a resumed state, otherwise, the thread is + created in a non-resumed state. */ extern struct thread_info *add_thread (process_stratum_target *targ, - ptid_t ptid); + ptid_t ptid, bool resumed_p = true); /* Same as add_thread, but does not print a message about new thread. */ extern struct thread_info *add_thread_silent (process_stratum_target *targ, - ptid_t ptid); + ptid_t ptid, + bool resumed_p = true); /* Same as add_thread, and sets the private info. */ extern struct thread_info *add_thread_with_info (process_stratum_target *targ, ptid_t ptid, - private_thread_info *); + private_thread_info *, + bool resumed_p = true); /* Delete thread THREAD and notify of thread exit. If the thread is currently not deletable, don't actually delete it but still tag it diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c index afa38de6ef7..0366bb940e0 100644 --- a/gdb/inf-ptrace.c +++ b/gdb/inf-ptrace.c @@ -94,7 +94,7 @@ inf_ptrace_target::create_inferior (const char *exec_file, /* We have something that executes now. We'll be running through the shell at this point (if startup-with-shell is true), but the pid shouldn't change. */ - thread_info *thr = add_thread_silent (this, ptid); + thread_info *thr = add_thread_silent (this, ptid, false); switch_to_thread (thr); unpusher.release (); diff --git a/gdb/infcmd.c b/gdb/infcmd.c index d948f4bafc5..d31ba5af144 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -244,22 +244,29 @@ post_create_inferior (int from_tty) don't need to. */ target_find_description (); - /* Now that we know the register layout, retrieve current PC. But - if the PC is unavailable (e.g., we're opening a core file with - missing registers info), ignore it. */ + /* Now that we know the register layout, update the stop_pc if it is not + already set. If GDB attached to a running process then the stop_pc + will have been set while processing the stop events triggered during + the attach. If this is a core file, or we're just starting a new + process, then the stop_pc will not currently be set. + + But, if the PC is unavailable (e.g., we're opening a core file with + missing registers info), ignore it. Obviously, if we're trying to + debug a running process and we can't read the PC then this is bad and + shouldn't be ignored, but we'll soon hit errors trying to read the PC + elsewhere in GDB, so ignoring this here is fine. */ thread_info *thr = inferior_thread (); - - thr->clear_stop_pc (); - try - { - regcache *rc = get_thread_regcache (thr); - thr->set_stop_pc (regcache_read_pc (rc)); - } - catch (const gdb_exception_error &ex) - { - if (ex.error != NOT_AVAILABLE_ERROR) - throw; - } + if (!thr->stop_pc_p ()) + try + { + regcache *rc = get_thread_regcache (thr); + thr->set_stop_pc (regcache_read_pc (rc)); + } + catch (const gdb_exception_error &ex) + { + if (ex.error != NOT_AVAILABLE_ERROR) + throw; + } if (current_program_space->exec_bfd ()) { @@ -453,6 +460,19 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how) shouldn't refer to run_target again. */ run_target = NULL; + /* After creating the inferior its threads are not resumed. */ + for (thread_info *thread : current_inferior ()->threads ()) + { + /* Ideally, all targets would leave their threads non-resumed after + the create_inferior call above. Unfortunately, this is probably + not the case. The 'probably' here is simply due to an inability + to test all targets. So, rather than asserting that all threads + are non-resumed here, set the threads to be non-resumed. + + We'd like to say: gdb_assert (!thread->resumed ()); */ + thread->set_resumed (false); + } + /* We're starting off a new process. When we get out of here, in non-stop mode, finish the state of all threads of that process, but leave other threads alone, as they may be stopped in internal diff --git a/gdb/infrun.c b/gdb/infrun.c index d1ac9b4cbbb..ca4568ba272 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -580,7 +580,13 @@ holding the child stopped. Try \"set detach-on-fork\" or \ /* If there is a child inferior, target_follow_fork must have created a thread for it. */ if (child_inf != nullptr) - gdb_assert (!child_inf->thread_list.empty ()); + { + gdb_assert (!child_inf->thread_list.empty ()); + + /* Any added thread should have been marked non-resumed already. */ + for (thread_info *thread : child_inf->threads ()) + gdb_assert (!thread->resumed ()); + } /* Detach the parent if needed. */ if (follow_child) @@ -1091,6 +1097,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target) breakpoint or similar, it's gone now. We cannot truly step-to-next statement through an exec(). */ thread_info *th = inferior_thread (); + gdb_assert (!th->resumed ()); th->control.step_resume_breakpoint = NULL; th->control.exception_resume_breakpoint = NULL; th->control.single_step_breakpoints = NULL; @@ -1172,6 +1179,11 @@ follow_exec (ptid_t ptid, const char *exec_file_target) gdb_assert (current_inferior () == inf); gdb_assert (current_program_space == inf->pspace); + /* The inferior (and so thread) might have changed depending on which + path we took through the above code. However, the currently selected + thread should not be in a resumed state. */ + gdb_assert (!inferior_thread ()->resumed ()); + /* Attempt to open the exec file. SYMFILE_DEFER_BP_RESET is used because the proper displacement for a PIE (Position Independent Executable) main symbol file will only be computed by @@ -2100,6 +2112,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 +2166,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 +2209,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 +2371,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 +2579,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 +5680,10 @@ 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 switch threads above, then the new thread should still + be in the non-resumed state. */ + gdb_assert (!ecs->event_thread->resumed ()); + ecs->event_thread->set_stop_pc (regcache_read_pc (get_thread_regcache (ecs->event_thread))); @@ -5865,12 +5933,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 +5943,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 +7596,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/process-stratum-target.c b/gdb/process-stratum-target.c index 4c726419b99..b6871403317 100644 --- a/gdb/process-stratum-target.c +++ b/gdb/process-stratum-target.c @@ -100,7 +100,7 @@ process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid, may decide to unpush itself from the original inferior's target stack after that, at its discretion. */ follow_inf->push_target (orig_inf->process_target ()); - thread_info *t = add_thread (follow_inf->process_target (), ptid); + thread_info *t = add_thread (follow_inf->process_target (), ptid, false); /* Leave the new inferior / thread as the current inferior / thread. */ switch_to_thread (t); @@ -118,7 +118,7 @@ process_stratum_target::follow_fork (inferior *child_inf, ptid_t child_ptid, if (child_inf != nullptr) { child_inf->push_target (this); - add_thread_silent (this, child_ptid); + add_thread_silent (this, child_ptid, false); } } diff --git a/gdb/remote.c b/gdb/remote.c index b6da6b086a2..bb96149f70f 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -751,7 +751,8 @@ class remote_target : public process_stratum_target int remote_resume_with_vcont (ptid_t ptid, int step, gdb_signal siggnal); - thread_info *add_current_inferior_and_thread (const char *wait_status); + thread_info *add_current_inferior_and_thread (const char *wait_status, + bool resumed_p); ptid_t wait_ns (ptid_t ptid, struct target_waitstatus *status, target_wait_flags options); @@ -2546,8 +2547,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; } @@ -4440,7 +4442,8 @@ remote_target::get_current_thread (const char *wait_status) The function returns pointer to the main thread of the inferior. */ thread_info * -remote_target::add_current_inferior_and_thread (const char *wait_status) +remote_target::add_current_inferior_and_thread (const char *wait_status, + bool resumed_p) { struct remote_state *rs = get_remote_state (); bool fake_pid_p = false; @@ -4471,7 +4474,7 @@ remote_target::add_current_inferior_and_thread (const char *wait_status) /* Add the main thread and switch to it. Don't try reading registers yet, since we haven't fetched the target description yet. */ - thread_info *tp = add_thread_silent (this, curr_ptid); + thread_info *tp = add_thread_silent (this, curr_ptid, resumed_p); switch_to_thread_no_regs (tp); return tp; @@ -4586,6 +4589,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 (); } @@ -4841,7 +4845,8 @@ remote_target::start_remote (int from_tty, int extended_p) /* Target has no concept of threads at all. GDB treats non-threaded target as single-threaded; add a main thread. */ - thread_info *tp = add_current_inferior_and_thread (wait_status); + thread_info *tp + = add_current_inferior_and_thread (wait_status, true); get_remote_thread_info (tp)->set_resumed (); } else @@ -10488,7 +10493,7 @@ Remote replied unexpectedly while setting startup-with-shell: %s"), /* vRun's success return is a stop reply. */ stop_reply = run_worked ? rs->buf.data () : NULL; - add_current_inferior_and_thread (stop_reply); + add_current_inferior_and_thread (stop_reply, false); /* Get updated offsets, if the stub uses qOffsets. */ get_offsets (); diff --git a/gdb/thread.c b/gdb/thread.c index 10c3dcd6991..a74ba94bbe8 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -245,7 +245,7 @@ new_thread (struct inferior *inf, ptid_t ptid) } struct thread_info * -add_thread_silent (process_stratum_target *targ, ptid_t ptid) +add_thread_silent (process_stratum_target *targ, ptid_t ptid, bool resumed_p) { gdb_assert (targ != nullptr); @@ -260,6 +260,14 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid) delete_thread (tp); tp = new_thread (inf, ptid); + + /* Upon creation, all threads are non-executing, and non-resumed. Before + notifying observers of the new thread, we set the resumed flag to the + desired value. */ + gdb_assert (!tp->executing ()); + tp->set_resumed (resumed_p); + + /* Announce the new thread to all observers. */ gdb::observers::new_thread.notify (tp); return tp; @@ -267,9 +275,9 @@ add_thread_silent (process_stratum_target *targ, ptid_t ptid) struct thread_info * add_thread_with_info (process_stratum_target *targ, ptid_t ptid, - private_thread_info *priv) + private_thread_info *priv, bool resumed_p) { - thread_info *result = add_thread_silent (targ, ptid); + thread_info *result = add_thread_silent (targ, ptid, resumed_p); result->priv.reset (priv); @@ -281,9 +289,9 @@ add_thread_with_info (process_stratum_target *targ, ptid_t ptid, } struct thread_info * -add_thread (process_stratum_target *targ, ptid_t ptid) +add_thread (process_stratum_target *targ, ptid_t ptid, bool resumed_p) { - return add_thread_with_info (targ, ptid, NULL); + return add_thread_with_info (targ, ptid, NULL, resumed_p); } private_thread_info::~private_thread_info () = default; @@ -322,9 +330,15 @@ thread_info::deletable () const void thread_info::set_executing (bool executing) { - m_executing = executing; + if (executing == m_executing) + return; + + gdb_assert (m_resumed); + if (executing) this->clear_stop_pc (); + + m_executing = executing; } /* See gdbthread.h. */ @@ -335,6 +349,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