Merge pull request swiftlang#10127 from felipepiovezan/felipe/fix_heap_cfa

[lldb] Fix HeapCFA comparison to be symmetric

Author: felipepiovezan

lldb/source/Target/StackID.cpp

@@ -76,11 +76,40 @@ bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
 }
 
 // BEGIN SWIFT
+/// Given two async contexts, source and maybe_parent, chase continuation
+/// pointers to check if maybe_parent can be reached from source. The search
+/// stops when it hits the end of the chain (parent_ctx == 0) or a safety limit
+/// in case of an invalid continuation chain.
+static llvm::Expected<bool> IsReachableParent(lldb::addr_t source,
+                                              lldb::addr_t maybe_parent,
+                                              Process &process) {
+  auto max_num_frames = 512;
+  for (lldb::addr_t parent_ctx = source; parent_ctx && max_num_frames;
+       max_num_frames--) {
+    Status error;
+    lldb::addr_t old_parent_ctx = parent_ctx;
+    // The continuation's context is the first field of an async context.
+    parent_ctx = process.ReadPointerFromMemory(old_parent_ctx, error);
+    if (error.Fail())
+      return llvm::createStringError(llvm::formatv(
+          "Failed to read parent async context of: {0:x}. Error: {1}",
+          old_parent_ctx, error.AsCString()));
+    if (parent_ctx == maybe_parent)
+      return true;
+  }
+  if (max_num_frames == 0)
+    return llvm::createStringError(
+        llvm::formatv("Failed to read continuation chain from {0:x} to "
+                      "possible parent {1:x}. Reached limit of frames.",
+                      source, maybe_parent));
+  return false;
+}
+
 enum class HeapCFAComparisonResult { Younger, Older, NoOpinion };
 /// If at least one of the stack IDs (lhs, rhs) is a heap CFA, perform the
 /// swift-specific async frame comparison. Otherwise, returns NoOpinion.
 static HeapCFAComparisonResult
-IsYoungerHeapCFAs(const StackID &lhs, const StackID &rhs, Process &process) {
+CompareHeapCFAs(const StackID &lhs, const StackID &rhs, Process &process) {
   const bool lhs_cfa_on_stack = lhs.IsCFAOnStack(process);
   const bool rhs_cfa_on_stack = rhs.IsCFAOnStack(process);
   if (lhs_cfa_on_stack && rhs_cfa_on_stack)
@@ -103,34 +132,26 @@ IsYoungerHeapCFAs(const StackID &lhs, const StackID &rhs, Process &process) {
 
   // Both CFAs are on the heap and they are distinct.
   // LHS is younger if and only if its continuation async context is (directly
-  // or indirectly) RHS. Chase continuation pointers to check this case, until
-  // we hit the end of the chain (parent_ctx == 0) or a safety limit in case of
-  // an invalid continuation chain.
-  auto max_num_frames = 512;
-  for (lldb::addr_t parent_ctx = lhs_cfa; parent_ctx && max_num_frames;
-       max_num_frames--) {
-    Status error;
-    lldb::addr_t old_parent_ctx = parent_ctx;
-    // The continuation's context is the first field of an async context.
-    parent_ctx = process.ReadPointerFromMemory(old_parent_ctx, error);
-    if (error.Fail()) {
-      Log *log = GetLog(LLDBLog::Unwind);
-      LLDB_LOGF(log, "Failed to read parent async context of: 0x%8.8" PRIx64,
-                old_parent_ctx);
-      break;
-    }
-    if (parent_ctx == rhs_cfa)
-      return HeapCFAComparisonResult::Younger;
-  }
-
+  // or indirectly) RHS.
+  llvm::Expected<bool> lhs_younger =
+      IsReachableParent(lhs_cfa, rhs_cfa, process);
+  if (auto E = lhs_younger.takeError())
+    LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "{0}");
+  else if (*lhs_younger)
+    return HeapCFAComparisonResult::Younger;
+  llvm::Expected<bool> lhs_older = IsReachableParent(rhs_cfa, lhs_cfa, process);
+  if (auto E = lhs_older.takeError())
+    LLDB_LOG_ERROR(GetLog(LLDBLog::Unwind), std::move(E), "{0}");
+  else if (*lhs_older)
+    return HeapCFAComparisonResult::Older;
   return HeapCFAComparisonResult::NoOpinion;
 }
 // END SWIFT
 
 bool StackID::IsYounger(const StackID &lhs, const StackID &rhs,
                         Process &process) {
   // BEGIN SWIFT
-  switch (IsYoungerHeapCFAs(lhs, rhs, process)) {
+  switch (CompareHeapCFAs(lhs, rhs, process)) {
   case HeapCFAComparisonResult::Younger:
     return true;
   case HeapCFAComparisonResult::Older:

lldb/unittests/StackID/StackIDTest.cpp

@@ -123,3 +123,30 @@ TEST_F(StackIDTest, HeapHeapCFAComparison) {
   EXPECT_TRUE(StackID::IsYounger(middle_cfa, oldest_cfa, process));
   EXPECT_FALSE(StackID::IsYounger(oldest_cfa, middle_cfa, process));
 }
+
+TEST_F(StackIDTest, HeapHeapCFAComparisonDecreasing) {
+  // Create a mock async continuation chain:
+  // 100 -> 90 -> 80 -> 0
+  // This should be read as:
+  // "Async context whose address is 100 has a continuation context whose
+  // address is 90", etc.
+  llvm::DenseMap<addr_t, addr_t> memory_map;
+  memory_map[100] = 90;
+  memory_map[90] = 80;
+  memory_map[80] = 0;
+  auto process = MockProcess(m_target_sp, Listener::MakeListener("dummy"),
+                             std::move(memory_map));
+
+  MockStackID oldest_cfa(/*cfa*/ 80, OnStack::No);
+  MockStackID middle_cfa(/*cfa*/ 90, OnStack::No);
+  MockStackID youngest_cfa(/*cfa*/ 100, OnStack::No);
+
+  EXPECT_TRUE(StackID::IsYounger(youngest_cfa, oldest_cfa, process));
+  EXPECT_FALSE(StackID::IsYounger(oldest_cfa, youngest_cfa, process));
+
+  EXPECT_TRUE(StackID::IsYounger(youngest_cfa, middle_cfa, process));
+  EXPECT_FALSE(StackID::IsYounger(middle_cfa, youngest_cfa, process));
+
+  EXPECT_TRUE(StackID::IsYounger(middle_cfa, oldest_cfa, process));
+  EXPECT_FALSE(StackID::IsYounger(oldest_cfa, middle_cfa, process));
+}