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

Wed Nov 4 03:33:34 PST 2020

omjavaid added a comment.

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

> I'm sorry about the delay, I was having trouble finding the time to think this through...
>
> In D82863#2342112 <https://reviews.llvm.org/D82863#2342112>, @omjavaid wrote:
>
>> In D82863#2341628 <https://reviews.llvm.org/D82863#2341628>, @labath wrote:
>>
>>> I'm afraid I don't understand what you mean. Could you try to be more specific?
>>>
>>> It's true that currently lldb client uses the register offsets provided by the server (if they are present), and this is a divergence from pure gdb-remote. Were it not for variable-sized registers, I might even say that it is a good one. What the current patch does though is add another exception to the rule. This exception means that the offset provided by the server may not actually be the definitive value -- the client may choose to override it based on some built-in knowledge (and then assume the server has the same knowledge).
>>>
>>> My proposal is to avoid this exception, and make it such that the server-provided offset is always definitive. And in cases where a definitive answer cannot be provided, the server will just not send the offset over.
>>
>> I agree with your thoughts about not sending offset, infact as far as AArch64 is concerned if we skip sending offset for SVE registers we can construct offset filed anyway by running UpdateARM64SVERegistersInfos in DynamicRegisterInfo::Finalize function.
>>
>> In principle offsets are always definitive and sending explicit Offset is actually redundant information we actually dont need it as it can be calculated based on register size in the increasing order of register numbers. If we know variable sized SVE registers are always going to come after VG register we can always know the correct location of these registers in g packet. We also know FFR is the last SVE register and any data after FFR will correspond to any new registers that may be added after SVE. What we essentially need here is to sync register numbers and register sizes between client and server.  The offset field in register info structure must be populated corrently on both client and server side for read/write of register data from its correct location in register data heap. In addition to that for variable sized registers we need a mechanism to update the size and offset fields of variable register and also the register coming after those registers.
>
> Could we make it so that  it's not necessary to update the register info offset information on the server size. And instead, compute them on the fly, when needed (and it's not explicitly specified)? 
> Something like:
>
>   Handle_g() {
>     for (info: regs) {
>       if (info.byte_offset == -1)
>         offset = next_offset;
>       else
>         offset = info.byte_offset;
>       memcpy(buffer+offset, reg_data, info.byte_size);
>       next_offset = offset + info.byte_size
>     }
>     send(buffer);
>   }
>
> Then qRegisterInfo (and target.xml) could just check for the -1 value and use that as a cue to skip sending the offset field? All current architectures always hard-code an offset, so this would be a no-op for them...

byte_offset field in RegisterInfo struct is also used elsewhere in code specially to copy data to-from cached register values in data heap buffer. Also on the server side this byte_offset field is used to calculate position of register data in ptrace buffer etc.
So we can not skip populating this field. But what we can do is that populate it with valid values replicating g/G packet positions which can be reconstructed using register sizes in increasing order of register numbers.

>> Secondly, this offset field also determines the location of value_regs without the offset field being explicitly sent across we cannot exactly be sure about the location of pseudo registers in g/G packet data. GDB's XML register description format does not have a provision for pseudo registers rather it has user defined register types what it calls composite types where a register can be vector, union or struct of various custom fields.
>
> I'm not sure I understand that. Can you elaborate on the relevance of value_regs here?

So value regs are pseudo register for example V register in Arm64 SVE is a subset of first 16-bytes of a Z register. We do not include value registers in g packet but then need a way to tell where in g/G packet data a specific Vx pseudo register will be located. In my updated patch I update offsets of all primary registers GPR, Z, P, FFR etc and also I copy that offset into their corresponding value registers for example for Z0, same offset is copied to V0, D0, S0. This helps with quick fetching of data on both client and server side.

>> Finally, how about I post an update of this patch where lldb client side will ignore offset it receives for SVE register and incrementally places variable sized SVE registers after VG register. For any future registers we can choose an to put them before SVE registers or add a mechanism to update their offset if they are placed after SVE.
>
> I'd like to avoid sending bogus offsets, if possible. I'd rather that we teach the server to not send them, and then have the client use this as a cue that it needs to recompute something.

I dont want to write -1 into offset field on server side because it is good helper for accessing data in register buffers but we can introduce a mechanism for all architectures supporting g/G packet we should not send the offset field in qRegisterInfo or target xml. The offset will be constructed by client itself and populated on client side register infos list for helping out with register data management.

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

https://reviews.llvm.org/D82863