[lldb-dap] Forward any error from stepping.

[da-viper]

The current implementation hides any possible error from performing a step command.

It makes it easier to know where an issue is from.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

The current implementation hides any possible error from performing a step command.

---

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

3 Files Affected:

• (modified) lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp (+6-3)
• (modified) lldb/tools/lldb-dap/Handler/StepInRequestHandler.cpp (+7-4)
• (modified) lldb/tools/lldb-dap/Handler/StepOutRequestHandler.cpp (+4-2)

diff --git a/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp
index 3fa167686d2f9..02fd77470fa08 100644
--- a/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp
@@ -8,6 +8,7 @@
 
 #include "DAP.h"
 #include "EventHelper.h"
+#include "LLDBUtils.h"
 #include "Protocol/ProtocolTypes.h"
 #include "RequestHandler.h"
 #include "llvm/Support/Error.h"
@@ -33,13 +34,15 @@ Error NextRequestHandler::Run(const NextArguments &args) const {
   // Remember the thread ID that caused the resume so we can set the
   // "threadCausedFocus" boolean value in the "stopped" events.
   dap.focus_tid = thread.GetThreadID();
+  lldb::SBError error;
   if (args.granularity == eSteppingGranularityInstruction) {
-    thread.StepInstruction(/*step_over=*/true);
+    thread.StepInstruction(/*step_over=*/true, error);
   } else {
-    thread.StepOver(args.singleThread ? eOnlyThisThread : eOnlyDuringStepping);
+    thread.StepOver(args.singleThread ? eOnlyThisThread : eOnlyDuringStepping,
+                    error);
   }
 
-  return Error::success();
+  return ToError(error);
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/StepInRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/StepInRequestHandler.cpp
index 15f242a9e18ff..1a70be7d220c5 100644
--- a/lldb/tools/lldb-dap/Handler/StepInRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StepInRequestHandler.cpp
@@ -8,6 +8,7 @@
 
 #include "DAP.h"
 #include "EventHelper.h"
+#include "LLDBUtils.h"
 #include "Protocol/ProtocolRequests.h"
 #include "Protocol/ProtocolTypes.h"
 #include "RequestHandler.h"
@@ -39,9 +40,10 @@ Error StepInRequestHandler::Run(const StepInArguments &args) const {
   // "threadCausedFocus" boolean value in the "stopped" events.
   dap.focus_tid = thread.GetThreadID();
 
+  lldb::SBError error;
   if (args.granularity == eSteppingGranularityInstruction) {
-    thread.StepInstruction(/*step_over=*/false);
-    return Error::success();
+    thread.StepInstruction(/*step_over=*/false, error);
+    return ToError(error);
   }
 
   std::string step_in_target;
@@ -50,8 +52,9 @@ Error StepInRequestHandler::Run(const StepInArguments &args) const {
     step_in_target = it->second;
 
   RunMode run_mode = args.singleThread ? eOnlyThisThread : eOnlyDuringStepping;
-  thread.StepInto(step_in_target.c_str(), run_mode);
-  return Error::success();
+  thread.StepInto(step_in_target.c_str(), LLDB_INVALID_LINE_NUMBER, error,
+                  run_mode);
+  return ToError(error);
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/StepOutRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/StepOutRequestHandler.cpp
index 6b98582262a68..e31b38cb68bfd 100644
--- a/lldb/tools/lldb-dap/Handler/StepOutRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StepOutRequestHandler.cpp
@@ -8,6 +8,7 @@
 
 #include "DAP.h"
 #include "EventHelper.h"
+#include "LLDBUtils.h"
 #include "Protocol/ProtocolRequests.h"
 #include "RequestHandler.h"
 #include "llvm/Support/Error.h"
@@ -35,9 +36,10 @@ Error StepOutRequestHandler::Run(const StepOutArguments &arguments) const {
   // Remember the thread ID that caused the resume so we can set the
   // "threadCausedFocus" boolean value in the "stopped" events.
   dap.focus_tid = thread.GetThreadID();
-  thread.StepOut();
+  lldb::SBError error;
+  thread.StepOut(error);
 
-  return Error::success();
+  return ToError(error);
 }
 
 } // namespace lldb_dap

[ashgti]

For these three request handlers, should we also add a sanity check of:

  if (!SBDebugger::StateIsStoppedState(process.GetState()))
    return make_error<NotStoppedError>();

To the Run(...) so we can get a specific error if the process isn't stopped?

In normal debugging flows we shouldn't be in that situation, but our tests may incorrectly try to step when the process isn't stopped.

[ashgti]

LGTM!

[JDevlieghere]

Nice