Minidump plugin: redesign the x86_64 register context

[dvlahovski]

Summary

I misunderstood the format of the register context layout.
I thought it was a dynamically changing structure, and that it's size
depended on context_flags.
It turned out that it always has the same fixed layout and size,
and the context_flags says which fields of the
struct have valid values.
This required a minor redesign of the register context class.

The layout inconsistency, however, was not a "problem" before (e.g. the plugin was working)
because there also was a bug with checking context_flags - the code was
parsing the entire struct regardless of context_flags.
This bug is also fixed in this commit.

[dvlahovski]

Adding sanity check for the context's size

[labath]

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp

61 ↗

(On Diff #74852)

This looks like error handling. Should we return an empty buffer in that case (so that the caller can see that we actually failed in parsing this)?

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h

141 ↗

(On Diff #74852)

uint128_struct xmm[16]; ?

I'm not that sure about that struct and it's name though, I'll need to think about that.

[dvlahovski]

Return a nullptr in case of an error

[dvlahovski]

Make a single array of sse registers instead of separated fields

[dvlahovski]

Merge if's checking the same context flag

[labath]

Just a couple of cleanups and then I think we're ready.

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp

78 ↗

(On Diff #74987)

These would be cleaner, if we made writeRegister call getDestRegister internally.
Then these two lines would become:

writeRegister(&context->cs, result_base, reg_info[lldb_cs_x86_64]);

Note that you don't have to pass in lldb_cs_x86_64 explicitly, as that information can be recovered via reg_info->kinds[eRegisterKindLLDB].

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h

141 ↗

(On Diff #74852)

I don't really have a good idea about struct issue. Let's at least call it Uint128, so that it's consistent with the rest of the struct's in this class, and then call it a day.

[dvlahovski]

Simplified writeRegister function

s/uint128_struct/Uint128/

[labath]

Looks much better, thanks. :)

lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h

 // Other libraries and framework includes
 #include "Plugins/Process/Utility/RegisterInfoInterface.h"
 #include "Plugins/Process/Utility/lldb-x86-register-enums.h"
 
 #include "lldb/Target/RegisterContext.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitmaskEnum.h"
+#include "llvm/Support/Endian.h"
 
 // C includes
 // C++ includes
 
 namespace lldb_private {
 
 namespace minidump {
 
-// The content of the Minidump register context is as follows:
-// (for reference see breakpad's source or WinNT.h)
-// Register parameter home addresses: (p1_home .. p6_home)
-// - uint64_t p1_home
-// - uint64_t p2_home
-// - uint64_t p3_home
-// - uint64_t p4_home
-// - uint64_t p5_home
-// - uint64_t p6_home
-//
-// - uint32_t context_flags - field that determines the layout of the structure
-//     and which parts of it are populated
-// - uint32_t mx_csr
-//
-// - uint16_t cs - included if MinidumpContext_x86_64_Flags::Control
-//
-// - uint16_t ds - included if MinidumpContext_x86_64_Flags::Segments
-// - uint16_t es - included if MinidumpContext_x86_64_Flags::Segments
-// - uint16_t fs - included if MinidumpContext_x86_64_Flags::Segments
-// - uint16_t gs - included if MinidumpContext_x86_64_Flags::Segments
-//
-// - uint16_t ss     - included if MinidumpContext_x86_64_Flags::Control
-// - uint32_t rflags - included if MinidumpContext_x86_64_Flags::Control
-//
-// Debug registers: (included if MinidumpContext_x86_64_Flags::DebugRegisters)
-// - uint64_t dr0
-// - uint64_t dr1
-// - uint64_t dr2
-// - uint64_t dr3
-// - uint64_t dr6
-// - uint64_t dr7
-//
-// The next 4 registers are included if MinidumpContext_x86_64_Flags::Integer
-// - uint64_t rax
-// - uint64_t rcx
-// - uint64_t rdx
-// - uint64_t rbx
-//
-// - uint64_t rsp - included if MinidumpContext_x86_64_Flags::Control
-//
-// The next 11 registers are included if MinidumpContext_x86_64_Flags::Integer
-// - uint64_t rbp
-// - uint64_t rsi
-// - uint64_t rdi
-// - uint64_t r8
-// - uint64_t r9
-// - uint64_t r10
-// - uint64_t r11
-// - uint64_t r12
-// - uint64_t r13
-// - uint64_t r14
-// - uint64_t r15
-//
-// - uint64_t rip - included if MinidumpContext_x86_64_Flags::Control
-//
-// TODO: add floating point registers here
-
 // This function receives an ArrayRef pointing to the bytes of the Minidump
 // register context and returns a DataBuffer that's ordered by the offsets
 // specified in the RegisterInfoInterface argument
 // This way we can reuse the already existing register contexts
 lldb::DataBufferSP
 ConvertMinidumpContextToRegIface(llvm::ArrayRef<uint8_t> source_data,
                                  RegisterInfoInterface *target_reg_interface);
 
+struct Uint128 {
+  llvm::support::ulittle64_t high;
+  llvm::support::ulittle64_t low;
+};
+
+// Reference: see breakpad/crashpad source or WinNT.h
+struct MinidumpXMMSaveArea32AMD64 {
+  llvm::support::ulittle16_t control_word;
+  llvm::support::ulittle16_t status_word;
+  uint8_t tag_word;
+  uint8_t reserved1;
+  llvm::support::ulittle16_t error_opcode;
+  llvm::support::ulittle32_t error_offset;
+  llvm::support::ulittle16_t error_selector;
+  llvm::support::ulittle16_t reserved2;
+  llvm::support::ulittle32_t data_offset;
+  llvm::support::ulittle16_t data_selector;
+  llvm::support::ulittle16_t reserved3;
+  llvm::support::ulittle32_t mx_csr;
+  llvm::support::ulittle32_t mx_csr_mask;
+  Uint128 float_registers[8];
+  Uint128 xmm_registers[16];
+  uint8_t reserved4[96];
+};
+
+struct MinidumpContext_x86_64 {
+  // Register parameter home addresses.
+  llvm::support::ulittle64_t p1_home;
+  llvm::support::ulittle64_t p2_home;
+  llvm::support::ulittle64_t p3_home;
+  llvm::support::ulittle64_t p4_home;
+  llvm::support::ulittle64_t p5_home;
+  llvm::support::ulittle64_t p6_home;
+
+  // The context_flags field determines and which parts
+  // of the structure are populated (have valid values)
+  llvm::support::ulittle32_t context_flags;
+  llvm::support::ulittle32_t mx_csr;
+
+  // The next register is included with
+  // MinidumpContext_x86_64_Flags::Control
+  llvm::support::ulittle16_t cs;
+
+  // The next 4 registers are included with
+  // MinidumpContext_x86_64_Flags::Segments
+  llvm::support::ulittle16_t ds;
+  llvm::support::ulittle16_t es;
+  llvm::support::ulittle16_t fs;
+  llvm::support::ulittle16_t gs;
+
+  // The next 2 registers are included with
+  // MinidumpContext_x86_64_Flags::Control
+  llvm::support::ulittle16_t ss;
+  llvm::support::ulittle32_t eflags;
+
+  // The next 6 registers are included with
+  // MinidumpContext_x86_64_Flags::DebugRegisters
+  llvm::support::ulittle64_t dr0;
+  llvm::support::ulittle64_t dr1;
+  llvm::support::ulittle64_t dr2;
+  llvm::support::ulittle64_t dr3;
+  llvm::support::ulittle64_t dr6;
+  llvm::support::ulittle64_t dr7;
+
+  // The next 4 registers are included with
+  // MinidumpContext_x86_64_Flags::Integer
+  llvm::support::ulittle64_t rax;
+  llvm::support::ulittle64_t rcx;
+  llvm::support::ulittle64_t rdx;
+  llvm::support::ulittle64_t rbx;
+
+  // The next register is included with
+  // MinidumpContext_x86_64_Flags::Control
+  llvm::support::ulittle64_t rsp;
+
+  // The next 11 registers are included with
+  // MinidumpContext_x86_64_Flags::Integer
+  llvm::support::ulittle64_t rbp;
+  llvm::support::ulittle64_t rsi;
+  llvm::support::ulittle64_t rdi;
+  llvm::support::ulittle64_t r8;
+  llvm::support::ulittle64_t r9;
+  llvm::support::ulittle64_t r10;
+  llvm::support::ulittle64_t r11;
+  llvm::support::ulittle64_t r12;
+  llvm::support::ulittle64_t r13;
+  llvm::support::ulittle64_t r14;
+  llvm::support::ulittle64_t r15;
+
+  // The next register is included with
+  // MinidumpContext_x86_64_Flags::Control
+  llvm::support::ulittle64_t rip;
+
+  // The next set of registers are included with
+  // MinidumpContext_x86_64_Flags:FloatingPoint
+  union FPR {
+    MinidumpXMMSaveArea32AMD64 flt_save;
+    struct {
+      Uint128 header[2];
+      Uint128 legacy[8];
+      Uint128 xmm[16];
+    } sse_registers;
+  };
+
+  enum {
+    VRCount = 26,
+  };
+
+  Uint128 vector_register[VRCount];
+  llvm::support::ulittle64_t vector_control;
+
+  // The next 5 registers are included with
+  // MinidumpContext_x86_64_Flags::DebugRegisters
+  llvm::support::ulittle64_t debug_control;
+  llvm::support::ulittle64_t last_branch_to_rip;
+  llvm::support::ulittle64_t last_branch_from_rip;
+  llvm::support::ulittle64_t last_exception_to_rip;
+  llvm::support::ulittle64_t last_exception_from_rip;
+};
+
 // For context_flags. These values indicate the type of
-// context stored in the structure.  The high 24 bits identify the CPU, the
+// context stored in the structure. The high 24 bits identify the CPU, the
 // low 8 bits identify the type of context saved.
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 
 enum class MinidumpContext_x86_64_Flags : uint32_t {
   x86_64_Flag = 0x00100000,
   Control = x86_64_Flag | 0x00000001,
   Integer = x86_64_Flag | 0x00000002,
   Segments = x86_64_Flag | 0x00000004,

lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp

 #include "lldb/Core/DataBufferHeap.h"
 
 // C includes
 // C++ includes
 
 using namespace lldb_private;
 using namespace minidump;
 
-void writeRegister(llvm::ArrayRef<uint8_t> &reg_src,
-                   llvm::MutableArrayRef<uint8_t> reg_dest) {
-  memcpy(reg_dest.data(), reg_src.data(), reg_dest.size());
-  reg_src = reg_src.drop_front(reg_dest.size());
-}
-
 llvm::MutableArrayRef<uint8_t> getDestRegister(uint8_t *context,
-                                               uint32_t lldb_reg_num,
                                                const RegisterInfo &reg) {
   auto bytes = reg.mutable_data(context);
 
-  switch (lldb_reg_num) {
+  switch (reg.kinds[lldb::eRegisterKindLLDB]) {
   case lldb_cs_x86_64:
   case lldb_ds_x86_64:
   case lldb_es_x86_64:
   case lldb_fs_x86_64:
   case lldb_gs_x86_64:
   case lldb_ss_x86_64:
     return bytes.take_front(2);
     break;
   case lldb_rflags_x86_64:
     return bytes.take_front(4);
     break;
   default:
     return bytes.take_front(8);
     break;
   }
 }
 
+void writeRegister(const void *reg_src, uint8_t *context,
+                   const RegisterInfo &reg) {
+  llvm::MutableArrayRef<uint8_t> reg_dest = getDestRegister(context, reg);
+  memcpy(reg_dest.data(), reg_src, reg_dest.size());
+}
+
 lldb::DataBufferSP lldb_private::minidump::ConvertMinidumpContextToRegIface(
     llvm::ArrayRef<uint8_t> source_data,
     RegisterInfoInterface *target_reg_interface) {
 
   const RegisterInfo *reg_info = target_reg_interface->GetRegisterInfo();
 
   lldb::DataBufferSP result_context_buf(
       new DataBufferHeap(target_reg_interface->GetGPRSize(), 0));
   uint8_t *result_base = result_context_buf->GetBytes();
 
-  source_data = source_data.drop_front(6 * 8); // p[1-6] home registers
-  const uint32_t *context_flags;
-  consumeObject(source_data, context_flags);
-  const uint32_t x86_64_Flag =
-      static_cast<uint32_t>(MinidumpContext_x86_64_Flags::x86_64_Flag);
-  const uint32_t ControlFlag =
-      static_cast<uint32_t>(MinidumpContext_x86_64_Flags::Control);
-  const uint32_t IntegerFlag =
-      static_cast<uint32_t>(MinidumpContext_x86_64_Flags::Integer);
-  const uint32_t SegmentsFlag =
-      static_cast<uint32_t>(MinidumpContext_x86_64_Flags::Segments);
-  const uint32_t DebugRegistersFlag =
-      static_cast<uint32_t>(MinidumpContext_x86_64_Flags::DebugRegisters);
-
-  if (!(*context_flags & x86_64_Flag)) {
-    return result_context_buf; // error
-  }
-
-  source_data = source_data.drop_front(4); // mx_csr
-
-  if (*context_flags & ControlFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_cs_x86_64,
-                                               reg_info[lldb_cs_x86_64]));
-  }
-
-  if (*context_flags & SegmentsFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_ds_x86_64,
-                                               reg_info[lldb_ds_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_es_x86_64,
-                                               reg_info[lldb_es_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_fs_x86_64,
-                                               reg_info[lldb_fs_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_gs_x86_64,
-                                               reg_info[lldb_gs_x86_64]));
-  }
-
-  if (*context_flags & ControlFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_ss_x86_64,
-                                               reg_info[lldb_ss_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_rflags_x86_64,
-                                               reg_info[lldb_rflags_x86_64]));
-  }
-
-  if (*context_flags & DebugRegistersFlag) {
-    source_data =
-        source_data.drop_front(6 * 8); // 6 debug registers 64 bit each
-  }
-
-  if (*context_flags & IntegerFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_rax_x86_64,
-                                               reg_info[lldb_rax_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_rcx_x86_64,
-                                               reg_info[lldb_rcx_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_rdx_x86_64,
-                                               reg_info[lldb_rdx_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_rbx_x86_64,
-                                               reg_info[lldb_rbx_x86_64]));
-  }
-
-  if (*context_flags & ControlFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_rsp_x86_64,
-                                               reg_info[lldb_rsp_x86_64]));
-  }
-
-  if (*context_flags & IntegerFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_rbp_x86_64,
-                                               reg_info[lldb_rbp_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_rsi_x86_64,
-                                               reg_info[lldb_rsi_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_rdi_x86_64,
-                                               reg_info[lldb_rdi_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r8_x86_64,
-                                               reg_info[lldb_r8_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r9_x86_64,
-                                               reg_info[lldb_r9_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r10_x86_64,
-                                               reg_info[lldb_r10_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r11_x86_64,
-                                               reg_info[lldb_r11_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r12_x86_64,
-                                               reg_info[lldb_r12_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r13_x86_64,
-                                               reg_info[lldb_r13_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r14_x86_64,
-                                               reg_info[lldb_r14_x86_64]));
-    writeRegister(source_data, getDestRegister(result_base, lldb_r15_x86_64,
-                                               reg_info[lldb_r15_x86_64]));
-  }
-
-  if (*context_flags & ControlFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_rip_x86_64,
-                                               reg_info[lldb_rip_x86_64]));
+  if (source_data.size() < sizeof(MinidumpContext_x86_64))
+    return nullptr;
+
+  const MinidumpContext_x86_64 *context;
+  consumeObject(source_data, context);
+
+  const MinidumpContext_x86_64_Flags context_flags =
+      static_cast<MinidumpContext_x86_64_Flags>(
+          static_cast<uint32_t>(context->context_flags));
+  auto x86_64_Flag = MinidumpContext_x86_64_Flags::x86_64_Flag;
+  auto ControlFlag = MinidumpContext_x86_64_Flags::Control;
+  auto IntegerFlag = MinidumpContext_x86_64_Flags::Integer;
+  auto SegmentsFlag = MinidumpContext_x86_64_Flags::Segments;
+
+  if ((context_flags & x86_64_Flag) != x86_64_Flag)
+    return nullptr;
+
+  if ((context_flags & ControlFlag) == ControlFlag) {
+    writeRegister(&context->cs, result_base, reg_info[lldb_cs_x86_64]);
+    writeRegister(&context->ss, result_base, reg_info[lldb_ss_x86_64]);
+    writeRegister(&context->eflags, result_base, reg_info[lldb_rflags_x86_64]);
+    writeRegister(&context->rsp, result_base, reg_info[lldb_rsp_x86_64]);
+    writeRegister(&context->rip, result_base, reg_info[lldb_rip_x86_64]);
+  }
+
+  if ((context_flags & SegmentsFlag) == SegmentsFlag) {
+    writeRegister(&context->ds, result_base, reg_info[lldb_ds_x86_64]);
+    writeRegister(&context->es, result_base, reg_info[lldb_es_x86_64]);
+    writeRegister(&context->fs, result_base, reg_info[lldb_fs_x86_64]);
+    writeRegister(&context->gs, result_base, reg_info[lldb_gs_x86_64]);
+  }
+
+  if ((context_flags & IntegerFlag) == IntegerFlag) {
+    writeRegister(&context->rax, result_base, reg_info[lldb_rax_x86_64]);
+    writeRegister(&context->rcx, result_base, reg_info[lldb_rcx_x86_64]);
+    writeRegister(&context->rdx, result_base, reg_info[lldb_rdx_x86_64]);
+    writeRegister(&context->rbx, result_base, reg_info[lldb_rbx_x86_64]);
+    writeRegister(&context->rbp, result_base, reg_info[lldb_rbp_x86_64]);
+    writeRegister(&context->rsi, result_base, reg_info[lldb_rsi_x86_64]);
+    writeRegister(&context->rdi, result_base, reg_info[lldb_rdi_x86_64]);
+    writeRegister(&context->r8, result_base, reg_info[lldb_r8_x86_64]);
+    writeRegister(&context->r9, result_base, reg_info[lldb_r9_x86_64]);
+    writeRegister(&context->r10, result_base, reg_info[lldb_r10_x86_64]);
+    writeRegister(&context->r11, result_base, reg_info[lldb_r11_x86_64]);
+    writeRegister(&context->r12, result_base, reg_info[lldb_r12_x86_64]);
+    writeRegister(&context->r13, result_base, reg_info[lldb_r13_x86_64]);
+    writeRegister(&context->r14, result_base, reg_info[lldb_r14_x86_64]);
+    writeRegister(&context->r15, result_base, reg_info[lldb_r15_x86_64]);
   }
 
   // TODO parse the floating point registers
 
   return result_context_buf;
 }