dvlahovski created this revision.
dvlahovski added reviewers: labath, zturner.
dvlahovski added subscribers: amccarth, lldb-commits.

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.


https://reviews.llvm.org/D25677

Files:
  source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h

Index: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
===================================================================
--- source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
+++ source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
@@ -21,81 +21,160 @@
 
 #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_struct {
+  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_struct float_registers[8];
+  uint128_struct 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_struct header[2];
+      uint128_struct legacy[8];
+      uint128_struct xmm0;
+      uint128_struct xmm1;
+      uint128_struct xmm2;
+      uint128_struct xmm3;
+      uint128_struct xmm4;
+      uint128_struct xmm5;
+      uint128_struct xmm6;
+      uint128_struct xmm7;
+      uint128_struct xmm8;
+      uint128_struct xmm9;
+      uint128_struct xmm10;
+      uint128_struct xmm11;
+      uint128_struct xmm12;
+      uint128_struct xmm13;
+      uint128_struct xmm14;
+      uint128_struct xmm15;
+    } sse_registers;
+  };
+
+  enum {
+    VRCount = 26,
+  };
+
+  uint128_struct 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();
 
Index: source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
===================================================================
--- source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
+++ source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
@@ -19,10 +19,9 @@
 using namespace lldb_private;
 using namespace minidump;
 
-void writeRegister(llvm::ArrayRef<uint8_t> &reg_src,
+void writeRegister(const void *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());
+  memcpy(reg_dest.data(), reg_src, reg_dest.size());
 }
 
 llvm::MutableArrayRef<uint8_t> getDestRegister(uint8_t *context,
@@ -58,98 +57,89 @@
       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
-  }
+  const MinidumpContext_x86_64 *context;
+  consumeObject(source_data, context);
 
-  source_data = source_data.drop_front(4); // mx_csr
+  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 & ControlFlag) {
-    writeRegister(source_data, getDestRegister(result_base, lldb_cs_x86_64,
-                                               reg_info[lldb_cs_x86_64]));
+  if ((context_flags & x86_64_Flag) != x86_64_Flag) {
+    return result_context_buf; // error
   }
 
-  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) == ControlFlag) {
+    writeRegister(&context->cs, getDestRegister(result_base, lldb_cs_x86_64,
+                                                reg_info[lldb_cs_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 & SegmentsFlag) == SegmentsFlag) {
+    writeRegister(&context->ds, getDestRegister(result_base, lldb_ds_x86_64,
+                                                reg_info[lldb_ds_x86_64]));
+    writeRegister(&context->es, getDestRegister(result_base, lldb_es_x86_64,
+                                                reg_info[lldb_es_x86_64]));
+    writeRegister(&context->fs, getDestRegister(result_base, lldb_fs_x86_64,
+                                                reg_info[lldb_fs_x86_64]));
+    writeRegister(&context->gs, getDestRegister(result_base, lldb_gs_x86_64,
+                                                reg_info[lldb_gs_x86_64]));
   }
 
-  if (*context_flags & DebugRegistersFlag) {
-    source_data =
-        source_data.drop_front(6 * 8); // 6 debug registers 64 bit each
+  if ((context_flags & ControlFlag) == ControlFlag) {
+    writeRegister(&context->ss, getDestRegister(result_base, lldb_ss_x86_64,
+                                                reg_info[lldb_ss_x86_64]));
+    writeRegister(&context->eflags,
+                  getDestRegister(result_base, lldb_rflags_x86_64,
+                                  reg_info[lldb_rflags_x86_64]));
   }
 
-  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 & IntegerFlag) == IntegerFlag) {
+    writeRegister(&context->rax, getDestRegister(result_base, lldb_rax_x86_64,
+                                                 reg_info[lldb_rax_x86_64]));
+    writeRegister(&context->rcx, getDestRegister(result_base, lldb_rcx_x86_64,
+                                                 reg_info[lldb_rcx_x86_64]));
+    writeRegister(&context->rdx, getDestRegister(result_base, lldb_rdx_x86_64,
+                                                 reg_info[lldb_rdx_x86_64]));
+    writeRegister(&context->rbx, 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 & ControlFlag) == ControlFlag) {
+    writeRegister(&context->rsp, 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 & IntegerFlag) == IntegerFlag) {
+    writeRegister(&context->rbp, getDestRegister(result_base, lldb_rbp_x86_64,
+                                                 reg_info[lldb_rbp_x86_64]));
+    writeRegister(&context->rsi, getDestRegister(result_base, lldb_rsi_x86_64,
+                                                 reg_info[lldb_rsi_x86_64]));
+    writeRegister(&context->rdi, getDestRegister(result_base, lldb_rdi_x86_64,
+                                                 reg_info[lldb_rdi_x86_64]));
+    writeRegister(&context->r8, getDestRegister(result_base, lldb_r8_x86_64,
+                                                reg_info[lldb_r8_x86_64]));
+    writeRegister(&context->r9, getDestRegister(result_base, lldb_r9_x86_64,
+                                                reg_info[lldb_r9_x86_64]));
+    writeRegister(&context->r10, getDestRegister(result_base, lldb_r10_x86_64,
+                                                 reg_info[lldb_r10_x86_64]));
+    writeRegister(&context->r11, getDestRegister(result_base, lldb_r11_x86_64,
+                                                 reg_info[lldb_r11_x86_64]));
+    writeRegister(&context->r12, getDestRegister(result_base, lldb_r12_x86_64,
+                                                 reg_info[lldb_r12_x86_64]));
+    writeRegister(&context->r13, getDestRegister(result_base, lldb_r13_x86_64,
+                                                 reg_info[lldb_r13_x86_64]));
+    writeRegister(&context->r14, getDestRegister(result_base, lldb_r14_x86_64,
+                                                 reg_info[lldb_r14_x86_64]));
+    writeRegister(&context->r15, 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 ((context_flags & ControlFlag) == ControlFlag) {
+    writeRegister(&context->rip, getDestRegister(result_base, lldb_rip_x86_64,
+                                                 reg_info[lldb_rip_x86_64]));
   }
 
   // TODO parse the floating point registers
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to