Message ID | 20230130044518.3322695-7-thiago.bauermann@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 82E8E3858D38 for <patchwork@sourceware.org>; Mon, 30 Jan 2023 04:47:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 82E8E3858D38 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1675054054; bh=aDmfr8moEGlKeX79D9kihvQnYDRwBCH3CBQz7Qx3euc=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=ZxFLcJOFVu9vEze4Kt082qnR6T3pRq5pv5XYKzavDX9vRfTu5Dyu70E3xijTaX51a edI3Wtf760vWdmMpka7ACQS2+MLfVAcFPWVFOH2TBdVWYvBZVjJeSkQ14HoHQAyiDE naA0yQm3nZD+n0vsi85NefdvzA3Qw4kQbHJPNWIU= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-ot1-x335.google.com (mail-ot1-x335.google.com [IPv6:2607:f8b0:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id 7DDC13858C2C for <gdb-patches@sourceware.org>; Mon, 30 Jan 2023 04:45:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7DDC13858C2C Received: by mail-ot1-x335.google.com with SMTP id a1-20020a056830008100b006864df3b1f8so4228235oto.3 for <gdb-patches@sourceware.org>; Sun, 29 Jan 2023 20:45:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=aDmfr8moEGlKeX79D9kihvQnYDRwBCH3CBQz7Qx3euc=; b=XRAh+SOvfs3f0f8SMJRpPQklP54tfrnJBMhAANaSrgWQVCcSa662aB7tJu+U/7MsF9 /XqW7CxvLtsOoCi4LzLiG5h3eJB5lQ3uVfBTL/sS+/kRegiTxAOyhNnUJJNQb7oqZJlv W12Psw76WdHLBkhgYeGgC/GuEHevm3NfKlA1ORmj7ntpA57GbRbQsN1zCOjptooEzV4W xwGstST2qFHhhHL1n2nPR9BUga/vLHJMFivXDcWppsITTGqzFMpiK+5A+dDCr7C+Y+xV fNXlMIw5nkXnqxz1iVWcTXPhIhYrTN3ljSEmxmZ1vjOeGSnsRsqBycEiaAdhNEknaVcr xYaw== X-Gm-Message-State: AFqh2kqyQkiy/IWw8dcB/huc+KsBfFMOFd+vTQ0slL9KyFjaZfhLnNQQ p85WrTRasJESr9X4FPUy1kmDtW3qOzjMqiyw X-Google-Smtp-Source: AMrXdXskTik8qrHgmbT/+rsyBYqZJ+opXp2z42B1g6B6r3QHPKP6Q1b3jPW0kNpGf79hiaNR9TSUxw== X-Received: by 2002:a05:6830:1457:b0:670:6b50:fde3 with SMTP id w23-20020a056830145700b006706b50fde3mr24811534otp.26.1675053954093; Sun, 29 Jan 2023 20:45:54 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:df99:10bd:7dca:b2e9]) by smtp.gmail.com with ESMTPSA id g22-20020a9d6496000000b00684bede5359sm5046906otl.42.2023.01.29.20.45.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 29 Jan 2023 20:45:53 -0800 (PST) To: gdb-patches@sourceware.org Cc: Thiago Jung Bauermann <thiago.bauermann@linaro.org> Subject: [PATCH v3 6/8] gdb/remote: Parse tdesc field in stop reply and threads list XML Date: Mon, 30 Jan 2023 04:45:16 +0000 Message-Id: <20230130044518.3322695-7-thiago.bauermann@linaro.org> X-Mailer: git-send-email 2.39.1 In-Reply-To: <20230130044518.3322695-1-thiago.bauermann@linaro.org> References: <20230130044518.3322695-1-thiago.bauermann@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Thiago Jung Bauermann <thiago.bauermann@linaro.org> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
gdbserver improvements for AArch64 SVE support
|
|
Commit Message
Thiago Jung Bauermann
Jan. 30, 2023, 4:45 a.m. UTC
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 <thread> 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(-)
Comments
On 1/30/23 04:45, Thiago Jung Bauermann via Gdb-patches wrote: > 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 > <thread> 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 @@ > > <!ATTLIST thread id CDATA #REQUIRED> > <!ATTLIST thread core CDATA #IMPLIED> > +<!ATTLIST thread tdesc CDATA #IMPLIED> > 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 <unordered_map> > #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. */ Will it be requested soon or can gdb just ignore it if the user doesn't switch to that thread? If gdb can ignore it, then it might be nice to mention it here that gdb can chose to request it at any point in time, but may opt not to do it at all. > + void add_tdesc_id (ULONGEST id); > + > + /* Get the target description corresponding to remote protocol ID. */ s/remote protocol/remote target description? > + 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<struct gdbarch *, remote_arch_state> > 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<ULONGEST, const target_desc *> 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<cached_reg_t> regcache; > > + /* The target description ID communicated in the stop reply packet. */ > + gdb::optional<ULONGEST> 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<ULONGEST> 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<thread_item> items; > + > + /* The remote target associated with this context. */ > + remote_target *m_remote; > }; > > 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 }, > { 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<gdb::char_vector> 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 I noticed we're dealing with the target description id as ULONGEST on gdb's side, but as unsigned int on gdbserver's side. Should we make them the same, if possible?
Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org> 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 > <thread> 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 @@ > > <!ATTLIST thread id CDATA #REQUIRED> > <!ATTLIST thread core CDATA #IMPLIED> > +<!ATTLIST thread tdesc CDATA #IMPLIED> > 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 <unordered_map> > #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<struct gdbarch *, remote_arch_state> > 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<ULONGEST, const target_desc *> 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<ULONGEST, std::unique_ptr<const target_desc>> 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<cached_reg_t> regcache; > > + /* The target description ID communicated in the stop reply packet. */ > + gdb::optional<ULONGEST> 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<ULONGEST> 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<thread_item> 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<gdb::char_vector> 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
On 2/1/23 09:32, Andrew Burgess via Gdb-patches wrote: > Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org> > 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 >> <thread> 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 @@ >> >> <!ATTLIST thread id CDATA #REQUIRED> >> <!ATTLIST thread core CDATA #IMPLIED> >> +<!ATTLIST thread tdesc CDATA #IMPLIED> >> 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 <unordered_map> >> #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<ULONGEST, std::unique_ptr<const target_desc>> 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
> 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. Another question related to this popped while reading the following patch. When creating a gdbarch from a tdesc, the gdbarch keeps a pointer to that tdesc (accessible through gdbarch_target_desc). And AFAIK, we never delete gdbarches. So I suppose the gdbarch will refer a stale target desc. At first I thought it wouldn't be a problem in practice, because while that gdbarch object still exists, nothing references it (it is effectively leaked). But then I remember that we cache gdbarches to avoid creating arches with duplicate features. So later (let's say if you connect again to a remote), we might want to create a gdbarch with the same features as before, and we'll dig up the old gdbarch, that points to the now deleted tdesc. Perhaps it's possible to generate a crash with the current implementation by connecting, disconnecting, connecting again, and then doing something that uses the thread-specific gdbarch. Simon
On 2/1/23 20:16, Simon Marchi via Gdb-patches wrote: >> 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. > > Another question related to this popped while reading the following > patch. When creating a gdbarch from a tdesc, the gdbarch keeps a > pointer to that tdesc (accessible through gdbarch_target_desc). And > AFAIK, we never delete gdbarches. So I suppose the gdbarch will refer a > stale target desc. At first I thought it wouldn't be a problem in > practice, because while that gdbarch object still exists, nothing > references it (it is effectively leaked). But then I remember that we > cache gdbarches to avoid creating arches with duplicate features. So > later (let's say if you connect again to a remote), we might want to > create a gdbarch with the same features as before, and we'll dig up the > old gdbarch, that points to the now deleted tdesc. The target descriptions for aarch64 are all cached using a map in gdb/aarch64-tdep.c: /* All possible aarch64 target descriptors. */ static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map; I don't think we should try to delete those, and they should live throughout the life of gdb (unless things get large, then we might consider cleanups). > > Perhaps it's possible to generate a crash with the current > implementation by connecting, disconnecting, connecting again, and then > doing something that uses the thread-specific gdbarch. > > Simon >
On 2/3/23 06:27, Luis Machado wrote: > On 2/1/23 20:16, Simon Marchi via Gdb-patches wrote: >>> 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. >> >> Another question related to this popped while reading the following >> patch. When creating a gdbarch from a tdesc, the gdbarch keeps a >> pointer to that tdesc (accessible through gdbarch_target_desc). And >> AFAIK, we never delete gdbarches. So I suppose the gdbarch will refer a >> stale target desc. At first I thought it wouldn't be a problem in >> practice, because while that gdbarch object still exists, nothing >> references it (it is effectively leaked). But then I remember that we >> cache gdbarches to avoid creating arches with duplicate features. So >> later (let's say if you connect again to a remote), we might want to >> create a gdbarch with the same features as before, and we'll dig up the >> old gdbarch, that points to the now deleted tdesc. > > The target descriptions for aarch64 are all cached using a map in gdb/aarch64-tdep.c: > > /* All possible aarch64 target descriptors. */ > static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map; > > I don't think we should try to delete those, and they should live throughout the life of gdb (unless things get large, then we might consider cleanups). When debugging natively with GDB, that's true. When debugging remotely, on GDBserver-side, that's true too. But when debugging remotely, on GDB-side, don't we create a new target_desc object for each read target description? Ok, I just saw in xml-tdesc.c: /* A record of every XML description we have parsed. We never discard old descriptions, because we never discard gdbarches. As long as we have a gdbarch referencing this description, we want to have a copy of it here, so that if we parse the same XML document again we can return the same "struct target_desc *"; if they are not singletons, then we will create unnecessary duplicate gdbarches. See gdbarch_list_lookup_by_info. */ static std::unordered_map<std::string, target_desc_up> xml_cache; So, at least, a remote sending the same exact XML over and over will lead to the same target_desc object being reused. And there won't be lifetime issues, since the target_desc created from XML also live forever. So I guess we're good. Simon
Simon Marchi <simark@simark.ca> writes: > On 2/3/23 06:27, Luis Machado wrote: >> On 2/1/23 20:16, Simon Marchi via Gdb-patches wrote: >>>> 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. >>> >>> Another question related to this popped while reading the following >>> patch. When creating a gdbarch from a tdesc, the gdbarch keeps a >>> pointer to that tdesc (accessible through gdbarch_target_desc). And >>> AFAIK, we never delete gdbarches. So I suppose the gdbarch will refer a >>> stale target desc. At first I thought it wouldn't be a problem in >>> practice, because while that gdbarch object still exists, nothing >>> references it (it is effectively leaked). But then I remember that we >>> cache gdbarches to avoid creating arches with duplicate features. So >>> later (let's say if you connect again to a remote), we might want to >>> create a gdbarch with the same features as before, and we'll dig up the >>> old gdbarch, that points to the now deleted tdesc. >> >> The target descriptions for aarch64 are all cached using a map in gdb/aarch64-tdep.c: >> >> /* All possible aarch64 target descriptors. */ >> static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map; >> >> I don't think we should try to delete those, and they should live throughout the life of gdb (unless things get large, then we might consider cleanups). > > When debugging natively with GDB, that's true. When debugging remotely, > on GDBserver-side, that's true too. But when debugging remotely, on > GDB-side, don't we create a new target_desc object for each read target > description? > > Ok, I just saw in xml-tdesc.c: > > /* A record of every XML description we have parsed. We never discard > old descriptions, because we never discard gdbarches. As long as we > have a gdbarch referencing this description, we want to have a copy > of it here, so that if we parse the same XML document again we can > return the same "struct target_desc *"; if they are not singletons, > then we will create unnecessary duplicate gdbarches. See > gdbarch_list_lookup_by_info. */ > > static std::unordered_map<std::string, target_desc_up> xml_cache; > > So, at least, a remote sending the same exact XML over and over will > lead to the same target_desc object being reused. And there won't be > lifetime issues, since the target_desc created from XML also live > forever. So I guess we're good. Simon, Thanks for chasing this down. I guess my concerns about object life time are addressed then. Thanks, Andrew
Luis Machado <luis.machado@arm.com> writes: > On 1/30/23 04:45, Thiago Jung Bauermann via Gdb-patches wrote: >> --- a/gdb/remote.c >> +++ b/gdb/remote.c >> @@ -80,6 +80,7 @@ >> #include <unordered_map> >> #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. */ > > Will it be requested soon or can gdb just ignore it if the user > doesn't switch to that thread? In this patch it would be requested soon, right after the threads list XML and the stop reply packet were parsed. But in my local branch that will become v4 I implemented Andrew's suggestion of getting the target descriptions on demand in remote_state::get_tdesc so now GDB may indeed ignore it. > If gdb can ignore it, then it might be nice to mention it here that > gdb can chose to request it at any point in time, but may opt not to > do it at all. As a consequence of Andrew's suggestion, the add_tdesc_id method isn't necessary anymore, so this comment isn't present anymore. >> + void add_tdesc_id (ULONGEST id); >> + >> + /* Get the target description corresponding to remote protocol ID. */ > > s/remote protocol/remote target description? I meant that in the sense of “ID that is used in the remote protocol”, but I agree it's more confusing than helpful. I changed it to: /* Get the target description corresponding to the given remote target description ID. */ WDYT? >> @@ -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 Fixed. >> 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 > > I noticed we're dealing with the target description id as ULONGEST on > gdb's side, but as unsigned int on gdbserver's side. > > Should we make them the same, if possible? My thinking was that since it's gdbserver that defines what the ID will be, it can use a simpler type (2³² target descriptions should be enough for anybody) since it knows what kind of IDs it will issue. But GDB doesn't know what the remote stub will want to do, so it should use a big type. But with Andrew's idea of passing a value during target negotiation to decide whether the ID will be a number or a hash, then we can use unsigned int for both types.
On 2/5/23 00:06, Thiago Jung Bauermann wrote: > > Luis Machado <luis.machado@arm.com> writes: > >> On 1/30/23 04:45, Thiago Jung Bauermann via Gdb-patches wrote: >>> --- a/gdb/remote.c >>> +++ b/gdb/remote.c >>> @@ -80,6 +80,7 @@ >>> #include <unordered_map> >>> #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. */ >> >> Will it be requested soon or can gdb just ignore it if the user >> doesn't switch to that thread? > > In this patch it would be requested soon, right after the threads list > XML and the stop reply packet were parsed. > > But in my local branch that will become v4 I implemented Andrew's > suggestion of getting the target descriptions on demand in > remote_state::get_tdesc so now GDB may indeed ignore it. > >> If gdb can ignore it, then it might be nice to mention it here that >> gdb can chose to request it at any point in time, but may opt not to >> do it at all. > > As a consequence of Andrew's suggestion, the add_tdesc_id method isn't > necessary anymore, so this comment isn't present anymore. > >>> + void add_tdesc_id (ULONGEST id); >>> + >>> + /* Get the target description corresponding to remote protocol ID. */ >> >> s/remote protocol/remote target description? > > I meant that in the sense of “ID that is used in the remote protocol”, > but I agree it's more confusing than helpful. I changed it to: > > /* Get the target description corresponding to the given remote target > description ID. */ > > WDYT? > Looks good! >>> @@ -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 > > Fixed. > >>> 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 >> >> I noticed we're dealing with the target description id as ULONGEST on >> gdb's side, but as unsigned int on gdbserver's side. >> >> Should we make them the same, if possible? > > My thinking was that since it's gdbserver that defines what the ID will > be, it can use a simpler type (2³² target descriptions should be enough > for anybody) since it knows what kind of IDs it will issue. But GDB > doesn't know what the remote stub will want to do, so it should use a > big type. > > But with Andrew's idea of passing a value during target negotiation to > decide whether the ID will be a number or a hash, then we can use > unsigned int for both types. > Got it.
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 @@ <!ATTLIST thread id CDATA #REQUIRED> <!ATTLIST thread core CDATA #IMPLIED> +<!ATTLIST thread tdesc CDATA #IMPLIED> 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 <unordered_map> #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); + + /* 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<struct gdbarch *, remote_arch_state> 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<ULONGEST, const target_desc *> 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<cached_reg_t> regcache; + /* The target description ID communicated in the stop reply packet. */ + gdb::optional<ULONGEST> 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<ULONGEST> 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<thread_item> items; + + /* The remote target associated with this context. */ + remote_target *m_remote; }; 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) + { + 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 }, { 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<gdb::char_vector> 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