[Lldb-commits] [PATCH] D82863: [LLDB] Add support to resize SVE registers at run-time

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D82863#2324560 <https://reviews.llvm.org/D82863#2324560>, @omjavaid wrote:

> In D82863#2321647 <https://reviews.llvm.org/D82863#2321647>, @labath wrote:
>
>> In D82863#2321370 <https://reviews.llvm.org/D82863#2321370>, @omjavaid wrote:
>>
>>> In D82863#2320342 <https://reviews.llvm.org/D82863#2320342>, @jasonmolenda wrote:
>>>
>>>> The original g/G packets were designed for little embedded systems where the stub had to be very small and dumb, and could just memcpy the payload in the packet into the register context & back out again.  Any sensible design today would, at least, have some form of an array of regnum:regval to eliminate many of the problems.
>>>>
>>>>> Unless of course, we make sure SVE regs come last, but that imposes some constraints on future registers sets. I suppose we might be able to say that all variable-length or otherwise-funny registers must come last.
>>>>
>>>> Yeah, Omair's patch currently assumes that the SVE regs come last already when they copy & truncate the registers context to heap.  I fear that we'll get to armv12 and we'll be adding registers after the SVE and wonder why they're being truncated somewhere in lldb. :)
>>
>> Well.. we could design it such that the SVE registers (and any other dynamically-sized regs) always come last. Then they wouldn't impact the offsets of static registers.
>>
>> I don't think we have a requirement that newer register sets must be appended. Or at least, we shouldn't have, as now both intel and arm have cpu features (and registers) that may be arbitrarily combined in future processors.
>
> For now I am considering variable sized registers i-e SVE Z and P registers to always come last which is currently the case and will be in future patches which add support for Pointer Authentication and MTE registers.
>
>>>> @omjavaid , what do you think about disabling g/G when we're working with SVE registers (GDBRemoteCommunicationClient::AvoidGPackets)?  There are some gdb-remote stubs that can *only* read/write registers with g/G, but I think it's reasonable to say "you must implement p/P for a target with SVE", that's a generic packet shared by both lldb and gdb.  We could add a more-modern g/G replacement packet, but no one would have that implemented, and if they were going to add anything, I'd have them implement p/P unless it's perf problems and they need a read-all-registers / write-all-registers packet that works with SVE.
>
> Just scrolling through GDB log one of the answers about target XML is that target XML is not exchanged for native connection where gdbserver is not being used. For gdbserver connection, target xml is exchanged onces per connection and register sizes are updated based on vg after that internally. However testing gdb with same executable as the one I wrote for LLDB was having gdb remote connection crash which could be something to do with SVE or some other bug.

Interesting.

I started drafting an email to the gdb mailing list to try to clarify this thing. For that, I was re-reading the gdb docs for the qXfer:target.xml packet,  I realized that the gdb interpretation is mostly unambiguous about this thing:

- **regnum**: The register’s number. If omitted, <default value algorithm...>. This register number is used to read or write the register; e.g. it is used in the remote p and P packets, and **registers appear in the g and G packets in order of increasing register number**.

I'm sure that whoever wrote this did not have variable-length registers in mind. However, it's interpretation for variable-length registers is pretty clear: take each register, and dump its bytes in sequence (no matter many of them are in the register at this particular time). The reason this is unambiguous is because gdb does not have the "offset" attribute of the "reg" element, and so the only way to find its 'g' offset is via this algorithm. And given your explanation about "switching" target.xml descriptions at runtime, I'm would expect this is what is happening within gdb.

The problem is that he "offset" attribute (added by lldb) makes things ambiguous, as it provides another way to find a register in the 'g' blob. So how about we "fix" that ambiguity by having lldb-server omit the "offset" attribute for variable-length registers?

Sending it over is actively harmful, as the value is very likely to be wrong, and I believe that in your current implementation these numbers are pretty much ignored by the client anyway. Lldb should already have code to automatically compute the regiser offsets when the "offset" attribute is missing (though it's possible it may not handle partially missing values correctly). This code would need to be extended to recompute those offsets whenever register sizes change (which is roughly what this patch does). We could also extend the parser with a check to ensure that all registers with explicit offsets come before offset-less registers. That would ensure that the register offsets dynamically computed by the client never overlap with any registers whose offsets are explicitly given by the server.

How does that sound?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82863/new/

https://reviews.llvm.org/D82863