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 1409E3858D3C for ; Wed, 1 Feb 2023 19:50:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1409E3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.11] (unknown [217.28.27.60]) (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 4A2F31E112; Wed, 1 Feb 2023 14:50:01 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1675281001; bh=XhNejq4tsEeUDH5jTx1bktJbgNBgbee/CIruPbcHiRs=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=CyvM7K89lRmidlF6R2K9Hv8KS3BF43rwiiYnnW2kjHpKeJKReYv7x3BbjHbVOlePs t8xzy1AwCSAexazclyzPRtuAPoUzHo80vBDwnsISSlEXTbhEOq/lf1vObzlheoQTPV mwfNYc1EY4pYcxUctA52Co9nhH6eN+LqxGF5J6h0= Message-ID: <9f5deefd-52fc-9792-f9a5-dede9c415777@simark.ca> Date: Wed, 1 Feb 2023 14:50:00 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH v3 6/8] gdb/remote: Parse tdesc field in stop reply and threads list XML Content-Language: en-US To: Andrew Burgess , Thiago Jung Bauermann via Gdb-patches Cc: Thiago Jung Bauermann References: <20230130044518.3322695-1-thiago.bauermann@linaro.org> <20230130044518.3322695-7-thiago.bauermann@linaro.org> <87edr9tq0c.fsf@redhat.com> From: Simon Marchi In-Reply-To: <87edr9tq0c.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2/1/23 09:32, Andrew Burgess via Gdb-patches wrote: > Thiago Jung Bauermann via Gdb-patches > writes: > >> gdbserver added the concept of target description IDs to the remote >> protocol and uses them in the threads list XML and in the 'T AA' stop >> reply packet. It also allows fetching a target description with a given >> ID. This patch is for the GDB-side support. The target descriptions >> obtained this way aren't yet used but will be in the next patch. >> >> In the DTD for the threads list XML, add a "tdesc" attribute to the >> node. A tdesc_id field is added to the stop_reply and >> thread_item structs. An m_remote member is added to the >> threads_listing_context struct, and to simplify its initialisation a >> constructor is added as well. This is to provide access to the remote >> state in start_thread. >> >> Finally, the remote_state object keeps a map of the target descriptions >> that have been received from the target, keyed by their ID. There are >> also methods to get a target description given its ID, and to fetch target >> descriptions for IDs that were mentioned by gdbserver but not yet >> retrieved by GDB. The latter gets called after parsing the response of >> qXfer:threads:read and of the stop reply packet. >> --- >> gdb/features/threads.dtd | 1 + >> gdb/remote.c | 85 +++++++++++++++++++++++++++++++++++++++- >> gdb/xml-tdesc.c | 27 ++++++++++--- >> gdb/xml-tdesc.h | 6 +++ >> 4 files changed, 112 insertions(+), 7 deletions(-) >> >> diff --git a/gdb/features/threads.dtd b/gdb/features/threads.dtd >> index 036b2ce58837..3102d1352978 100644 >> --- a/gdb/features/threads.dtd >> +++ b/gdb/features/threads.dtd >> @@ -11,3 +11,4 @@ >> >> >> >> + >> diff --git a/gdb/remote.c b/gdb/remote.c >> index 218bca30d047..f1d1944414c3 100644 >> --- a/gdb/remote.c >> +++ b/gdb/remote.c >> @@ -80,6 +80,7 @@ >> #include >> #include "async-event.h" >> #include "gdbsupport/selftest.h" >> +#include "xml-tdesc.h" >> >> /* The remote target. */ >> >> @@ -238,6 +239,16 @@ class remote_state >> /* Get the remote arch state for GDBARCH. */ >> struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch); >> >> + /* Add new ID to the target description list. The corresponding XML will be >> + requested soon. */ >> + void add_tdesc_id (ULONGEST id); > > I'm wondering why this function is needed? Could we not just have > get_tdesc take a remote_target* argument, and fetch the descriptions on > demand? This would also allow fetch_unknown_tdescs to be removed I > think, as well as the remote_target* inside threads_listing_context. Piggybacking on Andrew's review again, because he said most of what I noticed. I had a comment going in a similar direction. I think the intent of threads_listing_context to be a "parsed" version of the XML document, that can then be used by the caller that implements the logic. So I would just store the target description id in the thread_item, and that's it. update_thread_list can then fetch the missing target descriptions, probably as Andrew suggested. > Who owns the objects pointed too by this data structure? From my > reading of the code I suspect they are owned by the remote_state, in > which case we should possibly be deleting the objects in > remote_state::~remote_state. > > The only problem with this would be, what happens to any threads that > reference a target description within a remote connection that is close, > and thus the referenced target description is deleted.... > > ... having just run a test it appears that when we disconnect from a > remote target, the remote target itself (and the associated > remote_state) is deleted first, and then we delete the threads of > inferiors running on that target. That means that if we did delete the > target descriptions in ~remote_state, then we would, for a time, be in a > situation where the thread_info referenced a deleted target description. > > I'm not sure how easy that would be to fix, maybe we can just add some > code in remote_unpush_target before the call to > inferior::pop_all_targets_at_and_above? IIUC, the tdescs would be deleted during the pop_all_targets_at_and_above, when the refcount of the remote_target gets to 0 and it gets deleted. And the threads would be removed in generic_mourn_inferior just after. An idea could be to call generic_mourn_inferior before remote_unpush_target (no idea if it works). Another one would be to get a temporary reference to the remote_target object in remote_unpush_target, just so that it outlives the threads. Or maybe we should say that it's a process target's responsibility to delete any thread it "owns" before getting deleted itself. > Anyway, I think the first attempt should be to make the m_tdescs data > structure be: > > std::unordered_map> m_tdescs; Note that we have target_desc_up, with a custom deleter. It's necessary because struct target_desc is in target-description.c, not the header. Although I wouldn't mind if we moved the struct to the header and got rid of the custom deleter. >> @@ -3833,6 +3870,7 @@ const struct gdb_xml_attribute thread_attributes[] = { >> { "core", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL }, >> { "name", GDB_XML_AF_OPTIONAL, NULL, NULL }, >> { "handle", GDB_XML_AF_OPTIONAL, NULL, NULL }, >> + { "tdesc", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL }, > > Ideally s/NULL/nullptr/ here too, but this one's clearly a bit odd as > we're in a table surrounded by legacy code. But I think I'd still use > nullptr in preference. Also, feel free to submit a patch to change NULL for nullptr in this whole array. Or heck, the whole file. Simon