Merge pull request #9154 from felipepiovezan/felipe/cherry-pick-step_out_preliminaries

[cherry-pick][lldb][Swift] Small steps towards fixing StepOut in async functions

Author: felipepiovezan

lldb/source/Target/StackID.cpp

@@ -73,16 +73,41 @@ bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
   return lhs_scope != rhs_scope;
 }
 
-bool StackID::IsYounger(const StackID &lhs, const StackID &rhs,
-                        Process &process) {
+// BEGIN SWIFT
+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) {
+  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)
+    return HeapCFAComparisonResult::NoOpinion;
+
   // FIXME: rdar://76119439
   // At the boundary between an async parent frame calling a regular child
   // frame, the CFA of the parent async function is a heap addresses, and the
   // CFA of concrete child function is a stack address. Therefore, if lhs is
   // on stack, and rhs is not, lhs is considered less than rhs, independent of
   // address values.
-  if (lhs.IsCFAOnStack(process) && !rhs.IsCFAOnStack(process))
+  if (lhs_cfa_on_stack && !rhs_cfa_on_stack)
+    return HeapCFAComparisonResult::Younger;
+  return HeapCFAComparisonResult::NoOpinion;
+}
+// END SWIFT
+
+bool StackID::IsYounger(const StackID &lhs, const StackID &rhs,
+                        Process &process) {
+  // BEGIN SWIFT
+  switch (IsYoungerHeapCFAs(lhs, rhs, process)) {
+  case HeapCFAComparisonResult::Younger:
     return true;
+  case HeapCFAComparisonResult::Older:
+    return false;
+  case HeapCFAComparisonResult::NoOpinion:
+    break;
+  }
+  // END SWIFT
 
   const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddress();
   const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddress();

lldb/test/API/lang/swift/async/stepping/step_out/Makefile

@@ -0,0 +1,3 @@
+SWIFT_SOURCES := main.swift
+SWIFTFLAGS_EXTRAS := -parse-as-library
+include Makefile.rules

lldb/test/API/lang/swift/async/stepping/step_out/TestSteppingOutAsync.py

@@ -0,0 +1,44 @@
+import lldb
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbtest as lldbtest
+import lldbsuite.test.lldbutil as lldbutil
+import re
+
+
+class TestCase(lldbtest.TestBase):
+
+    def check_and_get_frame_names(self, process):
+        frames = process.GetSelectedThread().frames
+        # Expected frames:
+        # ASYNC___0___ <- ASYNC___1___ <- ASYNC___2___ <- ASYNC___3___ <- Main
+        num_frames = 5
+        func_names = [frame.GetFunctionName() for frame in frames[:num_frames]]
+        for idx in range(num_frames - 1):
+            self.assertIn(f"ASYNC___{idx}___", func_names[idx])
+        self.assertIn("Main", func_names[num_frames - 1])
+        return func_names
+
+    def step_out_checks(self, thread, expected_func_names):
+        # Keep stepping out, comparing the top frame's name with the expected name.
+        for expected_func_name in expected_func_names:
+            error = lldb.SBError()
+            thread.StepOut(error)
+            self.assertSuccess(error, "step out failed")
+            stop_reason = thread.GetStopReason()
+            self.assertStopReason(stop_reason, lldb.eStopReasonPlanComplete)
+            self.assertEqual(thread.frames[0].GetFunctionName(), expected_func_name)
+
+    @swiftTest
+    @skipIf(oslist=["windows", "linux"])
+    @skipIf("rdar://133849022")
+    def test(self):
+        """Test `frame variable` in async functions"""
+        self.build()
+
+        source_file = lldb.SBFileSpec("main.swift")
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
+            self, "BREAK HERE", source_file
+        )
+
+        func_names = self.check_and_get_frame_names(process)
+        self.step_out_checks(thread, func_names[1:])

lldb/test/API/lang/swift/async/stepping/step_out/main.swift

@@ -0,0 +1,38 @@
+func work() {
+  print("working")
+}
+
+func ASYNC___0___() async -> Int {
+  var myvar = 111;
+  work()
+  myvar += 1; // BREAK HERE
+  return myvar
+}
+
+func ASYNC___1___() async -> Int {
+  var blah = 333
+  work()
+  var result = await ASYNC___0___()
+  work()
+  return result + blah
+}
+
+func ASYNC___2___() async -> Int {
+  work()
+  var result1 = await ASYNC___1___()
+  work()
+  return result1
+}
+
+func ASYNC___3___() async -> Int {
+  var result = await ASYNC___2___()
+  work()
+  return result
+}
+
+@main struct Main {
+  static func main() async {
+    let result = await ASYNC___3___()
+    print(result)
+  }
+}

