From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 1OZ0NjWdYGG6IAAAWB0awg (envelope-from ) for ; Fri, 08 Oct 2021 15:34:13 -0400 Received: by simark.ca (Postfix, from userid 112) id CDBF21EE20; Fri, 8 Oct 2021 15:34:13 -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 960781E79C for ; Fri, 8 Oct 2021 15:34:12 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1BA213858012 for ; Fri, 8 Oct 2021 19:34:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1BA213858012 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1633721652; bh=4muJJSx8snpw6fN42oSg30N/BE5nD27Caq5dIa4+NsA=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=mok7uciYsI+Ge+h2bsn/6YjbbQu+Mje1kUUe3B3HQ47F6JQ5qbb0ey/DoASDuHwsq RpBay1rnxdXvYuLSWFQdBJrs9Kr1amPG+5IugoYwFGgnPDf4yLkpybPwPhjK4fla3S XrXEe8urzuXWeQ8I5Fln1yCu1ZeyvmA9Vm3DoZzo= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id C65F73858400 for ; Fri, 8 Oct 2021 19:33:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C65F73858400 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 198JXhEQ028065 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 8 Oct 2021 15:33:48 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 198JXhEQ028065 Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 487591E79C; Fri, 8 Oct 2021 15:33:43 -0400 (EDT) Subject: Re: [PATCH 3/3] gdb: make thread_info executing and resumed state more consistent To: Andrew Burgess References: <0491ec20c3c5f489f625383f756d3853a8e48074.1630353626.git.andrew.burgess@embecosm.com> <68c1d1c6-940c-f38d-4f95-5cb863f75938@polymtl.ca> <20210922112114.GF4950@embecosm.com> <36212932-3978-3caa-30aa-b426a1e20bc8@polymtl.ca> <20210929080958.GJ1900093@embecosm.com> Message-ID: Date: Fri, 8 Oct 2021 15:33:42 -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: <20210929080958.GJ1900093@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 8 Oct 2021 19:33:43 +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 Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 2021-09-29 4:09 a.m., Andrew Burgess wrote: > * Simon Marchi [2021-09-23 13:14:33 -0400]: > >>> 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. >> >> The later is what I was thinking. >> >> What do you mean "assert the correct state transitions"? With the enum >> all possible transitions are valid. I'm not sure if >> RESUMED_AND_TARGET_RESUMED -> RESUMED_BUT_NOT_TARGET_RESUMED is actually >> used, but it's not something prevented by your patch either. > > The RESUMED_AND_TARGET_RESUMED -> RESUMED_BUT_NOT_TARGET_RESUMED > transition is definitely used, and is, I believe required with my > patch. > > I don't know if there are many places in GDB that we actually sit in > the RESUMED_BUT_NOT_TARGET_RESUMED state for long (that would > currently be resumed=true,executing=false, right?) but my intention is > that we have to go back through that state, i.e. we can't set resumed > to false while executing is true. The most prominent case I know of is when resuming threads that have a pending status (an event that was pulled out of the target, but not consumed, instead saved in the thread_info structure). When resuming such a thread, infrun just pretends to resume it: it marks it as resumed=true, but does not call target_ops::resume on it, and therefore leaves it at executing=false (this is where I think the terminology is confusing). We will go back to the event loop, and infrun will go "oh! a thread just stopped!" and consume that event. That will make the thread go back to resumed=false,executing=false. > From you original proposed states: > > enum class thread_resumption_state > { > NOT_RESUMED, /* 1 */ > RESUMED_BUT_NOT_TARGET_RESUMED, /* 2 */ > RESUMED_AND_TARGET_RESUMED, /* 3 */ > }; > > Under the current scheme the transition order should always be: > > 1 -> 2 -> 3 -> 2 -> 1 > > Anything else should trigger an assertion. I think that would be fine, but I don't see the value. The 3 -> 1 transition happens when target_ops::wait returns an event for a thread. The thread is no longer executing on the target, and no longer resumed in the eyes of infrun. So we could go 3 -> 2 -> 1 but that just seems like an extra step. > Ideally, if I transitioned to the new scheme I'd (at least initially) > want to keep assertions that force that transition scheme, so no > transitions 1->3 or 3->1. > > However, there are places where set_resumed and set_executing are > right next to each other. > > So maybe they would be merged into a single set_state call, which > means the 1->3 or 3->1 transitions would be legal. So, then we remove > the asserts and allow any state transitions. > > But then, if we have a bug which GDB skips the logic that used to be > responsible for "set_resumed" we'd never know, right? When we hit the > old set_executing call, this will now just set the state as if the > resumed had happened... > > ...this seems less good to me. If resumed and executing are kept as separate states, then yeah the transition needs to go through 2, as it's not possible to go from resumed=true,executing=true to resume=false,executing=false atomically. > I guess given you've said you're not against what I have then for now > I'd prefer to stick with the separate resumed/executing. We can > always change later - I think it would be best done in a separate > patch anyway - I just want to try and explain my thinking. Yeah, that's fine with me. Of course, I'd like if Pedro could take a look, but he has a gazillion things to do. And, food for future thoughts / changes, while reading about the 3 -> 2 -> 1 vs 3 -> 1 transition, I thought: it makes sense for infrun to manage the resumed flag, because it's the "owner" of that flag. That flag means whether infrun considers this thread is running (regardless of whether this thread is actually executing on target or infrun just pretends it is running, for its own internal pending status logic). But the executing flag means "has the target been asked to resume execution for this thread and hasn't yet reported an event implying that this thread's execution on the target has halted". So it seems to me that the management of the executing flag could be done entirely in the target_ops API frontier, in target_resume and target_wait. target_resume already sets executing=true, which makes sense: we ask the target to resume execution of the thread. I think that the mirror operation could be done in target_wait. Let's say a target reports a TARGET_WAITKIND_STOPPED for a thread, target_wait would mark this thread as executing=false, because the target has reported an event that implies that execution of that thread has stopped. Basically, I'm saying that the job of mark_non_executing_threads should maybe be done by target_wait, except the set_resumed call at the end (that is still infrun's job). That would avoid the possibility of pulling an event out of the target and forgetting to set executing=false, leading to problems later. And if doing so, the transition would always naturally be 3 -> 2 -> 1, because 3 -> 2 would be done by the target API, and later 2 -> 1 would be done by infrun. Simon