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

Mon Nov 16 04:35:50 PST 2020

labath added a comment.

I disagree, and my main reason for that is that this approach requires hardcoding the "supported architectures" for which we do this fixup in the DynamicRegisterInfo class. Besides being wrong as a matter of priniciple, I suspect this will also be a problem in practice -- for example, I have no idea what will happen when this code starts interacting with the aarch64 debugserver (which will still be sending the offsets). Doing this decision based on whether the server sends the offsets avoids this issue, and the process can be seen as an extension of what we're already doing when communicating with offset-less stubs.

In D91241#2396445 <https://reviews.llvm.org/D91241#2396445>, @omjavaid wrote:

> In LLDB we are trying to support legacy offset field

I wouldn't go so far as to call this a "legacy" field. Let's just say that for this particular problem on this architecture, we chose to stop using the 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).

Why is that? I don't see the connection here.

I don't see why the offsets could not be computed in the DynamicRegisterInfo class. The `ParseRegisters`/`BuildDynamicRegisterInfo` functions could just refrain from filling in the offset fields (leave them as invalid, or something) if the server did not provide them. Then DynamicRegisterInfo could use that invalid value to know which offsets need filling in.

In fact, if we wanted to be truly faithful to the gdb algorithm you described ("gdb creates a sorted list based of register numbers and then assigns offsets accordingly"), then it seems like we would _have to_ do this as a separate step, once all register numbers are known.

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

https://reviews.llvm.org/D91241