lldb/unittests/CMakeLists.txt

@@ -73,6 +73,7 @@ add_subdirectory(Platform)
 add_subdirectory(Process)
 add_subdirectory(ScriptInterpreter)
 add_subdirectory(Signals)
+add_subdirectory(StackID)
 add_subdirectory(Symbol)
 add_subdirectory(SymbolFile)
 add_subdirectory(TypeSystem)

lldb/unittests/StackID/CMakeLists.txt

@@ -0,0 +1,8 @@
+add_lldb_unittest(StackIDTests
+  StackIDTest.cpp
+
+  LINK_LIBS
+      lldbCore
+      lldbTarget
+      lldbPluginPlatformMacOSX
+  )

lldb/unittests/StackID/StackIDTest.cpp

@@ -0,0 +1,93 @@
+
+#include "lldb/Target/StackID.h"
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+static std::once_flag initialize_flag;
+
+// Initialize the bare minimum to enable defining a mock Process class.
+class StackIDTest : public ::testing::Test {
+public:
+  void SetUp() override {
+    std::call_once(initialize_flag, []() {
+      HostInfo::Initialize();
+      PlatformMacOSX::Initialize();
+      FileSystem::Initialize();
+    });
+    ArchSpec arch("x86_64-apple-macosx-");
+    Platform::SetHostPlatform(
+        PlatformRemoteMacOSX::CreateInstance(true, &arch));
+    m_debugger_sp = Debugger::CreateInstance();
+    m_debugger_sp->GetTargetList().CreateTarget(*m_debugger_sp, "", arch,
+                                                eLoadDependentsNo,
+                                                m_platform_sp, m_target_sp);
+    ASSERT_TRUE(m_target_sp);
+    ASSERT_TRUE(m_target_sp->GetArchitecture().IsValid());
+    ASSERT_TRUE(m_platform_sp);
+  }
+
+  PlatformSP m_platform_sp;
+  TargetSP m_target_sp;
+  DebuggerSP m_debugger_sp;
+};
+
+struct MockProcess : Process {
+  MockProcess(TargetSP target_sp, ListenerSP listener_sp)
+      : Process(target_sp, listener_sp) {}
+  size_t DoReadMemory(addr_t vm_addr, void *buf, size_t size,
+                      Status &error) override {
+    return 0;
+  }
+  size_t ReadMemory(addr_t addr, void *buf, size_t size,
+                    Status &status) override {
+    return DoReadMemory(addr, buf, size, status);
+  }
+  bool CanDebug(TargetSP, bool) override { return true; }
+  Status DoDestroy() override { return Status(); }
+  llvm::StringRef GetPluginName() override { return ""; }
+  void RefreshStateAfterStop() override {}
+  bool DoUpdateThreadList(ThreadList &, ThreadList &) override { return false; }
+};
+
+enum OnStack { Yes, No };
+/// Helper class to enable testing StackID::IsYounger.
+struct MockStackID : StackID {
+  MockStackID(addr_t cfa, OnStack on_stack) : StackID() {
+    SetPC(0);
+    SetCFA(cfa);
+    m_cfa_on_stack = on_stack == OnStack::Yes ? LazyBool::eLazyBoolYes
+                                              : LazyBool::eLazyBoolNo;
+  }
+};
+
+TEST_F(StackIDTest, StackStackCFAComparison) {
+  auto process = MockProcess(m_target_sp, Listener::MakeListener("dummy"));
+
+  MockStackID small_cfa_on_stack(/*cfa*/ 10, OnStack::Yes);
+  MockStackID big_cfa_on_stack(/*cfa*/ 100, OnStack::Yes);
+
+  EXPECT_TRUE(
+      StackID::IsYounger(small_cfa_on_stack, big_cfa_on_stack, process));
+  EXPECT_FALSE(
+      StackID::IsYounger(big_cfa_on_stack, small_cfa_on_stack, process));
+}
+
+TEST_F(StackIDTest, StackHeapCFAComparison) {
+  auto process = MockProcess(m_target_sp, Listener::MakeListener("dummy"));
+
+  MockStackID cfa_on_stack(/*cfa*/ 100, OnStack::Yes);
+  MockStackID cfa_on_heap(/*cfa*/ 10, OnStack::No);
+
+  EXPECT_TRUE(StackID::IsYounger(cfa_on_stack, cfa_on_heap, process));
+
+  // FIXME: if the above returned true, swapping the arguments **must** return
+  // false. And yet it doesn't.
+  // EXPECT_FALSE(StackID::IsYounger(cfa_on_heap, cfa_on_stack, process));
+}