[Lldb-commits] [PATCH] D92063: [LLDB] RegisterInfoPOSIX_arm64 remove unused bytes from g/G packet

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D92063#2417989 <https://reviews.llvm.org/D92063#2417989>, @mgorny wrote:

> I was referring to:
>
>> Ideally offset calculation should look something like this: reg[index].byte_offset = reg[index - 1].byte_offset + reg[index - 1].byte_size.
>
> I'm not saying this is wrong for the time being but IMO we should assume that we might need to have a non-obvious offset in the future.

Ok. I think I understand what you meant now. Overall, I think that having the registers placed in the `g` packet in the right order and without any gaps (as the spec prescribes) is a good idea. Doing that cleanly might not be so easy though.

The way I see it, our main problem is that the RegisterInfo struct just serves too many masters. The `byte_offset` field in particular is used both as a gdb-remote offset, and as the offset into various os data structures.

Sometimes one can use the same offset for both numbers, and then everything is fine. But there are cases where this is not possible and then things start to get ugly.

I don't think that adding another field to this struct is a good solution, as it does not scale. In reality, noone needs to know both numbers. NativeXXXProcess only deals with the ptrace and it doesn't (shouldn't) care about gdb-remote offsets. gdb-remote code only cares about laying out the `g` packet and does not care how the register values are obtained.

One solution for this would be to invert our representation of register information (vector of structs => struct of vectors). That way it would be easy for anyone to add a parallel vector to represent any additional register information it wants, and the rest of the code could just ignore that. llvm's register information is organized is a somewhat similar way.

But that's a pretty long way away. For now we have to figure out a way to share the offset fields, and I think this patch makes a good effort at that.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h:99-100
 
+  size_t GetGPRBufferSize() { return sizeof(m_gpr_arm64); }
+
 private:
----------------
Please put this next to the `GetGPRBuffer` function. And add comments to explain the difference between the two.


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

https://reviews.llvm.org/D92063