[lldb/Interpreter] Discard ScriptedThreadPlan::GetStopDescription return value

[medismailben]

This patch changes ScriptedThreadPlan::GetStopDescription behavior by discarding its return value since it is optional in the first place (the user doesn't need to provide a return value in their implementation).

This patch also addresses the test failures in TestStepScripted following 9a9ec22 and re-enables the tests that were XFAIL'd previously.

The issue here was that the Stream* that's passed to ThreadPlanPython::GetDescription wasn't being passed by reference to the python method so it was never updated to reflect how the python method interacted with it.

This patch solves this issue by making a temporary StreamSP that will be passed to the python method by reference, after what we will copy its content to the caller Stream pointer argument.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Med Ismail Bennani (medismailben)

Changes

This patch changes ScriptedThreadPlan::GetStopDescription behaviour by discarding its return value since it is optional in the first place (i.e. the user doesn't need to provide a return value in their implementation).

This patch also re-enables the tests that were XFAIL'd previously.

---

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

5 Files Affected:

• (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadPlanInterface.h (+2-2)
• (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp (+2-2)
• (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h (+1-1)
• (modified) lldb/source/Target/ThreadPlanPython.cpp (+3-3)
• (modified) lldb/test/API/functionalities/step_scripted/TestStepScripted.py (+1-4)

diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadPlanInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadPlanInterface.h
index 9130f9412cb0b..0f832b3b2029f 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadPlanInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadPlanInterface.h
@@ -29,8 +29,8 @@ class ScriptedThreadPlanInterface : public ScriptedInterface {
 
   virtual lldb::StateType GetRunState() { return lldb::eStateStepping; }
 
-  virtual llvm::Expected<bool> GetStopDescription(lldb_private::Stream *s) {
-    return true;
+  virtual llvm::Error GetStopDescription(lldb_private::Stream *s) {
+    return llvm::Error::success();
   }
 };
 } // namespace lldb_private
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp
index b7e475812f22b..8148e138ae564 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp
@@ -91,7 +91,7 @@ lldb::StateType ScriptedThreadPlanPythonInterface::GetRunState() {
       static_cast<uint32_t>(lldb::eStateStepping)));
 }
 
-llvm::Expected<bool>
+llvm::Error
 ScriptedThreadPlanPythonInterface::GetStopDescription(lldb_private::Stream *s) {
   Status error;
   Dispatch("stop_description", error, s);
@@ -99,7 +99,7 @@ ScriptedThreadPlanPythonInterface::GetStopDescription(lldb_private::Stream *s) {
   if (error.Fail())
     return error.ToError();
 
-  return true;
+  return llvm::Error::success();
 }
 
 #endif
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h
index 33f086786c47b..563874a590794 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h
@@ -40,7 +40,7 @@ class ScriptedThreadPlanPythonInterface : public ScriptedThreadPlanInterface,
 
   lldb::StateType GetRunState() override;
 
-  llvm::Expected<bool> GetStopDescription(lldb_private::Stream *s) override;
+  llvm::Error GetStopDescription(lldb_private::Stream *s) override;
 };
 } // namespace lldb_private
 
diff --git a/lldb/source/Target/ThreadPlanPython.cpp b/lldb/source/Target/ThreadPlanPython.cpp
index 373555324ba6e..5c0beb6409b90 100644
--- a/lldb/source/Target/ThreadPlanPython.cpp
+++ b/lldb/source/Target/ThreadPlanPython.cpp
@@ -182,9 +182,9 @@ void ThreadPlanPython::GetDescription(Stream *s, lldb::DescriptionLevel level) {
   if (m_implementation_sp) {
     ScriptInterpreter *script_interp = GetScriptInterpreter();
     if (script_interp) {
-      auto desc_or_err = m_interface->GetStopDescription(s);
-      if (!desc_or_err || !*desc_or_err) {
-        LLDB_LOG_ERROR(GetLog(LLDBLog::Thread), desc_or_err.takeError(),
+      llvm::Error err = m_interface->GetStopDescription(s);
+      if (err) {
+        LLDB_LOG_ERROR(GetLog(LLDBLog::Thread), std::move(err),
                        "Can't call ScriptedThreadPlan::GetStopDescription.");
         s->Printf("Python thread plan implemented by class %s.",
             m_class_name.c_str());
diff --git a/lldb/test/API/functionalities/step_scripted/TestStepScripted.py b/lldb/test/API/functionalities/step_scripted/TestStepScripted.py
index bb7479414dbbb..53901718019f9 100644
--- a/lldb/test/API/functionalities/step_scripted/TestStepScripted.py
+++ b/lldb/test/API/functionalities/step_scripted/TestStepScripted.py
@@ -7,6 +7,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
+
 class StepScriptedTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
@@ -15,14 +16,12 @@ def setUp(self):
         self.main_source_file = lldb.SBFileSpec("main.c")
         self.runCmd("command script import Steps.py")
 
-    @expectedFailureAll()
     def test_standard_step_out(self):
         """Tests stepping with the scripted thread plan laying over a standard
         thread plan for stepping out."""
         self.build()
         self.step_out_with_scripted_plan("Steps.StepOut")
 
-    @expectedFailureAll()
     def test_scripted_step_out(self):
         """Tests stepping with the scripted thread plan laying over an another
         scripted thread plan for stepping out."""
@@ -63,12 +62,10 @@ def test_misspelled_plan_name(self):
         # Make sure we didn't let the process run:
         self.assertEqual(stop_id, process.GetStopID(), "Process didn't run")
 
-    @expectedFailureAll()
     def test_checking_variable(self):
         """Test that we can call SBValue API's from a scripted thread plan - using SBAPI's to step"""
         self.do_test_checking_variable(False)
 
-    @expectedFailureAll()
     def test_checking_variable_cli(self):
         """Test that we can call SBValue API's from a scripted thread plan - using cli to step"""
         self.do_test_checking_variable(True)

[nico]

This adds a dependency cycle: lldb::SBStream is in source/API, so this makes Interpreter depend on API. But API already depends on Interpreter.

[nico]

This isn't an idle complaint: It has the effect that lldb-test no longer links after this PR if you don't pass -Wl,-dead_strip to the linker.

[nico]

Chances are it also breaks building on Windows, where /OPT:REF might not be strong enough to remove this.

Also, we do have a LLVM_NO_DEAD_STRIP option, which presumably no longer builds after this change either.

[nico]

Reverted in a2e3af5 for now.

[Endilll]

I think this is what broke my linux build with the following linker error:

mold: error: undefined symbol: lldb::SBStream::GetData()
>>> referenced by ScriptInterpreter.cpp
>>>               lib/liblldbInterpreter.a(ScriptInterpreter.cpp.o):(lldb_private::ScriptInterpreter::GetOpaqueTypeFromSBStream(lldb::SBStream const&) const)

lld complains in the same way.

[medismailben]

Looks like it broke the windows bot: https://lab.llvm.org/buildbot/#/builders/141/builds/369

@nico Thanks to the revert, I think I have a fix for this.