[lldb] Track CFA pointer metadata in StackID (llvm#157498)

In this commit:

9c8e716 [lldb] Make StackID call Fix{Code,Data} pointers (llvm#152796)

We made StackID keep track of the CFA without any pointer metadata in
it. This is necessary when comparing two StackIDs to determine which one
is "younger".

However, the CFA inside StackIDs is also used in other contexts through
the method StackID::GetCallFrameAddress. One notable case is
DWARFExpression: the computation of `DW_OP_call_frame_address` is done
using StackID. This feeds into many other places, e.g. expression
evaluation may require the address of a variable that is computed from
the CFA; to access the variable without faulting, we may need to
preserve the pointer metadata. As such, StackID must be able to provide
both versions of the CFA.

In the spirit of allowing consumers of pointers to decide what to do
with pointer metadata, this patch changes StackID to store both versions
of the cfa pointer. Two getter methods are provided, and all call sites
except DWARFExpression preserve their existing behavior (stripped
pointer). Other alternatives were considered:

* Just store the raw pointer. This would require changing the
comparisong operator `<` to also receive a Process, as the comparison
requires stripped pointers. It wasn't clear if all call-sites had a
non-null process, whereas we know we have a process when creating a
StackID.

* Store a weak pointer to the process inside the class, and then strip
metadata as needed. This would require a `weak_ptr::lock` in many
operations of LLDB, and it felt wasteful. It also prevents stripping
of the pointer if the process has gone away.

This patch also changes RegisterContextUnwind::ReadFrameAddress, which
is the method computing the CFA fed into StackID, to also preserve the
signature pointers.

(cherry picked from commit 5d088ba)
(cherry picked from commit c507ec1)

Author: felipepiovezan

lldb/include/lldb/Target/StackID.h

@@ -26,7 +26,11 @@ class StackID {
 
   lldb::addr_t GetPC() const { return m_pc; }
 
-  lldb::addr_t GetCallFrameAddress() const { return m_cfa; }
+  lldb::addr_t GetCallFrameAddressWithMetadata() const {
+    return m_cfa_with_metadata;
+  }
+
+  lldb::addr_t GetCallFrameAddressWithoutMetadata() const { return m_cfa; }
 
   SymbolContextScope *GetSymbolContextScope() const { return m_symbol_scope; }
 
@@ -71,6 +75,9 @@ class StackID {
   /// below)
   lldb::addr_t m_cfa = LLDB_INVALID_ADDRESS;
 
+  /// The cfa with metadata (i.e. prior to Process::FixAddress).
+  lldb::addr_t m_cfa_with_metadata = LLDB_INVALID_ADDRESS;
+
   /// If nullptr, there is no block or symbol for this frame. If not nullptr,
   /// this will either be the scope for the lexical block for the frame, or the
   /// scope for the symbol. Symbol context scopes are always be unique pointers

lldb/source/API/SBFrame.cpp

@@ -275,7 +275,7 @@ lldb::addr_t SBFrame::GetCFA() const {
   }
 
   if (StackFrame *frame = exe_ctx->GetFramePtr())
-    return frame->GetStackID().GetCallFrameAddress();
+    return frame->GetStackID().GetCallFrameAddressWithoutMetadata();
   return LLDB_INVALID_ADDRESS;
 }
 

lldb/source/Expression/DWARFExpression.cpp

@@ -2278,7 +2278,7 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
         // Note that we don't have to parse FDEs because this DWARF expression
         // is commonly evaluated with a valid stack frame.
         StackID id = frame->GetStackID();
-        addr_t cfa = id.GetCallFrameAddress();
+        addr_t cfa = id.GetCallFrameAddressWithMetadata();
         if (cfa != LLDB_INVALID_ADDRESS) {
           stack.push_back(Scalar(cfa));
           stack.back().SetValueType(Value::ValueType::LoadAddress);

lldb/source/Target/RegisterContextUnwind.cpp

@@ -2009,8 +2009,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
             reg_info, reg_to_deref_contents, reg_info->byte_size, reg_value);
         if (error.Success()) {
           address = reg_value.GetAsUInt64();
-          if (abi_sp)
-            address = abi_sp->FixCodeAddress(address);
           UnwindLogMsg(
               "CFA value via dereferencing reg %s (%d): reg has val 0x%" PRIx64
               ", CFA value is 0x%" PRIx64,
@@ -2033,8 +2031,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
     RegisterNumber cfa_reg(m_thread, row_register_kind,
                            fa.GetRegisterNumber());
     if (ReadGPRValue(cfa_reg, cfa_reg_contents)) {
-      if (abi_sp)
-        cfa_reg_contents = abi_sp->FixDataAddress(cfa_reg_contents);
       if (cfa_reg_contents == LLDB_INVALID_ADDRESS || cfa_reg_contents == 0 ||
           cfa_reg_contents == 1) {
         UnwindLogMsg(
@@ -2069,9 +2065,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
         dwarfexpr.Evaluate(&exe_ctx, this, 0, nullptr, nullptr);
     if (result) {
       address = result->GetScalar().ULongLong();
-      if (ABISP abi_sp = m_thread.GetProcess()->GetABI())
-        address = abi_sp->FixCodeAddress(address);
-
       UnwindLogMsg("CFA value set by DWARF expression is 0x%" PRIx64,
                    address);
       return true;
@@ -2111,7 +2104,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
   }
   case UnwindPlan::Row::FAValue::isConstant: {
     address = fa.GetConstant();
-    address = m_thread.GetProcess()->FixDataAddress(address);
     UnwindLogMsg("CFA value set by constant is 0x%" PRIx64, address);
     return true;
   }

lldb/source/Target/StackFrameList.cpp

@@ -461,7 +461,7 @@ bool StackFrameList::FetchFramesUpTo(uint32_t end_idx,
         }
       } else {
         unwind_frame_sp = m_frames.front();
-        cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
+        cfa = unwind_frame_sp->m_id.GetCallFrameAddressWithoutMetadata();
       }
     } else {
       // Check for interruption when building the frames.

lldb/source/Target/StackID.cpp

@@ -35,7 +35,8 @@ bool StackID::IsCFAOnStack(Process &process) const {
 
 StackID::StackID(lldb::addr_t pc, lldb::addr_t cfa,
                  SymbolContextScope *symbol_scope, Process *process)
-    : m_pc(pc), m_cfa(cfa), m_symbol_scope(symbol_scope) {
+    : m_pc(pc), m_cfa(cfa), m_cfa_with_metadata(cfa),
+      m_symbol_scope(symbol_scope) {
   if (process) {
     m_pc = process->FixCodeAddress(m_pc);
     m_cfa = process->FixDataAddress(m_cfa);
@@ -47,6 +48,7 @@ void StackID::SetPC(lldb::addr_t pc, Process *process) {
 }
 
 void StackID::SetCFA(lldb::addr_t cfa, Process *process) {
+  m_cfa_with_metadata = cfa;
   m_cfa = process ? process->FixDataAddress(cfa) : cfa;
 }
 
@@ -67,7 +69,8 @@ void StackID::Dump(Stream *s) {
 }
 
 bool lldb_private::operator==(const StackID &lhs, const StackID &rhs) {
-  if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
+  if (lhs.GetCallFrameAddressWithoutMetadata() !=
+      rhs.GetCallFrameAddressWithoutMetadata())
     return false;
 
   SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
@@ -134,8 +137,8 @@ CompareHeapCFAs(const StackID &lhs, const StackID &rhs, Process &process) {
   if (!lhs_cfa_on_stack && rhs_cfa_on_stack)
     return HeapCFAComparisonResult::Older;
 
-  const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddress();
-  const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddress();
+  const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddressWithoutMetadata();
+  const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddressWithoutMetadata();
   // If the cfas are the same, fallback to the usual scope comparison.
   if (lhs_cfa == rhs_cfa)
     return HeapCFAComparisonResult::NoOpinion;
@@ -170,9 +173,9 @@ bool StackID::IsYounger(const StackID &lhs, const StackID &rhs,
     break;
   }
   // END SWIFT
-
-  const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddress();
-  const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddress();
+  //
+  const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddressWithoutMetadata();
+  const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddressWithoutMetadata();
 
   // FIXME: We are assuming that the stacks grow downward in memory.  That's not
   // necessary, but true on

lldb/test/API/macosx/arm-pointer-metadata-cfa-dwarf-expr/Makefile

@@ -0,0 +1,11 @@
+ASM_SOURCES := main.s
+
+# This is to appease Makefile.rules, there is no main.c
+C_SOURCES := main.c
+
+ASM_OBJS := $(ASM_SOURCES:.s=.o)
+
+%.o: %.s
+	$(CC) -c -x assembler $< -o $@
+
+include Makefile.rules

lldb/test/API/macosx/arm-pointer-metadata-cfa-dwarf-expr/TestArmPointerMetadataCFADwarfExpr.py

@@ -0,0 +1,35 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+@skipUnlessDarwin
+@skipIf(archs=no_match(["arm64"]))
+class TestArmPointerMetadataStripping(TestBase):
+    def test(self):
+        self.build()
+        target, process, thread, bkpt = lldbutil.run_to_name_breakpoint(self, "foo")
+
+        # Step over the first two instructions of foo in order to
+        # toggle the bit of fp and save it on the stack:
+        # orr   x29, x29, #0x1000000000000000
+        # stp	x29, x30, [sp, #-16]!
+        # This is effectively adding metadata to the CFA of the caller frame (main).
+        thread.StepInstruction(False)
+        thread.StepInstruction(False)
+
+        # The location of `argv` has been artificially made equal to the CFA of the frame.
+        # As such, it should have the metadata artificially set previously.
+        argv_addr = thread.frames[1].GetValueForVariablePath("&argv")
+        self.assertTrue(argv_addr.IsValid())
+        argv_addr_uint = argv_addr.GetValueAsUnsigned()
+        self.assertNotEqual((argv_addr_uint & (1 << 60)), 0)
+
+        # GetCFA strips metadata.
+        cfa = thread.frames[1].GetCFA()
+        self.assertEqual((cfa & (1 << 60)), 0)
+
+        # If the test worked correctly, the cfa and the location should be identical,
+        # modulo the metadata.
+        self.assertEqual(cfa | (1 << 60), argv_addr_uint)