[lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

[mgorny]

Summary

Multiple fixes related to bugs discovered while debugging a crash
when reading all registers on i386.

The underlying problem was that GetSetForNativeRegNum() did not account
for MPX registers on i386, and since it only compared against upper
bounds of each known register set, the MPX registers were classified
into the wrong set and therefore considered supported. However, they
were not expected in RegNumX86ToX86_64() and caused the assertion
to fail.

This includes:

• adding (unused) i386 → x86_64 translations for MPX registers
• fixing GetSetForNativeRegNum() to check both lower and upper bound for register sets, to avoid wrongly classifying unhandled register sets
• adding missing range check for MPX registers on i386
• renaming k_last_mpxr to k_last_mpxr_i386 for consistency
• replacing return-assertions with llvm_unreachable() and adding more checks for unexpected parameters

[labath]

It sounds like the assertion could be useful (to detect the missing translations). Under what circumstances can an unknown register make its way to this function? How did you run into this code in the first place?

[mgorny]

In D88682#2307911, @labath wrote:

It sounds like the assertion could be useful (to detect the missing translations). Under what circumstances can an unknown register make its way to this function? How did you run into this code in the first place?

Very good question. I've ran into it because it crashed under me. But now I see that x86 variant of NativeRegisterContextNetBSD_x86_64::GetSetForNativeRegNum() doesn't handle mpx*, so they fall into wrong regsets. I suppose I should change the conditions to check both lower and upper bound of each group. Thanks!

[mgorny]

Ok, replaced with a few more assorted fixes ;-).

[labath]

Yes, looks good. It might not be unreasonable to add asserts to these functions too, as I don't think we should ever have a register that doesn't belong to any register set.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

404–426

Could you also remove all of these else's while you're in there, as per http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.

[mgorny]

Fixed coding style, improved description, and added more assertions for unexpected registers.

[krytarowski]

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

394

Use llvm_unreachable ? Same in other places where we add similar assert().

410

k_first_mpxr_i386 -> k_first_mpxc_i386 ?

424

k_first_mpxr_x86_64 -> k_first_mpxc_x86_64?

[mgorny]

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

394

@labath, what is the recommended practice for LLDB? Should I just replace these asserts with llvm_unreachable(message)?

[mgorny]

Fixed wrong constant as pointed out by Kamil.

[labath]

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

394

Yes, llvm_unreachable is better.

[mgorny]

Use llvm_unreachable().

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

 const RegisterSet *
 NativeRegisterContextNetBSD_x86_64::GetRegisterSet(uint32_t set_index) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
     return &g_reg_sets_i386[set_index];
   case llvm::Triple::x86_64:
     return &g_reg_sets_x86_64[set_index];
   default:
-    assert(false && "Unhandled target architecture.");
-    return nullptr;
+    llvm_unreachable("Unhandled target architecture.");
   }
