From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id fyNbGdGXL2E2RgAAWB0awg (envelope-from ) for ; Wed, 01 Sep 2021 11:10:09 -0400 Received: by simark.ca (Postfix, from userid 112) id 575121EE21; Wed, 1 Sep 2021 11:10:09 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.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 052E01ECEB for ; Wed, 1 Sep 2021 11:10:08 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8F7D73857022 for ; Wed, 1 Sep 2021 15:10:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8F7D73857022 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1630509007; bh=RxLxbW/ABJ3DsaFxyb9pd0mfnngkaHl0icPPLz14X0U=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=BUvL1iTxpDf6F/U17qQGTspZXo8/Hg1QToS+BRLvdxDnB/y3gEmVg+wbHUi9HWByC ylYIKf2tCCnbAte9ZOagEmm/Vz5gEnFdcQCY6uVF9oyeMLZx+qWiDNf+00ww8+ZzP7 JihbohbYq+oUxf6eXXfeo/g9Cimyd8ZfQQXlq1xw= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 64BFD3857411 for ; Wed, 1 Sep 2021 15:09:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 64BFD3857411 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 181F9VCi003140 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 1 Sep 2021 11:09:36 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 181F9VCi003140 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 41EAB1ECEB; Wed, 1 Sep 2021 11:09:31 -0400 (EDT) Subject: Re: [PATCH 3/3] gdb: make thread_info executing and resumed state more consistent To: Andrew Burgess , gdb-patches@sourceware.org References: <0491ec20c3c5f489f625383f756d3853a8e48074.1630353626.git.andrew.burgess@embecosm.com> Message-ID: <68c1d1c6-940c-f38d-4f95-5cb863f75938@polymtl.ca> Date: Wed, 1 Sep 2021 11:09:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <0491ec20c3c5f489f625383f756d3853a8e48074.1630353626.git.andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 1 Sep 2021 15:09:31 +0000 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: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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. > > 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? > @@ -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. > 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? Simon