Fix an ASAN bug I introduced in debugserver, accessing off the end of an array intentionally

[jasonmolenda]

Summary

In https://reviews.llvm.org/D136620 I needed to access fp/sp/pc/lr two different ways depending on the compile-time environment -- the headers name these registers differently, and the types are different so one of them needs to be cast. This was tiresome, so instead I indexed off of the array of general purpose registers right before them. ASAN expresses its displeasure with this shortcut.

Before my original patch, this code passed the register context in to the arm_thread_state64_get{sp,fp,lr,pc} and those macros handled this detail.

The ASAN CI bot does not build an in-tree debugserver and test with it, but when I looked into a bot test failure, I hit this first.

[JDevlieghere]

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

2026–2037

reg is a uint32_t so you could use a switch here:

switch(reg) {
  case gpr_pc: 
    ...
}

[jasonmolenda]

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

2026–2037

I thought about that, but then I'd need a break; after each element and it was one extra line. I don't have a real preference, and this was shorter.

[aprantl]

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

2026–2037

If we are already micro-optimizing, I guess the resulting code would be shorter (and 100% UB-free) if it were

uint64_t val = 0;
switch(reg) {
  case gpr_pc: 
      memcpy(&val, m_state.context.gpr.__opaque_pc);
      break;
  ...
}
value->value.uint64 = clear_pac_bits(val);

[jasonmolenda]

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

2026–2037

I don't have anything against using memcpy instead of the reinterpret_cast, but I don't see it being any clearer (IMO), and it doesn't reduce the code duplication - we still need separate lines for when the members are named __opaque_ and when not. If anyone feels it's clearer to use a switch statement and/or memcpy, I genuinely don't care either way, but I don't see it myself.

[JDevlieghere]

I think a switch case is easier to read (and spot things like missing cases) but this is code you own so go with the approach you like best. LGTM.

[jasonmolenda]

Update the patch to use a case statement for fp/sp/lr/pc instead of conditionals; both Adrian and Jonas preferred it.

lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp

     return false;
 
   const DNBRegisterInfo *regInfo = m_thread->GetRegisterInfo(set, reg);
   if (regInfo) {
     value->info = *regInfo;
     switch (set) {
     case e_regSetGPR:
       if (reg <= gpr_pc) {
-        if (reg == gpr_pc || reg == gpr_lr || reg == gpr_sp || reg == gpr_fp)
-          value->value.uint64 = clear_pac_bits(m_state.context.gpr.__x[reg]);
+#if __has_feature(ptrauth_calls) && defined(__LP64__)
+        if (reg == gpr_pc)
+          value->value.uint64 = clear_pac_bits(
+              reinterpret_cast<uint64_t>(m_state.context.gpr.__opaque_pc));
+        else if (reg == gpr_lr)
+          value->value.uint64 = clear_pac_bits(
+              reinterpret_cast<uint64_t>(m_state.context.gpr.__opaque_lr));
+        else if (reg == gpr_sp)
+          value->value.uint64 = clear_pac_bits(
+              reinterpret_cast<uint64_t>(m_state.context.gpr.__opaque_sp));
+        else if (reg == gpr_fp)
+          value->value.uint64 = clear_pac_bits(
+              reinterpret_cast<uint64_t>(m_state.context.gpr.__opaque_fp));
+#else
+        if (reg == gpr_pc)
+          value->value.uint64 = clear_pac_bits(m_state.context.gpr.__pc);
+        else if (reg == gpr_lr)
+          value->value.uint64 = clear_pac_bits(m_state.context.gpr.__lr);
+        else if (reg == gpr_sp)
+          value->value.uint64 = clear_pac_bits(m_state.context.gpr.__sp);
+        else if (reg == gpr_fp)
+          value->value.uint64 = clear_pac_bits(m_state.context.gpr.__fp);
+#endif
         else
           value->value.uint64 = m_state.context.gpr.__x[reg];
         return true;
       } else if (reg == gpr_cpsr) {
         value->value.uint32 = m_state.context.gpr.__cpsr;
         return true;
       }
       break;