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

Tue Oct 20 02:02:03 PDT 2020

omjavaid added a comment.

In D82863#2334664 <https://reviews.llvm.org/D82863#2334664>, @labath wrote:

> In D82863#2331892 <https://reviews.llvm.org/D82863#2331892>, @omjavaid wrote:
>
>> In D82863#2330122 <https://reviews.llvm.org/D82863#2330122>, @labath wrote:
>>
>>> 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?
>>
>> I think the algorithm you are suggestion is quite workable. Summarizing our discussion above:
>>
>> 1. registers should be sent over by lldb-server (via qRegisterInfo packet or target XML packet) in order of increasing register numbers
>
> Yep.
>
>> 2. If its a non-pseudo register that is not contained by any other register then its place is decided on the bases of its sequence via register number
>
> I'm not sure if that's consistent with what gdb does, but if that is what we are doing right now, then I don't think we need to change that on account of this.
>
>> This makes a register's name, size , type encoding and whether it is contained by any other register, the most important information while the remaining information including (dwarf and eh_frame reg numbers can be avoided somehow not sure exactly how??)
>> Also there are two fields that are sent over invalidate-regs and container-regs I think they are one and same thing. Moreover the encoding and format are also two difference field where I think we need only one of these.
>
> Yeah, that sounds like the direction we want to move in, but I don't think we have to do anything about this straight away as (IIUC) this is not blocking you -- we are able to put something meaningful into those fields of SVE registers, aren't we?
>
> I don't think you should refactor the entire lldb register passing conventions -- I just want to make sure anything new we come up with is consistent with the general direction and does not needlessly diverge from gdb. The main new thing here is variable-sized registers -- solving that should be quite sufficient.

As far as no lldb-server stubs are concerned we are mostly dealingin with gdb-server, QEMU stub and OpenOCD stub. We have already discussed gdb-server and I have given a look to QEMU's implementation of SVE registers handling in its target xml creation. Looking at arm_gen_dynamic_svereg_xml in https://github.com/qemu/qemu/blob/master/target/arm/gdbstub.c

I think LLDB has already diverged from gdb in the sense that when we create register infos we depend on hard-coded offsets and register numbers. This is not the case with either gdb-server or QEMU stub.

On client side LLDB does recieve registers in sequence and also calculates a offset locally but then ignores it in the favor of the one which is being received from lldb-server. There needs to be an incrementat effort where we first get rid of the dependence on fixed offsets and register numbers in NativeProcess and LLGS code. Then fix the LLDB client to act accordingly in sync with all types of stub implementation taking guidance from gdb protocol.

As far as SVE is concerned I dont think current implementation will cause any problem as long as we can keep SVE registers to come last if not that then also adjust offests of any register that are past SVE registers. For now there is no registers past SVE and they are ones which are coming last in our sequence so we seem to be good for now.

@labath What do you think?

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

https://reviews.llvm.org/D82863