From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id mPMYJIwfVGFuJwAAWB0awg (envelope-from ) for ; Wed, 29 Sep 2021 04:10:52 -0400 Received: by simark.ca (Postfix, from userid 112) id 913481EE27; Wed, 29 Sep 2021 04:10:52 -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.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,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 D95B31EDF7 for ; Wed, 29 Sep 2021 04:10:51 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 12A37385802E for ; Wed, 29 Sep 2021 08:10:51 +0000 (GMT) Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by sourceware.org (Postfix) with ESMTPS id 1B195385800C for ; Wed, 29 Sep 2021 08:10:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1B195385800C 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-x334.google.com with SMTP id f78-20020a1c1f51000000b0030cdb3d6079so1054376wmf.3 for ; Wed, 29 Sep 2021 01:10:01 -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=RHYpFmQCGDFEcvkNqWxsebctN7yN2Qjax3HaTmhsQHg=; b=C0zrQTIv1bN4SemVe82I+JAF1Vs9jM1Rf8F+ha7osO1daNGxTDAeYBIuyYDdf9kSlQ NiBb4428VS2pCO6dhja+DJzZWw5/nNgEdJyuNPw/+vpPIFNTs69unYEYYM5ReJjo1g0U 61wEdIEtAxNg9jg4AcghEbNz82R+K3MQ2bRmjrlAE1WBonHOvFu3R07ys2+5xkcqMvVM Kr4NMr3xnd8VGr5DwzsLLo1R8me380uLwzHFpm6ClHmcFrMaf75AeJuTD7bSjOWPMX9o 9FQgKYgBOemc8wHxU6i+IlYUaPok4Ph+ci+dte0XOE1TRi5fvzSa5r4hoS/AMQBJ2VPz hHTw== 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=RHYpFmQCGDFEcvkNqWxsebctN7yN2Qjax3HaTmhsQHg=; b=se2VVyagvi+yGHkhTdV+Vnom2jDN6v0uihwGz1Fk+oAuvKb7CW7X87hqyFCoqe397C 8f614ZvNA4KBaexI6jYvakYeYVsrwedjVhMTIcT+NoWbItAOz/ppraAMpIg//mIzhWxY KJ+YMgE6iV7Ho1ii9gzdiDRz8Fum/3EDgqoS/3KVzXoxCsS4dl6UhhA1dOhdWciQsozX OHYH3YA2DyuD7U2SXt/+LeQ7V+jLcsoOrbscK8WYGB17pXvVGL77/53vaKmBGA5xw0Nt llNpM/5P+9/1sDTCPa/q6p9WigcbzrU/29idVY8hy+Mz414mzy5Yfdv2uxSdAxqAHsmD w6Mg== X-Gm-Message-State: AOAM533rY0JvskLnPW7Vhcv1aOwbUuZ5+oIMfOh3+v5bzlw/btVTdQLk b03shsimy7fcc1K9OvtGsp8w4YSmVkGBug== X-Google-Smtp-Source: ABdhPJwnsM/twS5GbhY49jLeAl8sNl2RtaEk0UFdlgtHTrOSz2sKV0Umc6A4A0dC7ZAf1XUKK9cjSw== X-Received: by 2002:a1c:7302:: with SMTP id d2mr8969401wmb.92.1632902999774; Wed, 29 Sep 2021 01:09:59 -0700 (PDT) Received: from localhost (host86-169-137-11.range86-169.btcentralplus.com. [86.169.137.11]) by smtp.gmail.com with ESMTPSA id g13sm771696wmh.20.2021.09.29.01.09.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Sep 2021 01:09:59 -0700 (PDT) Date: Wed, 29 Sep 2021 09:09:58 +0100 From: Andrew Burgess To: Simon Marchi Subject: Re: [PATCH 3/3] gdb: make thread_info executing and resumed state more consistent Message-ID: <20210929080958.GJ1900093@embecosm.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <36212932-3978-3caa-30aa-b426a1e20bc8@polymtl.ca> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 08:54:16 up 9 days, 23:03, 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: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" * 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. >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. 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. 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. Thanks for all your other feedback. I'll go through and try and address all your points and post a new version of the patch. Thanks, Andrew > > 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