[LLDB] RegisterInfoPOSIX_arm64 remove unused bytes from g/G packet

[omjavaid]

Summary

This came up while putting together our new strategy to create g/G packets where register offsets are calculated in increasing order of register numbers without any unused spacing. RegisterInfoPOSIX_arm64::GPR size was being calculated after alignment correction to 8 bytes which meant there 4 bytes unused space between last gpr (cpsr) and first vector register V. To remove any ambiguity I have placed a 4 byte pad at the end of RegisterInfoPOSIX_arm64::GPR and also subtracted the same from any offset calculation to avoid any unused fragment in g/G packet which will eventually break our offset calculation algorithm.

[labath]

+@mgorny, as he's been navigating these waters lately...

So... I presume we can't just slap __attribute__((packed)) on the structure, because the kernel actually expects that the data structure will have the extra space for the padding. Is that so?

Even if we can't, I'm wondering if it wouldn't be cleaner to use two structures for this. Something like:

LLVM_PACKED_START
struct GPR {
  // as before...
};
/// Big comment explaining the purpose of padding
struct GPRBuffer: GPR {
  uint32_t pad;
};
LLVM_PACKED_END

and then using GPR or GPRBuffer accordingly. What do you think?

[omjavaid]

In D92063#2416162, @labath wrote:

+@mgorny, as he's been navigating these waters lately...

So... I presume we can't just slap __attribute__((packed)) on the structure, because the kernel actually expects that the data structure will have the extra space for the padding. Is that so?

Even if we can't, I'm wondering if it wouldn't be cleaner to use two structures for this. Something like:

LLVM_PACKED_START
struct GPR {
  // as before...
};
/// Big comment explaining the purpose of padding
struct GPRBuffer: GPR {
  uint32_t pad;
};
LLVM_PACKED_END

and then using GPR or GPRBuffer accordingly. What do you think?

So I didnt check this before but FreeBSD and Linux have different ptrace GPR size expectation. Here is what FreeBSD struct looks like:

struct reg {
uint64_t x[30];
uint64_t lr;
uint64_t sp;
uint64_t elr;
uint32_t spsr;
};

While on Linux it looks something like this:

struct {
u64 regs[31];
u64 sp;
u64 pc;
u64 pstate;
};

So I am going to put a attribute((packed)) and use the same for FreeBSD while going to isolate Linux implementation in my next update.

[omjavaid]

Update as per my last comment.

[mgorny]

In D92063#2416162, @labath wrote:

+@mgorny, as he's been navigating these waters lately...

So... I presume we can't just slap __attribute__((packed)) on the structure, because the kernel actually expects that the data structure will have the extra space for the padding. Is that so?

Even if we can't, I'm wondering if it wouldn't be cleaner to use two structures for this. Something like:

LLVM_PACKED_START
struct GPR {
  // as before...
};
/// Big comment explaining the purpose of padding
struct GPRBuffer: GPR {
  uint32_t pad;
};
LLVM_PACKED_END

and then using GPR or GPRBuffer accordingly. What do you think?

That would imply adding additional offset field to the register lists, wouldn't it? Not that I'm opposed — it might be reasonable to have the option to override the offset for system structs, coredumps...

[omjavaid]

In D92063#2417815, @mgorny wrote:

In D92063#2416162, @labath wrote:

+@mgorny, as he's been navigating these waters lately...

So... I presume we can't just slap __attribute__((packed)) on the structure, because the kernel actually expects that the data structure will have the extra space for the padding. Is that so?

Even if we can't, I'm wondering if it wouldn't be cleaner to use two structures for this. Something like:

LLVM_PACKED_START
struct GPR {
  // as before...
};
/// Big comment explaining the purpose of padding
struct GPRBuffer: GPR {
  uint32_t pad;
};
LLVM_PACKED_END

and then using GPR or GPRBuffer accordingly. What do you think?

That would imply adding additional offset field to the register lists, wouldn't it? Not that I'm opposed — it might be reasonable to have the option to override the offset for system structs, coredumps...

Register infos should contain g/G packet offset and Ideally offset calculation should look something like this: reg[index].byte_offset = reg[index - 1].byte_offset + reg[index - 1].byte_size.

In case of AArch64 we are using GPR struct to calculate offsets which I think is inspired from the thinking that offset == ptrace offset rather than the g/G packet offset. Coincidentally ptrace offsets do no interfere with g/G packet offset the way we are calculating them right now except for this pad bytes added at the end. And I have fixed that anyway in my latest update.

[mgorny]

I disagree. Since we're repeating gdb protocol, it would be nice to use offsets consistent with the gdb protocol, even if it means some extra padding. I do realize that this is broken right now and not trivially fixable but I don't think we should make things worse.

[omjavaid]

In D92063#2417948, @mgorny wrote:

