Refactor NativeRegisterContextLinux_x86_64 code.

[valentinagiusti]

Summary

This patch refactors the way the XState type is checked and, in order to
simplify the code, it removes the usage of the 'cpuid' instruction: just checking
if the ptrace calls done throuhg ReadFPR is enough to verify both if there is
HW support and if there is kernel support. Also the XCR0 bits are enough to check if
there is both HW and kernel support for AVX and MPX.

[zturner]

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

818 ↗

(On Diff #71930)

Is the AVX case supposed to fall-through to the mpx case?

[labath]

I am going to assume you know more about the exact details of the intel cpu's than me, so I am not going comment on the technical details. The code seems cleaner, so this looks like a good thing to do. Just please try to avoid using the const_cast if it's no absolutely necessary (and it almost never is).

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

807 ↗

(On Diff #71930)

I don't think the const_cast here is a good idea. How about we just make this functions non-const ?

808 ↗

(On Diff #71930)

I remember you #including some header to get the __get_cpuid function. Can that be removed now?

[valentinagiusti]

Thanks for your review! Please find my answers inline.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

807 ↗

(On Diff #71930)

IsCPUFeatureAvailable must also be called by IsRegisterSetAvailable, which is also const...

818 ↗

(On Diff #71930)

good catch, it's not supposed to fall-through

[valentinagiusti]

Removed unnecessary header, corrected switch-case.

[labath]

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

805 ↗

(On Diff #71949)

Then I think we should make that non-const as well. I mean, what's the point of making it const if it does actually modify the state of the object. Either that, or implement it in a way that does not modify the state.

[valentinagiusti]

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

805 ↗

(On Diff #71949)

This involves changing the interface of NativeRegisterContextLinux just because NativeRegisterContextLinux_x86_64 has some specific needs that make GetRegisterCount use a non-const function. Also, I don't have all the hardware to test such a cross-platform change.

[valentinagiusti]

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

805 ↗

(On Diff #71949)

Anyway I guess I can submit a follow-up patch with this, I will do it today ;)

[valentinagiusti]

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp

805 ↗

(On Diff #71949)

So I have looked into this and actually in "source/Host/common/NativeRegisterContextRegisterInfo.cpp" the method GetRegisterCount() is implemented as const (and the pure virtual method is defined in "include/lldb/Host/common/NativeRegisterContext.h"). Unfortunately I don't see an easy way 1. to make the method non-const or 2. to avoid that GetRegisterSetCount() in NativeRegisterContextLinux_x86_64 needs to call a non const function: in order to get the register set count, it must know if the XState type is XSAVE, and in order to do that it must call ReadFPR(), or directly PtraceWrapper(), which are both non-const.
Could you please tell me what you think is best?
Imho, it would be better to first merge this patch and then clean up the NativeRegisterContext code, but I am open to suggestions.

[labath]

So, the thing is that you already are changing the interface. The difference is that you are using the const cast to hide that fact, which is why I dont approve of it.

Also, since this is not an existing problem but rather something you are introducing in this change, I dont thinl it's appropriate to do it as a follow-up, but rather as a prep work for this change.

One possibility I would consider is to do any work you need to do before this function even executes (e.g. in the constructor - the result of IsCpuFeatureAvailable cannot change during the lifetime of the process anyway, right?). I am OOO today so I cannot really tell how feasible that ideaIis. I can look at It tomorrow.

[valentinagiusti]

Thechnically it's not correct that I am introducing this issue, because the old code already used a cast. It was done in the old and now not existing method "GetFPRType()", long before I introduced the MPX changes, and then I later moved it into HasXSave()/HasXSave() and now with this current refactoring patch I am moving it into IsCPUFeatureAvailable().

[labath]

I'm sorry, I did not notice that. Go ahead with this patch in that case. It looks great apart from this eexisting problem. If you're going to do further cleanups here,, I would recommend looking at the cast problem as well.

[valentinagiusti]

ok, I will keep it in mind for some further refactoring, thanks!

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

private:
   IOVEC m_iovec;
   YMM m_ymm_set;
   MPX m_mpx_set;
   RegInfo m_reg_info;
   uint64_t m_gpr_x86_64[k_num_gpr_registers_x86_64];
   uint32_t m_fctrl_offset_in_userarea;
 
   // Private member methods.
-  bool HasFXSAVE() const;
-
-  bool HasXSAVE() const;
-
   bool IsCPUFeatureAvailable(RegSet feature_code) const;
 
   bool IsRegisterSetAvailable(uint32_t set_index) const;
 
   bool IsGPR(uint32_t reg_index) const;
 
-  XStateType GetXStateType() const;
-
   bool IsFPR(uint32_t reg_index) const;
 
   bool CopyXSTATEtoYMM(uint32_t reg_index, lldb::ByteOrder byte_order);
 
   bool CopyYMMtoXSTATE(uint32_t reg, lldb::ByteOrder byte_order);
 
   bool IsAVX(uint32_t reg_index) const;
 

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

 #include "lldb/Core/Error.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/RegisterValue.h"
 #include "lldb/Host/HostInfo.h"
 
 #include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
 #include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
 
-#include <cpuid.h>
-
 using namespace lldb_private;
 using namespace lldb_private::process_linux;
 
 // ----------------------------------------------------------------------------
 // Private namespace.
 // ----------------------------------------------------------------------------
 
 namespace {
if (dst == nullptr) {
     error.SetErrorStringWithFormat("DataBufferHeap instance of size %" PRIu64
                                    " returned a null pointer",
                                    REG_CONTEXT_SIZE);
     return error;
   }
 
   ::memcpy(dst, &m_gpr_x86_64, GetRegisterInfoInterface().GetGPRSize());
   dst += GetRegisterInfoInterface().GetGPRSize();
-  if (GetXStateType() == XStateType::FXSAVE)
+  if (m_xstate_type == XStateType::FXSAVE)
     ::memcpy(dst, &m_fpr.xstate.fxsave, sizeof(m_fpr.xstate.fxsave));
-  else if (GetXStateType() == XStateType::XSAVE) {
+  else if (m_xstate_type == XStateType::XSAVE) {
     lldb::ByteOrder byte_order = GetByteOrder();
 
     if (IsCPUFeatureAvailable(RegSet::avx)) {
       // Assemble the YMM register content from the register halves.
       for (uint32_t reg = m_reg_info.first_ymm; reg <= m_reg_info.last_ymm;
            ++reg) {
         if (!CopyXSTATEtoYMM(reg, byte_order)) {
           error.SetErrorStringWithFormat(
Error NativeRegisterContextLinux_x86_64::WriteAllRegisterValues(
   }
   ::memcpy(&m_gpr_x86_64, src, GetRegisterInfoInterface().GetGPRSize());
 
   error = WriteGPR();
   if (error.Fail())
     return error;
 
   src += GetRegisterInfoInterface().GetGPRSize();
-  if (GetXStateType() == XStateType::FXSAVE)
+  if (m_xstate_type == XStateType::FXSAVE)
     ::memcpy(&m_fpr.xstate.fxsave, src, sizeof(m_fpr.xstate.fxsave));
-  else if (GetXStateType() == XStateType::XSAVE)
+  else if (m_xstate_type == XStateType::XSAVE)
     ::memcpy(&m_fpr.xstate.xsave, src, sizeof(m_fpr.xstate.xsave));
 
   error = WriteFPR();
   if (error.Fail())
     return error;
 
-  if (GetXStateType() == XStateType::XSAVE) {
+  if (m_xstate_type == XStateType::XSAVE) {
     lldb::ByteOrder byte_order = GetByteOrder();
 
     if (IsCPUFeatureAvailable(RegSet::avx)) {
       // Parse the YMM register content from the register halves.
       for (uint32_t reg = m_reg_info.first_ymm; reg <= m_reg_info.last_ymm;
            ++reg) {
         if (!CopyYMMtoXSTATE(reg, byte_order)) {
           error.SetErrorStringWithFormat(
if (IsCPUFeatureAvailable(RegSet::mpx)) {
         }
       }
     }
   }
 
   return error;
 }
 
-bool NativeRegisterContextLinux_x86_64::HasFXSAVE() const {
-  unsigned int rax, rbx, rcx, rdx;
-
-  // Check if FXSAVE is enabled.
-  if (!__get_cpuid(1, &rax, &rbx, &rcx, &rdx))
-    return false;
-  if ((rdx & bit_FXSAVE) == bit_FXSAVE) {
-    m_xstate_type = XStateType::FXSAVE;
-    if (const_cast<NativeRegisterContextLinux_x86_64 *>(this)->ReadFPR().Fail())
-      return false;
-    return true;
-  }
-  return false;
-}
-
-bool NativeRegisterContextLinux_x86_64::HasXSAVE() const {
-  unsigned int rax, rbx, rcx, rdx;
-
-  // Check if XSAVE is enabled.
-  if (!__get_cpuid(1, &rax, &rbx, &rcx, &rdx))
-    return false;
-  if ((rcx & bit_OSXSAVE) == bit_OSXSAVE) {
-    m_xstate_type = XStateType::XSAVE;
-    if (const_cast<NativeRegisterContextLinux_x86_64 *>(this)->ReadFPR().Fail())
-      return false;
-    return true;
-  }
-  return false;
-}
-
 bool NativeRegisterContextLinux_x86_64::IsCPUFeatureAvailable(
     RegSet feature_code) const {
-  unsigned int rax, rbx, rcx, rdx;
-
-  // Check if XSAVE is enabled.
-  if (!HasXSAVE())
+  if (m_xstate_type == XStateType::Invalid) {
+    if (const_cast<NativeRegisterContextLinux_x86_64 *>(this)->ReadFPR().Fail())
       return false;
-
-  __get_cpuid(1, &rax, &rbx, &rcx, &rdx);
+  }
   switch (feature_code) {
-  case RegSet::avx: // Check if CPU has AVX and if there is kernel support, by reading in the XCR0 area of XSAVE.
-    if (((rcx & bit_AVX) != 0) && ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_AVX) == mask_XSTATE_AVX))
+  case RegSet::gpr:
+  case RegSet::fpu:
+    return true;
+  case RegSet::avx: // Check if CPU has AVX and if there is kernel support, by
+                    // reading in the XCR0 area of XSAVE.
+    if ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_AVX) == mask_XSTATE_AVX)
       return true;
-  case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by reading in the XCR0 area of XSAVE.
-    if (__get_cpuid_max(0, NULL) > 7) {
-      __cpuid_count(7, 0, rax, rbx, rcx, rdx);
-      if (((rbx & bit_MPX) != 0) && ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_MPX) == mask_XSTATE_MPX))
-        return true;
-    }
-  default:
-    return false;
-  }
-}
+     break;
+  case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by
+                    // reading in the XCR0 area of XSAVE.
+    if ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_MPX) == mask_XSTATE_MPX)
+    break;
 
 bool NativeRegisterContextLinux_x86_64::IsRegisterSetAvailable(
     uint32_t set_index) const {
   uint32_t num_sets = k_num_register_sets - k_num_extended_register_sets;
 
   switch (static_cast<RegSet>(set_index)) {
   case RegSet::gpr:
   case RegSet::fpu:
     return (set_index < num_sets);
   case RegSet::avx:
     return IsCPUFeatureAvailable(RegSet::avx);
   case RegSet::mpx:
     return IsCPUFeatureAvailable(RegSet::mpx);
-  default:
-    return false;
   }
+  return false;
 }
 
 bool NativeRegisterContextLinux_x86_64::IsGPR(uint32_t reg_index) const {
   // GPRs come first.
   return reg_index <= m_reg_info.last_gpr;
 }
 
-NativeRegisterContextLinux_x86_64::XStateType
-NativeRegisterContextLinux_x86_64::GetXStateType() const {
-  if (m_xstate_type == XStateType::Invalid) {
-    if (HasXSAVE())
-      m_xstate_type = XStateType::XSAVE;
-    else if (HasFXSAVE())
-      m_xstate_type = XStateType::FXSAVE;
-  }
-  return m_xstate_type;
-}
-
 bool NativeRegisterContextLinux_x86_64::IsFPR(uint32_t reg_index) const {
   return (m_reg_info.first_fpr <= reg_index &&
           reg_index <= m_reg_info.last_fpr);
 }
 
 Error NativeRegisterContextLinux_x86_64::WriteFPR() {
-  const XStateType fpr_type = GetXStateType();
-  const lldb_private::ArchSpec &target_arch =
-      GetRegisterInfoInterface().GetTargetArchitecture();
-  switch (fpr_type) {
+  switch (m_xstate_type) {
   case XStateType::FXSAVE:
-    // For 32-bit inferiors on x86_32/x86_64 architectures,
-    // FXSAVE area can be written using PTRACE_SETREGSET ptrace api
-    // For 64-bit inferiors on x86_64 architectures,
-    // FXSAVE area can be written using PTRACE_SETFPREGS ptrace api
-    switch (target_arch.GetMachine()) {
-    case llvm::Triple::x86:
       return WriteRegisterSet(&m_iovec, sizeof(m_fpr.xstate.xsave),
                               NT_PRXFPREG);
-    case llvm::Triple::x86_64:
-      return NativeRegisterContextLinux::WriteFPR();
-    default:
-      assert(false && "Unhandled target architecture.");
-      break;
-    }
   case XStateType::XSAVE:
     return WriteRegisterSet(&m_iovec, sizeof(m_fpr.xstate.xsave),
                             NT_X86_XSTATE);
   default:
-    return Error("Unrecognized FPR type");
+    return Error("Unrecognized FPR type.");
   }
 }
 
 bool NativeRegisterContextLinux_x86_64::IsAVX(uint32_t reg_index) const {
   if (!IsCPUFeatureAvailable(RegSet::avx))
     return false;
   return (m_reg_info.first_ymm <= reg_index &&
           reg_index <= m_reg_info.last_ymm);
if (byte_order == lldb::eByteOrderBig) {
     ::memcpy(m_fpr.xstate.xsave.ymmh[reg - m_reg_info.first_ymm].bytes,
              m_ymm_set.ymm[reg - m_reg_info.first_ymm].bytes, sizeof(YMMHReg));
     return true;
   }
   return false; // unsupported or invalid byte order
 }
 
 void *NativeRegisterContextLinux_x86_64::GetFPRBuffer() {
-  const XStateType xstate_type = GetXStateType();
-  switch (xstate_type) {
+  switch (m_xstate_type) {
   case XStateType::FXSAVE:
     return &m_fpr.xstate.fxsave;
   case XStateType::XSAVE:
     return &m_iovec;
   default:
     return nullptr;
   }
 }
 
 size_t NativeRegisterContextLinux_x86_64::GetFPRSize() {
-  const XStateType xstate_type = GetXStateType();
-  switch (xstate_type) {
+  switch (m_xstate_type) {
   case XStateType::FXSAVE:
     return sizeof(m_fpr.xstate.fxsave);
   case XStateType::XSAVE:
     return sizeof(m_iovec);
   default:
     return 0;
   }
 }
 
 Error NativeRegisterContextLinux_x86_64::ReadFPR() {
-  const XStateType xstate_type = GetXStateType();
-  const lldb_private::ArchSpec &target_arch =
-      GetRegisterInfoInterface().GetTargetArchitecture();
-  switch (xstate_type) {
-  case XStateType::FXSAVE:
-    // For 32-bit inferiors on x86_32/x86_64 architectures,
-    // FXSAVE area can be read using PTRACE_GETREGSET ptrace api
-    // For 64-bit inferiors on x86_64 architectures,
-    // FXSAVE area can be read using PTRACE_GETFPREGS ptrace api
-    switch (target_arch.GetMachine()) {
-    case llvm::Triple::x86:
-      return ReadRegisterSet(&m_iovec, sizeof(m_fpr.xstate.xsave), NT_PRXFPREG);
-    case llvm::Triple::x86_64:
-      return NativeRegisterContextLinux::ReadFPR();
-    default:
-      assert(false && "Unhandled target architecture.");
-      break;
+  Error error;
+
+  // Probe XSAVE and if it is not supported fall back to FXSAVE.
+  if (m_xstate_type != XStateType::FXSAVE) {
+    error =
+        ReadRegisterSet(&m_iovec, sizeof(m_fpr.xstate.xsave), NT_X86_XSTATE);
+    if (!error.Fail()) {
+      m_xstate_type = XStateType::XSAVE;
+      return error;
     }
-  case XStateType::XSAVE:
-    return ReadRegisterSet(&m_iovec, sizeof(m_fpr.xstate.xsave), NT_X86_XSTATE);
-  default:
-    return Error("Unrecognized FPR type");
   }
+  error = ReadRegisterSet(&m_iovec, sizeof(m_fpr.xstate.xsave), NT_PRXFPREG);
+  if (!error.Fail()) {
+    m_xstate_type = XStateType::FXSAVE;
+    return error;
+  }
+  return Error("Unrecognized FPR type.");
 }
 
 bool NativeRegisterContextLinux_x86_64::IsMPX(uint32_t reg_index) const {
   if (!IsCPUFeatureAvailable(RegSet::mpx))
     return false;
   return (m_reg_info.first_mpxr <= reg_index &&
           reg_index <= m_reg_info.last_mpxc);
 }