Fix issue where GPR and FPR registers have overlapping byte offsets.

[chaoren]

Summary

The implementation of GDBRemoteRegisterContext relies on byte offsets to cache
register values. GPR, FPR, etc. should start on different offsets. This is
correctly done in debugserver (in DNBArchImplX86_64.cpp), but not on Linux or
FreeBSD (in RegisterInfos_x86_64.h).

[chaoren]

source/Plugins/Process/Utility/RegisterContextFreeBSD_x86_64.cpp

55 ↗

(On Diff #22841)

This doesn't sound right, but I don't know enough about FreeBSD to dispute it.

[chaoren]

Ping? Anyone know who would be the right person to review this?

[clayborg]

Looks good to me. I usually create assembler files that load all registers up with known values and then stop at a label like "stop_here" and debug the program and verify all registers are where they should be. It would be a good idea for us to start doing this for all targets we support.

[chaoren]

I usually create assembler files that load all registers up with known
values and then stop at a label like "stop_here" and debug the program and
verify all registers are where they should be.

Does this mean a test like that already exists for OS X?

[clayborg]

No, I do this when first debugging a new Process plug-in or architecture. I wasn't sure about the assembler portability so I never checked on in.

[emaste]

This causes >20 tests to segfault on FreeBSD.

The debug register implementation on FreeBSD is a bit different, see rL201706. I can work around it for now by moving DR_OFFSET's definition back into RegisterContextFreeBSD_x86_64.cpp and RegisterContextLinux_x86_64.cpp.

[chaoren]

Whoops, I apologize. I'll fix it when I get to the office. The GPR/FPR
offset change doesn't cause any problems on FreeBSD right? I just sort of
just plopped a dummy UserArea in there for FreeBSD to compile. It would be
great if I could find where the Linux one came from, and where to get the
correct FreeBSD layout.

[emaste]

In D8685#151122, @chaoren wrote:

Whoops, I apologize. I'll fix it when I get to the office. The GPR/FPR
offset change doesn't cause any problems on FreeBSD right? I just sort of
just plopped a dummy UserArea in there for FreeBSD to compile. It would be
great if I could find where the Linux one came from, and where to get the
correct FreeBSD layout.

I'm happy to see this sort of cleanup happening and a little collateral damage is understandable. I've committed rL234048 as a temporary change to restore basic functionality and fix the buildbot; we'll want to investigate a bit further to come up with a clean fix as you describe.

lldb/trunk/source/Plugins/Process/Utility/RegisterContextFreeBSD_x86_64.cpp

typedef struct _GPR
     uint16_t ds;
     uint64_t rip;
     uint64_t cs;
     uint64_t rflags;
     uint64_t rsp;
     uint64_t ss;
 } GPR;
 
-struct dbreg {
-       uint64_t  dr[16];  /* debug registers */
-                          /* Index 0-3: debug address registers */
-                          /* Index 4-5: reserved */
-                          /* Index 6: debug status */
-                          /* Index 7: debug control */
-                          /* Index 8-15: reserved */
+struct DBG {
 };
 
-#define DR_SIZE sizeof(uint64_t)
-#define DR_OFFSET(reg_index) \
-    (LLVM_EXTENSION offsetof(dbreg, dr[reg_index]))
-
+struct UserArea
+{
+    GPR gpr;
+    FPR fpr;
+    DBG dbg;
+};
 
 //---------------------------------------------------------------------------
 // Include RegisterInfos_x86_64 to declare our g_register_infos_x86_64 structure.
 //---------------------------------------------------------------------------
 #define DECLARE_REGISTER_INFOS_X86_64_STRUCT
 #include "RegisterInfos_x86_64.h"
 #undef DECLARE_REGISTER_INFOS_X86_64_STRUCT
 

lldb/trunk/source/Plugins/Process/Utility/RegisterContextLinux_x86_64.cpp

typedef struct _GPR
     uint64_t fs_base;
     uint64_t gs_base;
     uint64_t ds;
     uint64_t es;
     uint64_t fs;
     uint64_t gs;
 } GPR;
 
+struct DBG {
+    uint64_t dr[8];
+};
+
 struct UserArea
 {
     GPR      gpr;           // General purpose registers.
     int32_t  fpvalid;       // True if FPU is being used.
     int32_t  pad0;
-    FXSAVE   i387;          // General purpose floating point registers (see FPR for extended register sets).
+    FXSAVE   fpr;           // General purpose floating point registers (see FPR for extended register sets).
     uint64_t tsize;         // Text segment size.
     uint64_t dsize;         // Data segment size.
     uint64_t ssize;         // Stack segment size.
     uint64_t start_code;    // VM address of text.
     uint64_t start_stack;   // VM address of stack bottom (top in rsp).
     int64_t  signal;        // Signal causing core dump.
     int32_t  reserved;      // Unused.
     int32_t  pad1;
     uint64_t ar0;           // Location of GPR's.
     FXSAVE*  fpstate;       // Location of FPR's.
     uint64_t magic;         // Identifier for core dumps.
     char     u_comm[32];    // Command causing core dump.
-    uint64_t u_debugreg[8]; // Debug registers (DR0 - DR7).
+    DBG      dbg;           // Debug registers.
     uint64_t error_code;    // CPU error code.
     uint64_t fault_address; // Control register CR3.
 };
 
