[lldb] Add isConstant mode for FA locations

[felipepiovezan]

This is similar to 9fe455f, but for FA locations instead of register locations.
This is useful for unwind plans that cannot create abstract unwind rules, but instead must inspect the state of the program to determine the current CFA.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This is similar to 9fe455f, but for FA locations instead of register locations.
This is useful for unwind plans that cannot create abstract unwind rules, but instead must inspect the state of the program to determine the current CFA.

---

Full diff: https://github.com/llvm/llvm-project/pull/110726.diff

3 Files Affected:

• (modified) lldb/include/lldb/Symbol/UnwindPlan.h (+12)
• (modified) lldb/source/Symbol/UnwindPlan.cpp (+4)
• (modified) lldb/source/Target/RegisterContextUnwind.cpp (+6)

diff --git a/lldb/include/lldb/Symbol/UnwindPlan.h b/lldb/include/lldb/Symbol/UnwindPlan.h
index e1567c7357d0b5..48c9bef76857c0 100644
--- a/lldb/include/lldb/Symbol/UnwindPlan.h
+++ b/lldb/include/lldb/Symbol/UnwindPlan.h
@@ -215,6 +215,7 @@ class UnwindPlan {
         isRegisterDereferenced, // FA = [reg]
         isDWARFExpression,      // FA = eval(dwarf_expr)
         isRaSearch,             // FA = SP + offset + ???
+        isConstant,             // FA = constant
       };
 
       FAValue() : m_value() {}
@@ -259,6 +260,15 @@ class UnwindPlan {
         m_value.expr.length = len;
       }
 
+      bool IsConstant() const { return m_type == isConstant; }
+
+      void SetIsConstant(uint64_t constant) {
+        m_type = isConstant;
+        m_value.constant = constant;
+      }
+
+      uint64_t GetConstant() const { return m_value.constant; }
+
       uint32_t GetRegisterNumber() const {
         if (m_type == isRegisterDereferenced || m_type == isRegisterPlusOffset)
           return m_value.reg.reg_num;
@@ -329,6 +339,8 @@ class UnwindPlan {
         } expr;
         // For m_type == isRaSearch
         int32_t ra_search_offset;
+        // For m_type = isConstant
+        uint64_t constant;
       } m_value;
     }; // class FAValue
 
diff --git a/lldb/source/Symbol/UnwindPlan.cpp b/lldb/source/Symbol/UnwindPlan.cpp
index a06e7cfd7f5446..25e3676761436e 100644
--- a/lldb/source/Symbol/UnwindPlan.cpp
+++ b/lldb/source/Symbol/UnwindPlan.cpp
@@ -187,6 +187,8 @@ operator==(const UnwindPlan::Row::FAValue &rhs) const {
         return !memcmp(m_value.expr.opcodes, rhs.m_value.expr.opcodes,
                        m_value.expr.length);
       break;
+    case isConstant:
+      return m_value.constant == rhs.m_value.constant;
     }
   }
   return false;
@@ -214,6 +216,8 @@ void UnwindPlan::Row::FAValue::Dump(Stream &s, const UnwindPlan *unwind_plan,
   case isRaSearch:
     s.Printf("RaSearch@SP%+d", m_value.ra_search_offset);
     break;
+  case isConstant:
+    s.Printf("0x%" PRIx64, m_value.constant);
   }
 }
 
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp
index b683ea7237de04..8f4def9840712f 100644
--- a/lldb/source/Target/RegisterContextUnwind.cpp
+++ b/lldb/source/Target/RegisterContextUnwind.cpp
@@ -2088,6 +2088,12 @@ bool RegisterContextUnwind::ReadFrameAddress(
     UnwindLogMsg("No suitable CFA found");
     break;
   }
+  case UnwindPlan::Row::FAValue::isConstant: {
+    address = fa.GetConstant();
+    address = abi_sp->FixCodeAddress(address);
+    UnwindLogMsg("CFA value set by constant is 0x%" PRIx64, address);
+    return true;
+  }
   default:
     return false;
   }

[felipepiovezan]

@jasonmolenda I see a few other places in this file calling the function above (FixCodeAddress). But there are many other places that just take addresses and use them as is to do things like Process:ReadMemory... it makes me wonder whether we should audit those uses.

[felipepiovezan]

For example, line 2051:

if (ABISP abi_sp = m_thread.GetProcess()->GetABI())
        address = abi_sp->FixCodeAddress(address);

Now I wonder if this is shadowing the abi_sp variable...

[jasonmolenda]

LGTM. Yes, the question of "when do we need to run an address through a Fix method" is largely driven by the different ABIs we encounter on different platforms. If we know it is a virtual address value (no metadata in high bits/unaddressable bits that we may need to inspect), it is safe to run it through a Fix method. The question of FixCode or FixData doesn't really matter here with aligned stack addresses, but a typical difference is that FixCode may clear the low 1 or 2 bits on targets with fixed length instructions, whereas FixData can point to a byte boundary so all of the low bits are typically preserved.

This does mean that we see Fix calls in generic code that are specific to how, e.g. Darwin's ABI signs a vtable pointer which might not be the case on all target using AArch64 ptrauth signing. An example of where over-Fixing is a problem comes from Objective-C which has an "isa" pointer at the start of the object, identifying the object's metadata, and in some cases there may be a "non-pointer isa" (aka tagged pointer) which has the value of the object in the isa itself, e.g. a small NSNumber might be representable entirely within the isa word. And so treating this non-pointer isa as a virtual address will clear significant bits and make it meaningless.

Practically speaking, we've moved to a model where we Fix addresses before we Read or Write from them in the Process, so even if we miss a Fix that was needed for a signed address, by the time we actually try to read from that address it will work. This behavior is needed in the face of AArch64 MTE where we can have tags in the top nibble, or an AArch64 processor running in Top Byte Ignore mode where the inferior may store metadata in the type byte (Darwin's application processors run in TBI mode enabled).

[jasonmolenda]

We prefer people call Process::FixDataAddress() these days, a method which gets the ABI and runs the method. (this is the address on the stack, so FixDataAddress is the correct method). Yeah the code above at line 2051 should be doing the same thing, I know you were only copying it here.

[felipepiovezan]

Sounds good! I will push an updated version calling the process method!