[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

In D91241#2391245 <https://reviews.llvm.org/D91241#2391245>, @labath wrote:

> In D91241#2391156 <https://reviews.llvm.org/D91241#2391156>, @omjavaid wrote:
>
>> I guess GDB standard does enforce ascending order, here is what it says about regnum:
>>
>> "The register’s number. If omitted, a register’s number is one greater than that of the previous register (either in the current feature or in a preceding feature); the first register in the target description defaults to zero. 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."
>>
>> https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html
>
> I've just read that paragraph before writing that, but that's not how I would interpret it. What I think that says is:
>
> - in the `g` packet, registers appear in the increasing register number order. (I think we agree on that part)
> - **if the register number is omitted**, the registers are assigned increasing register numbers. Here, I think the bold part is very important, as it means the rest of the sentence describes default/fallback behavior. If the stub specifies the register number explicitly, then I'd say it's free to send order the register descriptions any way it likes. It can even combine things and set explicit numbers for some  register, but not others...

I gave thought and looked at GDB handling of g/G packet offsets. 
Important points to note:

1. GDB assigns register numbers if they are not provided, with scheme last regnum + 1.
2. GDB maintains registers list in order of their placement in XML regardless of any specific register numbers.
3. GDB sends g/G packet based on register numbers in order of increasing register numbers. For this gdb creates a sorted list based of register numbers and then assigns offsets accordingly.

In LLDB we are trying to support legacy offset field which means offset calculation has to be done in location where we parse an XML register entry (ParseRegisters) or qRegisterInfo packet (ProcessGDBRemote::BuildDynamicRegisterInfo).

Also both of above functions are just inserting into register's information list maintained by DynamicRegisterInfo class so there is not enough leverage available to make offset calculation in one single location without giving up offset field altogether. We may write a helper function to do offset post processing for pseudo registers but that will still be a part of DynamicRegisterInfo class and may be called from ParseRegisters and ProcessGDBRemote::BuildDynamicRegisterInfo.

IMO, lets keep this current patch as it is unless we decide on giving up offset field for all architectures.


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

https://reviews.llvm.org/D91241