-#define DR_SIZE sizeof(((UserArea*)NULL)->u_debugreg[0])
-#define DR_OFFSET(reg_index) \
-    (LLVM_EXTENSION offsetof(UserArea, u_debugreg[reg_index]))
-
 //---------------------------------------------------------------------------
 // Include RegisterInfos_x86_64 to declare our g_register_infos_x86_64 structure.
 //---------------------------------------------------------------------------
 #define DECLARE_REGISTER_INFOS_X86_64_STRUCT
 #include "RegisterInfos_x86_64.h"
 #undef DECLARE_REGISTER_INFOS_X86_64_STRUCT
 
 static std::vector<lldb_private::RegisterInfo>&

lldb/trunk/source/Plugins/Process/Utility/RegisterInfos_x86_64.h

 #include <stddef.h>
 
 // Computes the offset of the given GPR in the user data area.
 #define GPR_OFFSET(regname) \
     (LLVM_EXTENSION offsetof(GPR, regname))
 
 // Computes the offset of the given FPR in the extended data area.
 #define FPR_OFFSET(regname) \
-    (LLVM_EXTENSION offsetof(FPR, xstate) + \
+    (LLVM_EXTENSION offsetof(UserArea, fpr) + \
+     LLVM_EXTENSION offsetof(FPR, xstate) + \
      LLVM_EXTENSION offsetof(FXSAVE, regname))
 
 // Computes the offset of the YMM register assembled from register halves.
-#define YMM_OFFSET(regname) \
-    (LLVM_EXTENSION offsetof(YMM, regname))
+#define YMM_OFFSET(reg_index) \
+    (LLVM_EXTENSION offsetof(UserArea, fpr) + \
+     LLVM_EXTENSION offsetof(FPR, xstate) + \
+     LLVM_EXTENSION offsetof(XSAVE, ymmh[reg_index]) + \
+     (32 * reg_index))
+
+#define DR_OFFSET(reg_index) \
+    (LLVM_EXTENSION offsetof(UserArea, dbg) + \
+     LLVM_EXTENSION offsetof(DBG, dr[reg_index]))
 
 #ifdef DECLARE_REGISTER_INFOS_X86_64_STRUCT
 
 // Number of bytes needed to represent a FPR.
 #define FPR_SIZE(reg) sizeof(((FXSAVE*)NULL)->reg)
 
 // Number of bytes needed to represent the i'th FP register.
 #define FP_SIZE sizeof(((MMSReg*)NULL)->bytes)
 
 // Number of bytes needed to represent an XMM register.
 #define XMM_SIZE sizeof(XMMReg)
 
 // Number of bytes needed to represent a YMM register.
 #define YMM_SIZE sizeof(YMMReg)
 
+#define DR_SIZE sizeof(((DBG*)NULL)->dr[0])
+
 // RegisterKind: GCC, DWARF, Generic, GDB, LLDB
 
 // Note that the size and offset will be updated by platform-specific classes.
 #define DEFINE_GPR(reg, alt, kind1, kind2, kind3, kind4)    \
     { #reg, alt, sizeof(((GPR*)NULL)->reg), GPR_OFFSET(reg), eEncodingUint, \
       eFormatHex, { kind1, kind2, kind3, kind4, lldb_##reg##_x86_64 }, NULL, NULL }
 
 #define DEFINE_FPR(name, reg, kind1, kind2, kind3, kind4)    \

 
 #define DEFINE_XMM(reg, i)                                         \
     { #reg#i, NULL, XMM_SIZE, LLVM_EXTENSION FPR_OFFSET(reg[i]),   \
       eEncodingVector, eFormatVectorOfUInt8,                       \
       { gcc_dwarf_##reg##i##_x86_64, gcc_dwarf_##reg##i##_x86_64, LLDB_INVALID_REGNUM, gdb_##reg##i##_x86_64, lldb_##reg##i##_x86_64}, \
       NULL, NULL }
 
 #define DEFINE_YMM(reg, i)                                                          \
-    { #reg#i, NULL, YMM_SIZE, LLVM_EXTENSION YMM_OFFSET(reg[i]),                    \
+    { #reg#i, NULL, YMM_SIZE, LLVM_EXTENSION YMM_OFFSET(i),                         \
       eEncodingVector, eFormatVectorOfUInt8,                                        \
       { gcc_dwarf_##reg##i##h_x86_64, gcc_dwarf_##reg##i##h_x86_64, LLDB_INVALID_REGNUM, gdb_##reg##i##h_x86_64, lldb_##reg##i##_x86_64 }, \
       NULL, NULL }
 
 #define DEFINE_DR(reg, i)                                               \
     { #reg#i, NULL, DR_SIZE, DR_OFFSET(i), eEncodingUint, eFormatHex,   \
       { LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM,  \
       LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM }, NULL, NULL }

 
 #define UPDATE_XMM_INFO(reg, i)                                                 \
 do {                                                                            \
     g_register_infos[lldb_##reg##i##_i386].byte_offset = FPR_OFFSET(reg[i]);     \
 } while(false);
 
 #define UPDATE_YMM_INFO(reg, i)                                                 \
 do {                                                                            \
-    g_register_infos[lldb_##reg##i##_i386].byte_offset = YMM_OFFSET(reg[i]);     \
+    g_register_infos[lldb_##reg##i##_i386].byte_offset = YMM_OFFSET(i);         \
 } while(false);
 
 #define UPDATE_DR_INFO(reg_index)                                               \
 do {                                                                            \
     g_register_infos[lldb_dr##reg_index##_i386].byte_offset = DR_OFFSET(reg_index);  \
 } while(false);
 
     // Update the register offsets