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

Fri Oct 16 06:13:03 PDT 2020

labath added a comment.

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.

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

https://reviews.llvm.org/D82863