[v3,0/8] gdbserver improvements for AArch64 SVE support

  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/

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
  

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:  <xi:include href="s390-core64.xml"/>
 gdb/features/s390-linux64.xml:15:  <xi:include href="s390-acr.xml"/>
 gdb/features/s390-linux64.xml:16:  <xi:include href="s390-fpr.xml"/>

(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
>
  

> 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:  <xi:include href="s390-core64.xml"/>
>  gdb/features/s390-linux64.xml:15:  <xi:include href="s390-acr.xml"/>
>  gdb/features/s390-linux64.xml:16:  <xi:include href="s390-fpr.xml"/>
> 
> (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.

Just for completeness, how do you envision that working for SVE?
GDBserver would make up unique names for each configuration, like
"target-vq-%d.xml"?

If caching using filenames as keys, what is the scope of that namespace?
Per remote connection, per inferior?  I think it wouldn't work per
remote connection, because fetching "target.xml" for two different
inferiors could give two different answers.

Tangentially, I'm wondering if querying qXfer:features:read (mostly
fetching "target.xml" is going to become thread-sensitive.  In other
words, if GDB set the general thread to a thread with vq == A, gets
target.xml, then sets the general thread to a thread with vq == B, then
gets target.xml, is it going to get two different target descriptions?
I think that it would make sense to do so *.  Therefore, caching
target.xml per-inferior wouldn't be reliable either.  And if included
files could vary per thread, you'd have to make sure to give them unique
names.

* I think that since we're going towards thread-specific tdescs, the
  process-wide tdesc is going to become an obsolete concept, maybe just
  kept for backwards compatibility for when an old GDB not aware of
  per-thread tdescs talks to a new GDBserver aware of it, or vice versa.

Simon
  

On 2023-02-06 8:05 p.m., Simon Marchi via Gdb-patches wrote:
> 
>> 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:  <xi:include href="s390-core64.xml"/>
>>  gdb/features/s390-linux64.xml:15:  <xi:include href="s390-acr.xml"/>
>>  gdb/features/s390-linux64.xml:16:  <xi:include href="s390-fpr.xml"/>
>>
>> (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.
> 
> Just for completeness, how do you envision that working for SVE?
> GDBserver would make up unique names for each configuration, like
> "target-vq-%d.xml"?

The string is free form, up to the server to decide what to use for names.  Could 
be that, or "aarch64-linux-sve-vq-%d.xml" or whatever.  Of course, for ports that
wouldn't generate the tdescs on the fly, you'd want to use the same name as the
real existing xml files that are embedded into gdbserver.

> 
> If caching using filenames as keys, what is the scope of that namespace?
> Per remote connection, per inferior?  I think it wouldn't work per
> remote connection, because fetching "target.xml" for two different
> inferiors could give two different answers.

Right, I think we'd make it per-inferior, at least by default, for backwards compatibility.

But we can also think about the server telling gdb about the scope if we want, with qSupported
or some such.  And then if the stub tells gdb that the scope is connection, the stub would also
tell gdb about the architecture of the main thread (in stop reply and thread listings),
say, "the-right-secondary-arch.xml" so gdb would retrieve "qXfer:features:read:the-right-secondary-arch.xml",
for that inferior, and not "qXfer:features:read:target.xml".  Basically, gdb only fetches
"target.xml" when it doesn't know better.

> 
> Tangentially, I'm wondering if querying qXfer:features:read (mostly
> fetching "target.xml" is going to become thread-sensitive.  In other
> words, if GDB set the general thread to a thread with vq == A, gets
> target.xml, then sets the general thread to a thread with vq == B, then
> gets target.xml, is it going to get two different target descriptions?
> I think that it would make sense to do so *.  

I am not so sure about that making sense.

As target.xml is documented as the main tdesc, it would continue that way,
with "main" meaning the tdesc that the initial thread has when the process
is first started.

> Therefore, caching
> target.xml per-inferior wouldn't be reliable either.  And if included
> files could vary per thread, you'd have to make sure to give them unique
> names.

With the numerical ID idea, you have to know which ID to read for the thread,
before you read it.  You don't just go and read the desc with ID 0 thinking that
0 is a different ID depending on thread, right?  So it's no different -- the stop
reply would say "tdesc=whatever-secondary-arch.xml", and GDB would fetch that xml
file if not cached yet, and use the cached version otherwise.

> 
> * I think that since we're going towards thread-specific tdescs, the
>   process-wide tdesc is going to become an obsolete concept, maybe just
>   kept for backwards compatibility for when an old GDB not aware of
>   per-thread tdescs talks to a new GDBserver aware of it, or vice versa.
> 
> Simon
>
  

>> Tangentially, I'm wondering if querying qXfer:features:read (mostly
>> fetching "target.xml" is going to become thread-sensitive.  In other
>> words, if GDB set the general thread to a thread with vq == A, gets
>> target.xml, then sets the general thread to a thread with vq == B, then
>> gets target.xml, is it going to get two different target descriptions?
>> I think that it would make sense to do so *.  
> 
> I am not so sure about that making sense.
> 
> As target.xml is documented as the main tdesc, it would continue that way,
> with "main" meaning the tdesc that the initial thread has when the process
> is first started.

My interpretation of the work "main" here was "top-level".  At least, I
suppose that's what the original author meant, because I suppose it was
written assuming there's a single target description "tree" per process.
So there wouldn't need to be the need to differentiate "main" vs
"secondary".

Simon