[lldb] Fix offset intersection bug between MPX and AVX registers

[guiandrade]

Summary

This change increases the offset of MPX registers (by 128) so they
do not overlap with the offset associated with AVX registers. That was
causing MPX data in GDBRemoteRegisterContext::m_reg_data to get overwritten.

[labath]

Thanks for doing this, and especially thank you for writing the test.

For posterity: more discussion on this patch and the problem it solves can be found in https://reviews.llvm.org/D62931.

lldb/packages/Python/lldbsuite/test/commands/register/register/intel_xtended_registers/mpx_offset_intersection/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/commands/register/register/intel_xtended_registers/mpx_offset_intersection/TestMPXOffsetIntersection.py

+"""
+Test Intel(R) MPX registers do not get overwritten by AVX data.
+"""
+
+from __future__ import print_function
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class MPXOffsetIntersectionTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    AVX_REGS = ('ymm' + str(i) for i in range(16))
+    YMM_VALUE = '{' + ' '.join(('0x00' for _ in range(32))) + '}'
+
+    MPX_REGULAR_REGS = ('bnd0', 'bnd1', 'bnd2', 'bnd3')
+    MPX_CONFIG_REGS = ('bndcfgu', 'bndstatus')
+    BND_VALUE = '{' + ' '.join(('0xff' for _ in range(16))) + '}'
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    @skipIf(oslist=no_match(['linux']))
+    @skipIf(archs=no_match(['x86_64']))
+    def test_mpx_registers_offset_intersection(self):
+        """Test if AVX data does not overwrite MPX values."""
+        self.build()
+        self.mpx_registers_offset_intersection()
+
+    def mpx_registers_offset_intersection(self):
+        exe = self.getBuildArtifact('a.out')
+        self.runCmd('file ' + exe, CURRENT_EXECUTABLE_SET)
+        self.runCmd('run', RUN_SUCCEEDED)
+        target = self.dbg.GetSelectedTarget()
+        process = target.GetProcess()
+        thread = process.GetThreadAtIndex(0)
+        currentFrame = thread.GetFrameAtIndex(0)
+
+        has_avx = False
+        has_mpx = False
+        for registerSet in currentFrame.GetRegisters():
+            if 'advanced vector extensions' in registerSet.GetName().lower():
+                has_avx = True
+            if 'memory protection extension' in registerSet.GetName().lower():
+                has_mpx = True
+        if not (has_avx and has_mpx):
+            self.skipTest('Both AVX and MPX registers must be supported.')
+
+        for reg in self.AVX_REGS:
+            self.runCmd('register write ' + reg + " '" + self.YMM_VALUE + " '")
+        for reg in self.MPX_REGULAR_REGS + self.MPX_CONFIG_REGS:
+            self.runCmd('register write ' + reg + " '" + self.BND_VALUE + " '")
+
+        self.verify_mpx()
+        self.verify_avx()
+        self.verify_mpx()
+
+    def verify_mpx(self):
+        for reg in self.MPX_REGULAR_REGS:
+            self.expect('register read ' + reg,
+                        substrs = [reg + ' = {0xffffffffffffffff 0xffffffffffffffff}'])
+        for reg in self.MPX_CONFIG_REGS:
+            self.expect('register read ' + reg,
+                        substrs = [reg + ' = {0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff}'])
+
+    def verify_avx(self):
+        for reg in self.AVX_REGS:
+            self.expect('register read ' + reg, substrs = [reg + ' = ' + self.YMM_VALUE])

lldb/packages/Python/lldbsuite/test/commands/register/register/intel_xtended_registers/mpx_offset_intersection/main.cpp

+#include <cstdint>
+
+int main() {
+  asm volatile("int3");
+  return 0;
+}

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

protected:
   virtual size_t GetGPRSize() {
     return GetRegisterInfoInterface().GetGPRSize();
   }
 
   virtual void *GetFPRBuffer() = 0;
 
   virtual size_t GetFPRSize() = 0;
 
