From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by sourceware.org (Postfix) with ESMTPS id 28ECF3858D1E for ; Mon, 6 Feb 2023 19:11:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 28ECF3858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f48.google.com with SMTP id k8-20020a05600c1c8800b003dc57ea0dfeso11542688wms.0 for ; Mon, 06 Feb 2023 11:11:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=zjL1GdysppfPmdB/1YvRTtKETpGEEcMG6/EzfDUeEtw=; b=d4Qr7d7xnfEWBY+U1IsymF4eA7YMVikNa0gl/jjpLyrB6HGQ1U16uHRzzQutfF6dLn Z9j7VRtWQ9a98rbedJnJgCPcIrtuFlXJtxN4CMMs8I9X64uAQ9D1xBiyIwPjQirfVA5B +jrOQAK926e3oDspcZJIMth400J09cL/KPOjte6IhiLhY4511YrGz42KLmi4sgHEK60+ /iLo7Gs8XdAmKZr35wQ+cqhN5hnCbM5kN24F8SX4DXiVpl59ESqZ/nJwJNjZAgD1Mc13 ToW4ixF0FjMEpH8VVv53j1TZohbFdFFq5jTIYKm9GkkDOPcw+FlQGFEHhFf7QZKJzCFS uOfw== X-Gm-Message-State: AO0yUKWFFOa4i5oAiPWDQh3RjG00nn90pSSwGc0mqvk/xsYh03wQKSDN tJWZLQGkEFpIDGyE3Wkroe0JeJ7626cIQA== X-Google-Smtp-Source: AK7set8m9khH65go18P7VCl3bJNLqgrfcxgd1cyiQ7EZnwAUeynhevznEhu0H85ZahT/7OFX0W3Rhg== X-Received: by 2002:a05:600c:755:b0:3e0:6c4:6a3a with SMTP id j21-20020a05600c075500b003e006c46a3amr715688wmn.22.1675710693722; Mon, 06 Feb 2023 11:11:33 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id u16-20020a05600c19d000b003dd1b00bd9asm12598666wmq.32.2023.02.06.11.11.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Feb 2023 11:11:33 -0800 (PST) Subject: Re: [PATCH v3 0/8] gdbserver improvements for AArch64 SVE support To: Thiago Jung Bauermann , gdb-patches@sourceware.org References: <20230130044518.3322695-1-thiago.bauermann@linaro.org> From: Pedro Alves Message-ID: <28bdd610-982d-aebb-ebc7-17b6d5fd80ed@palves.net> Date: Mon, 6 Feb 2023 19:11:30 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20230130044518.3322695-1-thiago.bauermann@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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 List-Id: On 2023-01-30 4:45 a.m., Thiago Jung Bauermann via Gdb-patches wrote: > Hello, > > This is version 3 of the patch series adding support to gdbserver for > inferiors that change the SVE vector length at runtime. This is already > supported by GDB, but not by gdbserver. Version 2 was posted here: > > https://inbox.sourceware.org/gdb-patches/20221126020452.1686509-1-thiago.bauermann@linaro.org/ > > This version incorporates the review comments from v2 (thanks!). The > biggest change is that it implements Simon's idea¹: > >> If it's the case, that we need to extend the RSP to allow fetching >> per-thread tdesc, here's the idea I had in mind for a while, to avoid >> adding too much overhead. Stop replies and the qXfer:threads:read reply >> could include a per-thread tdesc identifier. That tdesc identifier >> would be something short, either an incrementing number, or some kind of >> hash of the tdesc. It would be something opaque chosen by the stub. A >> new packet would be introduced to allow GDB to request the XML given >> that ID. On the GDB side, GDB would request the XML when encountering a >> tdesc ID it doesn't know about, and then cache the result. > > The only difference from the above in this series is that instead of > creating a new packet to allow GDB to request the XML given the ID, I'm > extending the qXfer:features:read request: GDB can send "target-id-%u.xml" > as the annex, where %u is the target description ID. Please let me know if > you think a separate packet would be better. The remote protocol changes > are documented in patch 5. > > I had to drop the Reviewed-by tags from some patches because they have some > significant changes from the version that was reviewed. > > There's also a new testcase exercising the case of an inferior's thread > changing its SVE vector length. The previous version of this series failed > the testcase, but this one passes it. > > With this series applied, gdb.arch/aarch64-sve.exp passes all tests on > extended-remote aarch64-linux. Without them, it fails tests after the > testcase changes the vector length. > > Tested on native and extended-remote aarch64-linux, x86_64-linux and > armv7l-linux-gnueabihf (the last one on QEMU TCG). > > ¹ https://inbox.sourceware.org/gdb-patches/559069a3-f3ce-2059-bf4a-44add43979f7@simark.ca/ > Ah! I had also suggested to you something like that at the Cauldron (when we were in line for lunch. :-D However, IIRC, I had suggested that we should be able to cache the tdesc by filename. Let me explain -- when we fetch a target description, we actually tell the server to retrieve a tdesc _by a given filename_. By default, we ask for target.xml, like this: qXfer:features:read:target.xml but then, the retrieved target.xml file may "xi:include" some other file, like for example these do: gdb/features/s390-linux64.xml:14: gdb/features/s390-linux64.xml:15: gdb/features/s390-linux64.xml:16: (try grepping for xi:include in the gdb/features/ dir for a lot more hits.) and so when processing each of those xi:include's, gdb sends another qXfer:features:read packet, with the corresponding included filename, like e.g., qXfer:features:read:s390-core64.xml Here's what the manual says: @item qXfer:features:read:@var{annex}:@var{offset},@var{length} @anchor{qXfer target description read} Access the @dfn{target description}. @xref{Target Descriptions}. The annex specifies which XML document to access. The main description is always loaded from the @samp{target.xml} annex. So basically I am suggesting that instead a new ID mechanism, we should be able to use the preexisting annex/filename concept as key. That means that the stop reply and the thread listing would include a new "tdesc=foo.xml" attribute, instead of this ID that then is defined to map to "target-id-%u.xml", which is basically admitting that tdesc filenames exist anyhow. I admit I haven't yet read the patches at all yet (of any series version), but I'd like bring this up already, in case it helps with saving wasted effort. Pedro Alves > Changes since v2: > > - Patch “gdbserver: Add assert in find_register_by_number” > - Rewritten to follow Simon's suggestion of putting the assert in another > function. > - Patch “gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap” > - As suggested by Simon, directly access thread_info::id rather than use > pid_of. > - Also as suggested by Simon, updated doc comment of > process_stratum_target::read_auxv. > - Patch “gdbserver/linux-aarch64: Factor out function to get aarch64_features” > - As suggested by Simon, directly access thread_info::id rather than use > lwpid_of. > - Also as suggested by Simon, added doc comment for > aarch64_get_arch_features. > - Patch “gdbserver/linux-aarch64: When thread stops, update its target description” > - As suggested by Luis, added doc comments to thread_info::tdesc and > arch_process_info::has_sve. > - As suggested by Simon, renamed arch_update_tdesc to get_thread_tdesc, > and return a target_desc pointer (or nullptr) instead of one wrapped in > gdb::optional. The code was changed a bit as well. > - As suggested by Luis, expanded doc comment of get_thread_tdesc. > - As suggested by Luis, abstracted away trying to fetch a tdesc from a > thread and then from a process to a new function > "get_thread_target_desc". > - Export free_register_cache_thread in gdbserver/regcache.h. > - Use free_register_cache_thread in linux_process_target::filter_event to > implement Simon's suggestion of only freeing the regcache for the > thread with a different tdesc. > - “gdb/aarch64: Factor out most of the thread_architecture method” > - Dropped this patch. It's not needed anymore. > - “gdbserver: Transmit target description ID in thread list and stop reply” > - New patch. > - “gdb/remote: Parse and use tdesc field in stop reply and threads list XML” > - New patch. > - “gdb/aarch64: Detect vector length changes when debugging remotely” > - As suggested by Luis, clarified in the patch description that this patch improves > debugging programs remotely. Also, reworded description somewhat. > - As suggested by Luis, changed VL references in comments to VG. Also reworded a bit the > description of the update_architecture gdbarch method. > - Changed the update_architecture gdbarch method (and consequently also > aarch64_update_architecture) to take a target description instead of a vector of > cached_reg_t. > - Changed remote_target::update_thread_list to get the thread target description from > the remote target. > - Changed remote_target::process_stop_reply to get the thread target description from > the remote target. > - Renamed remote_thread_info::expedited_arch to arch. > - Changed code of remote_target::thread_architecture to be a little bit clearer, and > added doc comment. > - “gdb/testsuite: Add test to exercise multi-threaded AArch64 SVE inferiors” > - New patch. > > Thiago Jung Bauermann (8): > gdbserver: Add assert in find_register_by_number > gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap > gdbserver/linux-aarch64: Factor out function to get aarch64_features > gdbserver/linux-aarch64: When thread stops, update its target > description > gdbserver: Transmit target description ID in thread list and stop > reply > gdb/remote: Parse tdesc field in stop reply and threads list XML > gdb/aarch64: Detect vector length changes when debugging remotely > gdb/testsuite: Add test to exercise multi-threaded AArch64 SVE > inferiors > > gdb/aarch64-tdep.c | 20 +++ > gdb/arch-utils.c | 8 + > gdb/arch-utils.h | 4 + > gdb/doc/gdb.texinfo | 27 ++- > gdb/features/threads.dtd | 1 + > gdb/gdbarch-components.py | 15 ++ > gdb/gdbarch-gen.h | 10 ++ > gdb/gdbarch.c | 22 +++ > gdb/remote.c | 169 +++++++++++++++++- > gdb/testsuite/gdb.arch/aarch64-sve-threads.c | 125 +++++++++++++ > .../gdb.arch/aarch64-sve-threads.exp | 70 ++++++++ > gdb/xml-tdesc.c | 27 ++- > gdb/xml-tdesc.h | 6 + > gdbserver/gdbthread.h | 4 + > gdbserver/linux-aarch64-low.cc | 68 +++++-- > gdbserver/linux-arm-low.cc | 2 +- > gdbserver/linux-low.cc | 35 +++- > gdbserver/linux-low.h | 15 +- > gdbserver/linux-ppc-low.cc | 6 +- > gdbserver/linux-s390-low.cc | 2 +- > gdbserver/netbsd-low.cc | 4 +- > gdbserver/netbsd-low.h | 2 +- > gdbserver/regcache.cc | 14 +- > gdbserver/regcache.h | 4 + > gdbserver/remote-utils.cc | 57 ++++++ > gdbserver/remote-utils.h | 9 + > gdbserver/server.cc | 20 ++- > gdbserver/server.h | 4 + > gdbserver/target.cc | 4 +- > gdbserver/target.h | 4 +- > gdbserver/tdesc.cc | 13 +- > gdbserver/tdesc.h | 5 + > 32 files changed, 717 insertions(+), 59 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve-threads.c > create mode 100644 gdb/testsuite/gdb.arch/aarch64-sve-threads.exp > > > base-commit: 6c76a6beade05f8b2ca93e939cf73da2917d379b >