-
-  return nullptr;
 }
 
 static constexpr int RegNumX86ToX86_64(int regnum) {
   switch (regnum) {
   case lldb_eax_i386:
     return lldb_rax_x86_64;
   case lldb_ebx_i386:
     return lldb_rbx_x86_64;
static constexpr int RegNumX86ToX86_64(int regnum) {
   case lldb_ymm1_i386:
   case lldb_ymm2_i386:
   case lldb_ymm3_i386:
   case lldb_ymm4_i386:
   case lldb_ymm5_i386:
   case lldb_ymm6_i386:
   case lldb_ymm7_i386:
     return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
+  case lldb_bnd0_i386:
+  case lldb_bnd1_i386:
+  case lldb_bnd2_i386:
+  case lldb_bnd3_i386:
+    return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
+  case lldb_bndcfgu_i386:
+    return lldb_bndcfgu_x86_64;
+  case lldb_bndstatus_i386:
+    return lldb_bndstatus_x86_64;
   case lldb_dr0_i386:
   case lldb_dr1_i386:
   case lldb_dr2_i386:
   case lldb_dr3_i386:
   case lldb_dr4_i386:
   case lldb_dr5_i386:
   case lldb_dr6_i386:
   case lldb_dr7_i386:
     return lldb_dr0_x86_64 + regnum - lldb_dr0_i386;
   default:
-    assert(false && "Unhandled i386 register.");
-    return 0;
+    llvm_unreachable("Unhandled i386 register.");
   }
 }
 
 int NativeRegisterContextNetBSD_x86_64::GetSetForNativeRegNum(
     int reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
-    if (reg_num <= k_last_gpr_i386)
+    if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
       return GPRegSet;
-    else if (reg_num <= k_last_fpr_i386)
+    if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
       return FPRegSet;
-    else if (reg_num <= k_last_avx_i386)
+    if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
       return XStateRegSet; // AVX
-    else if (reg_num <= lldb_dr7_i386)
+    if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
+      return -1; // MPXR
+    if (reg_num >= k_first_mpxc_i386 && reg_num <= k_last_mpxc_i386)
+      return -1; // MPXC
+    if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
       return DBRegSet; // DBR
-    else
-      return -1;
+    break;
   case llvm::Triple::x86_64:
-    if (reg_num <= k_last_gpr_x86_64)
+    if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
       return GPRegSet;
-    else if (reg_num <= k_last_fpr_x86_64)
+    if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
       return FPRegSet;
-    else if (reg_num <= k_last_avx_x86_64)
+    if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
       return XStateRegSet; // AVX
-    else if (reg_num <= k_last_mpxr_x86_64)
+    if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
       return -1; // MPXR
-    else if (reg_num <= k_last_mpxc_x86_64)
+    if (reg_num >= k_first_mpxc_x86_64 && reg_num <= k_last_mpxc_x86_64)
       return -1; // MPXC
-    else if (reg_num <= lldb_dr7_x86_64)
+    if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
       return DBRegSet; // DBR
-    else
-      return -1;
+    break;
   default:
-    assert(false && "Unhandled target architecture.");
-    return -1;
+    llvm_unreachable("Unhandled target architecture.");
   }
+
+  llvm_unreachable("Register does not belong to any register set");
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadRegisterSet(uint32_t set) {
   switch (set) {
   case GPRegSet:
     return DoRegisterSet(PT_GETREGS, &m_gpr);
   case FPRegSet:
 #if defined(__x86_64__)
NativeRegisterContextNetBSD_x86_64::ReadRegister(const RegisterInfo *reg_info,
 
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86_64:
     break;
   case llvm::Triple::x86:
     reg = RegNumX86ToX86_64(reg);
     break;
   default:
-    assert(false && "Unhandled target architecture.");
-    error.SetErrorString("Unhandled target architecture.");
-    return error;
+    llvm_unreachable("Unhandled target architecture.");
   }
 
   error = ReadRegisterSet(set);
   if (error.Fail())
     return error;
 
   switch (reg) {
 #if defined(__x86_64__)
#endif
   case lldb_dr2_x86_64:
   case lldb_dr3_x86_64:
   case lldb_dr4_x86_64:
   case lldb_dr5_x86_64:
   case lldb_dr6_x86_64:
   case lldb_dr7_x86_64:
     reg_value = (uint64_t)m_dbr.dr[reg - lldb_dr0_x86_64];
     break;
+  default:
+    llvm_unreachable("Reading unknown/unsupported register");
   }
 
   return error;
 }
 
 Status NativeRegisterContextNetBSD_x86_64::WriteRegister(
     const RegisterInfo *reg_info, const RegisterValue &reg_value) {
 
Status NativeRegisterContextNetBSD_x86_64::WriteRegister(
 
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86_64:
     break;
   case llvm::Triple::x86:
     reg = RegNumX86ToX86_64(reg);
     break;
   default:
-    assert(false && "Unhandled target architecture.");
-    error.SetErrorString("Unhandled target architecture.");
-    return error;
+    llvm_unreachable("Unhandled target architecture.");
   }
 
   error = ReadRegisterSet(set);
   if (error.Fail())
     return error;
 
   switch (reg) {
 #if defined(__x86_64__)
if (!(m_xstate.xs_rfbm & XCR0_SSE) ||
       YMMReg ymm;
       ::memcpy(ymm.bytes, reg_value.GetBytes(), reg_value.GetByteSize());
       YMMToXState(ymm,
           m_xstate.xs_fxsave.fx_xmm[reg_index].xmm_bytes,
           m_xstate.xs_ymm_hi128.xs_ymm[reg_index].ymm_bytes);
     }
 #else
     error.SetErrorString("XState not supported by the kernel");
+    return error;
 #endif
     break;
   case lldb_dr0_x86_64:
   case lldb_dr1_x86_64:
   case lldb_dr2_x86_64:
   case lldb_dr3_x86_64:
   case lldb_dr4_x86_64:
   case lldb_dr5_x86_64:
   case lldb_dr6_x86_64:
   case lldb_dr7_x86_64:
     m_dbr.dr[reg - lldb_dr0_x86_64] = reg_value.GetAsUInt64();
     break;
+  default:
+    llvm_unreachable("Reading unknown/unsupported register");
   }
 
   return WriteRegisterSet(set);
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
   Status error;

 int NativeRegisterContextNetBSD_x86_64::GetDR(int num) const {
   assert(num >= 0 && num <= 7);
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
     return lldb_dr0_i386 + num;
   case llvm::Triple::x86_64:
     return lldb_dr0_x86_64 + num;
   default:
-    return -1;
+    llvm_unreachable("Unhandled target architecture.");
   }
 }
 
 Status NativeRegisterContextNetBSD_x86_64::IsWatchpointHit(uint32_t wp_index,
                                                            bool &is_hit) {
   if (wp_index >= NumSupportedHardwareWatchpoints())
     return Status("Watchpoint index out of range");
 

lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h

enum {
   lldb_ymm7_i386,
   k_last_avx_i386 = lldb_ymm7_i386,
 
   k_first_mpxr_i386,
   lldb_bnd0_i386 = k_first_mpxr_i386,
   lldb_bnd1_i386,
   lldb_bnd2_i386,
   lldb_bnd3_i386,
-  k_last_mpxr = lldb_bnd3_i386,
+  k_last_mpxr_i386 = lldb_bnd3_i386,
 
   k_first_mpxc_i386,
   lldb_bndcfgu_i386 = k_first_mpxc_i386,
   lldb_bndstatus_i386,
   k_last_mpxc_i386 = lldb_bndstatus_i386,
 
   k_first_dbr_i386,
   lldb_dr0_i386 = k_first_dbr_i386,