+  virtual uint32_t GetPtraceOffset(uint32_t reg_index) {
+    return GetRegisterInfoAtIndex(reg_index)->byte_offset;
+  }
+
   // The Do*** functions are executed on the privileged thread and can perform
   // ptrace
   // operations directly.
   virtual Status DoReadRegisterValue(uint32_t offset, const char *reg_name,
                                      uint32_t size, RegisterValue &value);
 
   virtual Status DoWriteRegisterValue(uint32_t offset, const char *reg_name,
                                       const RegisterValue &value);
 };
 
 } // namespace process_linux
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextLinux_h

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

 }
 
 Status NativeRegisterContextLinux::ReadRegisterRaw(uint32_t reg_index,
                                                    RegisterValue &reg_value) {
   const RegisterInfo *const reg_info = GetRegisterInfoAtIndex(reg_index);
   if (!reg_info)
     return Status("register %" PRIu32 " not found", reg_index);
 
-  return DoReadRegisterValue(reg_info->byte_offset, reg_info->name,
+  return DoReadRegisterValue(GetPtraceOffset(reg_index), reg_info->name,
                              reg_info->byte_size, reg_value);
 }
 
 Status
 NativeRegisterContextLinux::WriteRegisterRaw(uint32_t reg_index,
                                              const RegisterValue &reg_value) {
   uint32_t reg_to_write = reg_index;
   RegisterValue value_to_write = reg_value;
const RegisterInfo *const register_to_write_info_p =
       GetRegisterInfoAtIndex(reg_to_write);
   assert(register_to_write_info_p &&
          "register to write does not have valid RegisterInfo");
   if (!register_to_write_info_p)
     return Status("NativeRegisterContextLinux::%s failed to get RegisterInfo "
                   "for write register index %" PRIu32,
                   __FUNCTION__, reg_to_write);
 
-  return DoWriteRegisterValue(reg_info->byte_offset, reg_info->name, reg_value);
+  return DoWriteRegisterValue(GetPtraceOffset(reg_index), reg_info->name,
+                              reg_value);
 }
 
 Status NativeRegisterContextLinux::ReadGPR() {
   return NativeProcessLinux::PtraceWrapper(
       PTRACE_GETREGS, m_thread.GetID(), nullptr, GetGPRBuffer(), GetGPRSize());
 }
 
 Status NativeRegisterContextLinux::WriteGPR() {

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

protected:
   void *GetFPRBuffer() override;
 
   size_t GetFPRSize() override;
 
   Status ReadFPR() override;
 
   Status WriteFPR() override;
 
+  uint32_t GetPtraceOffset(uint32_t reg_index) override;
+
 private:
   // Private member types.
   enum class XStateType { Invalid, FXSAVE, XSAVE };
   enum class RegSet { gpr, fpu, avx, mpx };
 
   // Info about register ranges.
   struct RegInfo {
     uint32_t num_registers;

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

NativeRegisterContextLinux_x86_64::GetWatchpointAddress(uint32_t wp_index) {
   return reg_value.GetAsUInt64();
 }
 
 uint32_t NativeRegisterContextLinux_x86_64::NumSupportedHardwareWatchpoints() {
   // Available debug address registers: dr0, dr1, dr2, dr3
   return 4;
 }
 
+uint32_t
+NativeRegisterContextLinux_x86_64::GetPtraceOffset(uint32_t reg_index) {
+  // If register is MPX, remove extra factor from gdb offset
+  return GetRegisterInfoAtIndex(reg_index)->byte_offset -
+         (IsMPX(reg_index) ? 128 : 0);
+}
+
 #endif // defined(__i386__) || defined(__x86_64__)

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

 
 // Computes the offset of the YMM register assembled from register halves.
 // Based on DNBArchImplX86_64.cpp from debugserver
 #define YMM_OFFSET(reg_index)                                                  \
   (LLVM_EXTENSION offsetof(UserArea, fpr) +                                    \
    LLVM_EXTENSION offsetof(FPR, xsave) +                                       \
    LLVM_EXTENSION offsetof(XSAVE, ymmh[0]) + (32 * reg_index))
 
+// Guarantees BNDR/BNDC offsets do not overlap with YMM offsets.
+#define GDB_REMOTE_OFFSET 128
+
 #define BNDR_OFFSET(reg_index)                                                 \
   (LLVM_EXTENSION offsetof(UserArea, fpr) +                                    \
    LLVM_EXTENSION offsetof(FPR, xsave) +                                       \
-   LLVM_EXTENSION offsetof(XSAVE, mpxr[reg_index]))
+   LLVM_EXTENSION offsetof(XSAVE, mpxr[reg_index]) + GDB_REMOTE_OFFSET)
 
 #define BNDC_OFFSET(reg_index)                                                 \
   (LLVM_EXTENSION offsetof(UserArea, fpr) +                                    \
    LLVM_EXTENSION offsetof(FPR, xsave) +                                       \
-   LLVM_EXTENSION offsetof(XSAVE, mpxc[reg_index]))
+   LLVM_EXTENSION offsetof(XSAVE, mpxc[reg_index]) + GDB_REMOTE_OFFSET)
 
 #ifdef DECLARE_REGISTER_INFOS_X86_64_STRUCT
 
 // Number of bytes needed to represent a FPR.
 #define FPR_SIZE(reg) sizeof(((FXSAVE *)nullptr)->reg)
 
 // Number of bytes needed to represent the i'th FP register.
 #define FP_SIZE sizeof(((MMSReg *)nullptr)->bytes)