From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 96E19385DC1B for ; Wed, 15 Apr 2020 14:46:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 96E19385DC1B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (unknown [192.222.164.54]) (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 9BC5A1E5F9; Wed, 15 Apr 2020 10:46:10 -0400 (EDT) Subject: Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412) To: Pedro Alves , gdb-patches@sourceware.org References: <20200414175434.8047-1-palves@redhat.com> From: Simon Marchi Message-ID: <89e862fa-678d-7b64-6776-c9179fce6ba6@simark.ca> Date: Wed, 15 Apr 2020 10:46:09 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200414175434.8047-1-palves@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Wed, 15 Apr 2020 14:46:12 -0000 On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote: > In PR/25412, Simon noticed that after the multi-target series, the > tid-reuse.exp testcase manages to create a duplicate thread in the > thread list. Or rather, two threads with the same PTID. > > This in turn exposes a design problem in GDB. The inferior_thread() > function looks up the current thread based on inferior_ptid: > > struct thread_info* > inferior_thread (void) > { > struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid); > gdb_assert (tp); > return tp; > } > > But if there can be more than one thread in the thread list with the > same ptid_t, inferior_thread() may well return the wrong thread. I don't quite understand this part of the explanation. Thread lists are per-inferior, and an inferior is specific to one target, and ptids are unique per-target. So it's not possible at the moment (or at least not expected) to have two threads with the same ptid in one specific list. I am having trouble connecting the dots between this explanation and the problem originally reported, which IIRC correctly involves a single inferior. > This series fixes this by making the current thread be a global > thread_info pointer that is written to directly by switch_to_thread, > etc., and making inferior_thread() return that pointer, instead of > having inferior_thread() lookup up the inferior_ptid thread, by > ptid_t. You can look at this as a continuation of the effort of using > more thread_info pointers instead of ptids when possible. Even though I don't understand the explanation above, I think this is a good step forward. > This change required auditing the whole codebase for places where we > were writing to inferior_ptid directly to change the current thread, > and change them to use switch_to_thread instead or one of its > siblings, because otherwise inferior_thread() and inferior_ptid would > get out of sync and inferior_thread() would return a thread unrelated > to the new inferior_ptid we wanted to switch to. That was all > (hopefully) done in all the patches leading up to the last one. > > After this, inferior_ptid still exists, but it is mostly read-only and > mainly used by target backend code. It is also relied on by a number > of target methods as a global input argument. E.g., the target_resume > interface and the memory reading routines -- we still need it there > because we need to be able to access memory off of processes for which > we don't have a corresponding inferior/thread object, like when > handling forks. Maybe we could pass down a context explicitly to > target_read_memory, etc. I would like to do that eventually. > Most of the host-/native-specific code here is untested. I did my > best, but I won't be surprised if more tweaking is necessary. > > Testing on native x86_64 GNU/Linux is regression free for me. Testing > against gdbserver has regressed significantly in the past months and > is becoming difficult to run with a high number of long timeout > sequences; really looks like people aren't paying much attention to > that. I think this series doesn't regress gdbserver, but it's getting > hard to tell. :-/ Oops... we'll have to do some bisecting. Simon