Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Luis Machado <luis.machado@arm.com> writes:

> On 12/7/22 19:25, Simon Marchi wrote:
>> Reading this, I thought of another potential problem with using
>> expedited registers to communicate the architectures change.  When using
>> the non-stop version of the remote protocol, we will have a stop reply
>> for each thread that stops, so that's fine, we can infer each thread's
>> tdesc.  But with the all-stop variant of the protocol, you only get a
>> stop reply for one thread, for a given stop.
>
> I wasn't sure about this case, so I mentioned it in the first reply to
> patch 6/6. Isn't it possible to send back information about all the
> threads that have stopped in all-stop mode?

I think it could be done by adding the extra information in the XML of
the qXfer:threads:read reply as Simon suggested here.

I thought about adding the expedited registers for each thread to fix
this case, but as long as we are making changes to the remote protocol,
Simon's idea is cleaner and more general.

>> For instance, imagine you have two threads, thread 1 hits a breakpoint.
>> GDB gets a stop reply for thread 1, and thread 2 is just considered stop
>> because we're using the all-stop remote protocol.  If we need to read
>> thread 2's registers, we would need to know its tdesc.  Its vg may have
>> changed since last time, or it might be the first time we learned about
>> it.  In that case, I don't think we have a way out of adding some mean
>> for gdbserver to tell gdb which tdesc applies to that thread.
>> 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
>> additional overhead would be:
>
> The aarch64 port uses feature hashes for the target descriptions. We
> could leverage that, but we would need to keep the hash stable so
> older gdb's that don't know about certain features don't break/get
> confused.

IIUC the tdesc ids are defined by gdbserver and are opaque to GDB, and
also they are only meaningful within a single connection, so they don't
need to be stable.

Because of that, I think small integers are preferable to hashes because
they are shorter so it's less data to push through the wire.

>>   - fetching each XML tdesc once, that seems inevitable (we just don't
>>     want to fetch the same XML tdesc over and over)
>>   - a few characters more in stop replies and qXfer:threads:read replies
>> I believe this could be implemented in a backwards compatible way
>> without even adding a new qSupported feature.  XML is extensible by
>> nature.  And stop replies are too.  The T stop reply doc says:
>>    Otherwise, GDB should ignore this ‘n:r’ pair and go on to the next;
>>    this allows us to extend the protocol in the future.
>> So in theory we could add a `tdesc-id:1234` field without breaking old
>> GDBs.  However, those old GDBs would break when trying to read registers
>> of threads with unexpected SVE lengths.
>
> It sounds like a good approach to me, and slightly better than using
> the expedited registers.

I agree. Thanks Simon for this idea and for detailing it! I'll take a
stab at implementing it for v3.

> It would be nice if we could minimize the amount of tdesc-id entries
> for all the threads, if they're mostly the same.
>
> The most obvious case is having hundreds of threads that all have the
> same tdesc-id. It wouldn't be nice to duplicate all that data.

gdbserver can assign the same id for identical tdescs, so I think this
will happen naturally.

> Originally we discussed a packet to ask the remote if the target
> description had changed. If nothing's changed, it is a quick reply.
>
> I suppose we could have something similar to that here as well, to
> optimize things, that is, don't return data if nothing's changed.

Yes, if it happens that GDB needs to fetch registers without having
received a tdesc-updating T stop reply or qXfer:threads:read reply then
this is a good solution.

-- 
Thiago