From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id C95973858412 for ; Wed, 1 Feb 2023 14:32:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C95973858412 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675261978; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4dAfxqTd5R5/TQnNWQHtYm4ML0lxs5Rywtgengx6x9I=; b=W4TqByghemDEoVs69+oMGuSDw56NsbO391v7/b1CPoYqbUWitcxQEWOTWmk0rXpNYrGOZY N5nIaZtL7gN8/YofR87HDuByKZjLbTWEaMd4Emoubhatt1bQBZanCuceCoPsX3fT5aR2qo bv2T5nsd9VkbFXafQJb1PYZ9Q5QbMkg= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-102-5UTBITDwPPu2DhnO_gZgRg-1; Wed, 01 Feb 2023 09:32:54 -0500 X-MC-Unique: 5UTBITDwPPu2DhnO_gZgRg-1 Received: by mail-qv1-f69.google.com with SMTP id c10-20020a05621401ea00b004c72d0e92bcso10116878qvu.12 for ; Wed, 01 Feb 2023 06:32:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Pd0/174t//Jfn9Ls3FqF+yYOSstqEo3WtA/TwYaDbic=; b=30rdRALCnDukzb0V0USde8Y4zWwVWhyxFfIOVP+0jO6cL6P5wpZfbMxpoGPYgN7CCj Cqc5adHHaQSIjCPPgOUi2Mjuo1VVfxxbNBqv9BMYDgKb9Ab29tzI1luVkTmnc9p3shX+ I5OAOHCpq0WycLCMg4s7s1/MZvqGisowr7rkIiHZAwWbCzkbt2CLPzTFaI/5+XLRCzRk 2IRKBuvGBy4r8OPohdB/9pXPPRBVCCk8J8lycJ5AIPuvKAmZeKQl7yuFPtNG6qTyIYYq mw7C0iDwOhm4dVmNCIL6/a4iDy/0qr2ogiSHReX8R0SdypuY7BpOIVh6qTsuKF7dpdw9 Ccdg== X-Gm-Message-State: AO0yUKUkbY/+hhSIItXt8IC2fTEM/Ju3ASMOUlUqnJd81afiak2wBGaZ 6NboFXAfgUQ9ceRfhzl8/JdNM4BUmC/FILxfcH8LfqZMWYc3DPEo/T8fuSBZy2R/Adz37Bw0yGO Z64ogwhLD8rKiyKPvtlEPBskJ7aKi0zFdGVIOBrNGtka45pxZeNtDX8GHuu4KQB2zectAqvZxPA == X-Received: by 2002:a0c:e186:0:b0:53d:b0ff:b7ca with SMTP id p6-20020a0ce186000000b0053db0ffb7camr4284393qvl.23.1675261974231; Wed, 01 Feb 2023 06:32:54 -0800 (PST) X-Google-Smtp-Source: AK7set9hVG5i4fpu8+YVj7PtPO8UD9u43nm1fl2TbxqWs1C+stLzTWvWYTRgrhVzT8wMyVjNMZ3Tuw== X-Received: by 2002:a0c:e186:0:b0:53d:b0ff:b7ca with SMTP id p6-20020a0ce186000000b0053db0ffb7camr4284346qvl.23.1675261973703; Wed, 01 Feb 2023 06:32:53 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id oo12-20020a05620a530c00b0072692330190sm2998122qkn.64.2023.02.01.06.32.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Feb 2023 06:32:53 -0800 (PST) From: Andrew Burgess To: Thiago Jung Bauermann via Gdb-patches , gdb-patches@sourceware.org Cc: Thiago Jung Bauermann Subject: Re: [PATCH v3 6/8] gdb/remote: Parse tdesc field in stop reply and threads list XML In-Reply-To: <20230130044518.3322695-7-thiago.bauermann@linaro.org> References: <20230130044518.3322695-1-thiago.bauermann@linaro.org> <20230130044518.3322695-7-thiago.bauermann@linaro.org> Date: Wed, 01 Feb 2023 14:32:51 +0000 Message-ID: <87edr9tq0c.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: 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. > + > + /* Get the target description corresponding to remote protocol ID. */ > + const target_desc *get_tdesc (ULONGEST id) const; > + > + /* Get the target descriptions we don't know about from the target. */ > + void fetch_unknown_tdescs (remote_target *remote); > + > public: /* data */ > > /* A buffer to use for incoming packets, and its current size. The > @@ -387,6 +398,10 @@ class remote_state > support multi-process. */ > std::unordered_map > m_arch_states; > + > + /* The target descriptions that have been received from the target. The key > + is the ID used to reference it in the remote protocol. */ > + std::unordered_map m_tdescs; 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? Anyway, I think the first attempt should be to make the m_tdescs data structure be: std::unordered_map> m_tdescs; > }; > > static const target_info remote_target_info = { > @@ -1009,6 +1024,9 @@ struct stop_reply : public notif_event > fetch them is avoided). */ > std::vector regcache; > > + /* The target description ID communicated in the stop reply packet. */ > + gdb::optional tdesc_id; > + > enum target_stop_reason stop_reason; > > CORE_ADDR watch_data_address; > @@ -3689,6 +3707,9 @@ struct thread_item > > /* The thread handle associated with the thread. */ > gdb::byte_vector thread_handle; > + > + /* The ID of the thread's target description, if provided. */ > + gdb::optional tdesc_id; > }; > > /* Context passed around to the various methods listing remote > @@ -3697,6 +3718,12 @@ struct thread_item > > struct threads_listing_context > { > + threads_listing_context (remote_target *remote) > + : m_remote (remote) > + {} > + > + DISABLE_COPY_AND_ASSIGN (threads_listing_context); > + > /* Return true if this object contains an entry for a thread with ptid > PTID. */ > > @@ -3733,6 +3760,9 @@ struct threads_listing_context > > /* The threads found on the remote target. */ > std::vector items; > + > + /* The remote target associated with this context. */ > + remote_target *m_remote; The GDB style reserves the 'm_' prefix for private member variables. Ideally I'd prefer m_remote be made private, and then add a 'remote()' member function to return the pointer. Though if my comment above is correct then I think this new field could be dropped. > }; > > static int > @@ -3814,6 +3844,13 @@ start_thread (struct gdb_xml_parser *parser, > attr = xml_find_attribute (attributes, "handle"); > if (attr != NULL) > item.thread_handle = hex2bin ((const char *) attr->value.get ()); > + > + attr = xml_find_attribute (attributes, "tdesc"); > + if (attr != NULL) s/NULL/nullptr/ > + { > + item.tdesc_id = *(ULONGEST *) attr->value.get (); > + data->m_remote->get_remote_state ()->add_tdesc_id (*item.tdesc_id); > + } > } > > static void > @@ -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. Thanks, Andrew > { NULL, GDB_XML_AF_NONE, NULL, NULL } > }; > > @@ -3870,6 +3908,7 @@ remote_target::remote_get_threads_with_qxfer (threads_listing_context *context) > { > gdb_xml_parse_quick (_("threads"), "threads.dtd", > threads_elements, xml->data (), context); > + get_remote_state ()->fetch_unknown_tdescs (this); > } > > return 1; > @@ -3937,7 +3976,7 @@ has_single_non_exited_thread (inferior *inf) > void > remote_target::update_thread_list () > { > - struct threads_listing_context context; > + struct threads_listing_context context (this); > int got_list = 0; > > /* We have a few different mechanisms to fetch the thread list. Try > @@ -7223,7 +7262,11 @@ remote_notif_stop_parse (remote_target *remote, > struct notif_client *self, const char *buf, > struct notif_event *event) > { > - remote->remote_parse_stop_reply (buf, (struct stop_reply *) event); > + struct stop_reply *stop_reply = (struct stop_reply *) event; > + > + remote->remote_parse_stop_reply (buf, stop_reply); > + > + stop_reply->rs->fetch_unknown_tdescs (remote); > } > > static void > @@ -7516,6 +7559,36 @@ strprefix (const char *p, const char *pend, const char *prefix) > return *prefix == '\0'; > } > > +void > +remote_state::add_tdesc_id (ULONGEST id) > +{ > + /* Check whether the ID was already added. */ > + if (m_tdescs.find (id) != m_tdescs.cend ()) > + return; > + > + m_tdescs[id] = nullptr; > +} > + > +const target_desc * > +remote_state::get_tdesc (ULONGEST id) const > +{ > + auto found = m_tdescs.find (id); > + > + /* Check if the given ID was already provided. */ > + if (found == m_tdescs.cend ()) > + return nullptr; > + > + return found->second; > +} > + > +void > +remote_state::fetch_unknown_tdescs (remote_target *remote) > +{ > + for (auto &pair : m_tdescs) > + if (pair.second == nullptr) > + m_tdescs[pair.first] = target_read_description_xml (remote, pair.first); > +} > + > /* Parse the stop reply in BUF. Either the function succeeds, and the > result is stored in EVENT, or throws an error. */ > > @@ -7674,6 +7747,14 @@ Packet: '%s'\n"), > event->ws.set_thread_created (); > p = strchrnul (p1 + 1, ';'); > } > + else if (strprefix (p, p1, "tdesc")) > + { > + ULONGEST tdesc_id; > + > + p = unpack_varlen_hex (++p1, &tdesc_id); > + event->rs->add_tdesc_id (tdesc_id); > + event->tdesc_id = tdesc_id; > + } > else > { > ULONGEST pnum; > diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c > index ba7154c5d56f..302863e12365 100644 > --- a/gdb/xml-tdesc.c > +++ b/gdb/xml-tdesc.c > @@ -698,14 +698,13 @@ fetch_available_features_from_target (const char *name, target_ops *ops) > } > > > -/* Read an XML target description using OPS. Parse it, and return the > - parsed description. */ > +/* Actual implementation of the target_read_description_xml variants. */ > > -const struct target_desc * > -target_read_description_xml (struct target_ops *ops) > +static const struct target_desc * > +target_read_description_xml (struct target_ops *ops, const char *desc_name) > { > gdb::optional tdesc_str > - = fetch_available_features_from_target ("target.xml", ops); > + = fetch_available_features_from_target (desc_name, ops); > if (!tdesc_str) > return NULL; > > @@ -717,6 +716,24 @@ target_read_description_xml (struct target_ops *ops) > return tdesc_parse_xml (tdesc_str->data (), fetch_another); > } > > +/* See xml-tdesc.h. */ > + > +const struct target_desc * > +target_read_description_xml (struct target_ops *ops) > +{ > + return target_read_description_xml (ops, "target.xml"); > +} > + > +/* See xml-tdesc.h. */ > + > +const struct target_desc * > +target_read_description_xml (struct target_ops *ops, ULONGEST id) > +{ > + std::string desc_name = string_printf ("target-id-%" PRIu64 ".xml", id); > + > + return target_read_description_xml (ops, desc_name.c_str ()); > +} > + > /* Fetches an XML target description using OPS, processing > includes, but not parsing it. Used to dump whole tdesc > as a single XML file. */ > diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h > index 0fbfc7e043e9..c7cc97c5dfc0 100644 > --- a/gdb/xml-tdesc.h > +++ b/gdb/xml-tdesc.h > @@ -38,6 +38,12 @@ const struct target_desc *file_read_description_xml (const char *filename); > > const struct target_desc *target_read_description_xml (struct target_ops *); > > +/* Read an XML target description with the given ID using OPS. Parse it, and > + return the parsed description. */ > + > +const struct target_desc *target_read_description_xml (struct target_ops *ops, > + ULONGEST id); > + > /* Fetches an XML target description using OPS, processing includes, > but not parsing it. Used to dump whole tdesc as a single XML file. > Returns the description on success, and a disengaged optional