I disagree. Since we're repeating gdb protocol, it would be nice to use offsets consistent with the gdb protocol, even if it means some extra padding. I do realize that this is broken right now and not trivially fixable but I don't think we should make things worse.

I dont understand your disagreement. If you look at the current update of this patch, this is exactly what we are doing i-e Making offsets consistent with GDB protocol.

[labath]

(I haven't looked at the new changes yet.)

In D92063#2417948, @mgorny wrote:

I disagree. Since we're repeating gdb protocol, it would be nice to use offsets consistent with the gdb protocol, even if it means some extra padding.

I'm not sure what you mean by that. Are you implying that the gdb protocol (as implemented by gdb, let's say) does indeed have this padding in its g packet?

The point of this patch series is to make the our g packet more consistent with the "official" gdb-remote definition. The motivation for that are the SVE registers on arm which have a length that can change at runtime. The point of this makes the "offset" fields in the qRegisterInfo packets (and target.xml) meaningless. So we made a choice to just stop using them (in the packets, we still obviously need to know where the registers go) and have the client recompute the offsets according to the official algorithm. This means that there can be no random gaps in the packet data. The goal is to make the SVE implementation possible/saner and also bring us closer to the gdb's definition of these packets (which does not include an "offset" field in its target.xml).

[mgorny]

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.

[labath]

In 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.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

106–107

Please put this next to the GetGPRBuffer function. And add comments to explain the difference between the two.

[omjavaid]

Updated incorporating suggested changes.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h

protected:
   Status WriteGPR() override;
 
   Status ReadFPR() override;
 
   Status WriteFPR() override;
 
   void *GetGPRBuffer() override { return &m_gpr_arm64; }
 
+  // GetGPRBufferSize returns sizeof arm64 GPR ptrace buffer, it is different
+  // from GetGPRSize which returns sizeof RegisterInfoPOSIX_arm64::GPR.
+  size_t GetGPRBufferSize() { return sizeof(m_gpr_arm64); }
+
   void *GetFPRBuffer() override { return &m_fpr; }
 
   size_t GetFPRSize() override { return sizeof(m_fpr); }
 
 private:
   bool m_gpr_is_valid;
   bool m_fpu_is_valid;
   bool m_sve_buffer_is_valid;
 
   bool m_sve_header_is_valid;
 
-  RegisterInfoPOSIX_arm64::GPR m_gpr_arm64; // 64-bit general purpose registers.
+  struct user_pt_regs m_gpr_arm64; // 64-bit general purpose registers.
 
   RegisterInfoPOSIX_arm64::FPU
       m_fpr; // floating-point registers including extended register sets.
 
   SVEState m_sve_state;
   struct user_sve_header m_sve_header;
   std::vector<uint8_t> m_sve_ptrace_payload;
 

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

 Status NativeRegisterContextLinux_arm64::ReadGPR() {
   Status error;
 
   if (m_gpr_is_valid)
     return error;
 
   struct iovec ioVec;
   ioVec.iov_base = GetGPRBuffer();
-  ioVec.iov_len = GetGPRSize();
+  ioVec.iov_len = GetGPRBufferSize();
 
-  error = ReadRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS);
+  error = ReadRegisterSet(&ioVec, GetGPRBufferSize(), NT_PRSTATUS);
 
   if (error.Success())
     m_gpr_is_valid = true;
 
   return error;
 }
 
 Status NativeRegisterContextLinux_arm64::WriteGPR() {
   Status error = ReadGPR();
   if (error.Fail())
     return error;
 
   struct iovec ioVec;
   ioVec.iov_base = GetGPRBuffer();
-  ioVec.iov_len = GetGPRSize();
+  ioVec.iov_len = GetGPRBufferSize();
 
   m_gpr_is_valid = false;
 
-  return WriteRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS);
+  return WriteRegisterSet(&ioVec, GetGPRBufferSize(), NT_PRSTATUS);
 }
 
 Status NativeRegisterContextLinux_arm64::ReadFPR() {
   Status error;
 
   if (m_fpu_is_valid)
     return error;
 

lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h

public:
   // AArch64 Register set FP/SIMD feature configuration
   enum {
     eVectorQuadwordAArch64,
     eVectorQuadwordAArch64SVE,
     eVectorQuadwordAArch64SVEMax = 256
   };
 
   // based on RegisterContextDarwin_arm64.h
+  LLVM_PACKED_START
   struct GPR {
     uint64_t x[29]; // x0-x28
     uint64_t fp;    // x29
     uint64_t lr;    // x30
     uint64_t sp;    // x31
     uint64_t pc;    // pc
     uint32_t cpsr;  // cpsr
   };
+  LLVM_PACKED_END
 
   // based on RegisterContextDarwin_arm64.h
   struct VReg {
     uint8_t bytes[16];
   };
 
   // based on RegisterContextDarwin_arm64.h
   struct FPU {