Re: [PATCH v3 5/8] gdbserver: Transmit target description ID in thread list and stop reply

Simon Marchi via Gdb-patches
2023-02-01T20:46:42Z

On 2/1/23 07:07, Andrew Burgess via Gdb-patches wrote:
> Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>
> writes:
>
>> Now that an inferior thread can have a different target description than
>> its process, there needs to be a way to communicate this target
>> description to GDB. So add the concept of a target description ID to the
>> remote protocol, which is used to reference them and allows them to be
>> transferred only once over the wire.
>>
>> The ID is an unsigned integer, and is sent in the 'T AA n1:r1;n2:r2;...'
>> stop reply packet as a new 'n:r' pair, where n is "tdesc" and "r" is an
>> unsigned integer containing the ID.
>>
>> It is also sent in the threads list XML in the response of a
>> qXfer:threads:read request. The ID is sent as a new "tdesc" attribute of
>> the <thread> node.
>>
>> To request the target description XML of a given ID, GDB sends the
>> qXfer:features:read request with "target-id-%u.xml" as the annex, where %u
>> is the target description ID.
>
> Luis already commented that in some locations the ID is hex, while in
> others it is not explicitly stated if the value is hex or decimal. I'd
> like to second that feedback, and suggest that we pick one, and use that
> consistently throughout.
>
> My thinking is that it will be easier to understand remote packet traces
> if target descriptions are requested using an ID in the safe format as
> was sent to GDB, and if IDs in different packets match up.
>
> Thus, I would suggest we switch to using 'target-id-%x.xml' here, and
> send the ID as hex in the threads reply packet.
>
> Additionally, I think it would be worth adding a new feature to the
> qSupported packet, maybe 'per-thread-tdesc'. With this added, GDB would
> be able to tell gdbserver that it supports this feature, and gdbserver
> will be able to confirm that the feature is supported.
>
> I'm not 100% sure what we'd want to do if it turns out GDB doesn't
> support the feature? Is it better to push on with GDB using the wrong
> target description? Or would it be better if gdbserver exits with an
> error suggesting the GDB needs updating? In some ways, _what_ we do
> doesn't really matter to me, but I think having the feature will allow
> us to pick a suitable error handling solution later if needed.
>
> I'd be happy if adding the feature was done as a separate patch in this
> series, but I do think it should be part of this series.

I forgot to comment on that part. Just to think out loud:

- If GDB supports the new feature but GDBserver doesn't, I guess that
debugging works exactly like today? What happens today if a remote
thread changes its SVE vector size, the registers that GDB shows will
simply not reflect the reality, they might have bogus values, but
debugging will otherwise work?

- If GDB does not support the new feature but GDBserver does, I guess it
is problematic, because GDBserver will assume that GDB will have
adjusted its expectations about the length (and layout) of the g
packet response, since it told GDB about the thread-specific tdesc.
But since GDB doesn't know about thread-specific tdescs, it will try
to interpret the g packet response with the register layout of the
process-wide tdesc, possibly leading to the infamous "Remote 'g'
packet reply is too long", if the received response is longer than
expected.

So, I agree that we need some feature flag for this.

Simon