From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id +8LXGri5TGG1GwAAWB0awg (envelope-from ) for ; Thu, 23 Sep 2021 13:30:32 -0400 Received: by simark.ca (Postfix, from userid 112) id 59D8B1EE25; Thu, 23 Sep 2021 13:30:32 -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 09AA21EE14 for ; Thu, 23 Sep 2021 13:30:31 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A6F973858430 for ; Thu, 23 Sep 2021 17:30:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A6F973858430 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1632418229; bh=KAt+nY/wA4AauaV0iTlSGZsb47c5sOikgVPnmXJKxDg=; 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=I/SdQOcLlIAk25jamwUubPKdWQMqlDufH2j0EOCjymxLajfZOSgsMcqvE7JEaXman Sm4lTFtDGEIElfR9GdI94IPKWVujKk6+LQyCzi5F941eXK8Wq3lcKR5gqvX9Y5k6cs 4p7O6i9byU1exhiCdbrDmkTNulGqqc16m27tkMPI= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 399733857C4A for ; Thu, 23 Sep 2021 17:14:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 399733857C4A 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 18NHEYSB001474 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 23 Sep 2021 13:14:38 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 18NHEYSB001474 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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id CD6951EE14; Thu, 23 Sep 2021 13:14:33 -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> <68c1d1c6-940c-f38d-4f95-5cb863f75938@polymtl.ca> <20210922112114.GF4950@embecosm.com> Message-ID: <36212932-3978-3caa-30aa-b426a1e20bc8@polymtl.ca> Date: Thu, 23 Sep 2021 13:14:33 -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: <20210922112114.GF4950@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 Thu, 23 Sep 2021 17:14:34 +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" > 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. Note that I'm still ok with how you are doing things, I'm just discussing this to know whether we would want this as follow-up work or not (if somebody is interested in implementing it). > - 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'm not a fan of the default parameter values in add_thread and friends. I'd prefer having to pass an explicit value at each call site (there aren't that many), that would force thinking about what to pass at each call site. For targets you can't build and test... I'd say do a best effort to update them (update calls to add_thread* functions) and then use assertions. I much prefer a clear and consistent set of preconditions/postconditions to the target interface than a state where some targets do one thing and other targets do something else. That's easier to work with in the long run. And ideally, these would be documented in the target interface. For example, the documentation for target_ops::attach should say "threads created by the target during attach should be marked as (resumed|non-resumed)". If somebody hits one of your assertions, because a target is not respecting this postcondition, it will be quite obvious. > @@ -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 ()); Here, I suggested using a loop over all of current inferior's threads. It sounds a bit silly, but I'm thinking that some weird platform some day might have processes with two threads when coming out of an exec. Alternatively, we could assert that the current inferior contains exactly one thread. > + > /* 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 */ Seems like this comment was not finished. > +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. */ I'd say "If the object was not committed" instead of "If M_TARGET is not nullptr". M_TARGET being nullptr to represent the commit is an internal detail, and this comment is supposed to describe the interface. > + ~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. */ Same idea here. You could put the "Clear M_TARGET..." part inside the method body (to describe the implementation), while the comment above the method can describe the interface (e.g. "Calling commit makes it so threads are left as resumed on destruction."). Simon