[lldb-dap] Adding a DAPError for showing users error messages.

[ashgti]

The DAPError can be used to craft an error message that is displayed to a user (with showUser=true).

Any request handler implementation using subclassing RequestHandler<> should be able to use this.

I updated SourceRequestHandler to report DAPError's specifically.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

The DAPError can be used to craft an error message that is displayed to a user (with showUser=true).

Any request handler implementation using subclassing RequestHandler<> should be able to use this.

I updated SourceRequestHandler to report DAPError's specifically.

---

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

5 Files Affected:

• (modified) lldb/tools/lldb-dap/CMakeLists.txt (+1)
• (added) lldb/tools/lldb-dap/DAPError.cpp (+26)
• (added) lldb/tools/lldb-dap/DAPError.h (+33)
• (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+21-7)
• (modified) lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp (+2-2)

diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index 93c5ee4426783..e9773db7586e6 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -18,6 +18,7 @@ add_lldb_tool(lldb-dap
   Breakpoint.cpp
   BreakpointBase.cpp
   DAP.cpp
+  DAPError.cpp
   DAPLog.cpp
   EventHelper.cpp
   ExceptionBreakpoint.cpp
diff --git a/lldb/tools/lldb-dap/DAPError.cpp b/lldb/tools/lldb-dap/DAPError.cpp
new file mode 100644
index 0000000000000..044c9b48d61c6
--- /dev/null
+++ b/lldb/tools/lldb-dap/DAPError.cpp
@@ -0,0 +1,26 @@
+//===-- DAPError.cpp ------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DAPError.h"
+#include "llvm/Support/Error.h"
+#include <system_error>
+
+namespace lldb_dap {
+
+char DAPError::ID;
+
+DAPError::DAPError(std::string message, bool show_user)
+    : m_message(message), m_show_user(show_user) {}
+
+void DAPError::log(llvm::raw_ostream &OS) const { OS << m_message; }
+
+std::error_code DAPError::convertToErrorCode() const {
+  return llvm::inconvertibleErrorCode();
+}
+
+} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/DAPError.h b/lldb/tools/lldb-dap/DAPError.h
new file mode 100644
index 0000000000000..b990bcdb5ceb0
--- /dev/null
+++ b/lldb/tools/lldb-dap/DAPError.h
@@ -0,0 +1,33 @@
+//===-- DAPError.h --------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Error.h"
+#include <string>
+
+namespace lldb_dap {
+
+/// An Error that is reported as a DAP Error Message, which may be presented to
+/// the user.
+class DAPError : public llvm::ErrorInfo<DAPError> {
+public:
+  static char ID;
+
+  DAPError(std::string message, bool show_user = false);
+
+  void log(llvm::raw_ostream &OS) const override;
+  std::error_code convertToErrorCode() const override;
+
+  const std::string &getMessage() const { return m_message; }
+  bool getShowUser() const { return m_show_user; }
+
+private:
+  std::string m_message;
+  bool m_show_user;
+};
+
+} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index d327820224a30..01eaf3a6cfe65 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -10,11 +10,13 @@
 #define LLDB_TOOLS_LLDB_DAP_HANDLER_HANDLER_H
 
 #include "DAP.h"
+#include "DAPError.h"
 #include "DAPLog.h"
 #include "Protocol/ProtocolBase.h"
 #include "Protocol/ProtocolRequests.h"
 #include "lldb/API/SBError.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
 #include <optional>
 #include <type_traits>
@@ -118,20 +120,32 @@ class RequestHandler : public BaseRequestHandler {
       std::string parse_failure;
       llvm::raw_string_ostream OS(parse_failure);
       root.printErrorContext(request.arguments, OS);
+
+      protocol::ErrorMessage error;
+      error.format = parse_failure;
+
       response.success = false;
-      response.message = parse_failure;
+      response.body = std::move(error);
+
       dap.Send(response);
       return;
     }
 
-    auto body = Run(arguments);
-    // FIXME: Add a dedicated DAPError for enhanced errors that are
-    // user-visibile.
+    llvm::Expected<Body> body = Run(arguments);
     if (auto Err = body.takeError()) {
+      protocol::ErrorMessage error;
+      if (llvm::Error unhandled = llvm::handleErrors(
+              std::move(Err), [&](const DAPError &E) -> llvm::Error {
+                error.format = E.getMessage();
+                error.showUser = E.getShowUser();
+                error.id = E.convertToErrorCode().value();
+                // TODO: We could add url/urlLabel to the error message for more
+                // information for users.
+                return llvm::Error::success();
+              }))
+        error.format = llvm::toString(std::move(unhandled));
       response.success = false;
-      // FIXME: Build ErrorMessage based on error details instead of using the
-      // 'message' field.
-      response.message = llvm::toString(std::move(Err));
+      response.body = std::move(error);
     } else {
       response.success = true;
       if constexpr (!std::is_same_v<Body, std::monostate>)
diff --git a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
index 57cfb09692990..1a7a13d9f267a 100644
--- a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
@@ -29,7 +29,7 @@ SourceRequestHandler::Run(const protocol::SourceArguments &args) const {
       args.source->sourceReference.value_or(args.sourceReference);
 
   if (!source)
-    return llvm::createStringError(
+    return llvm::make_error<DAPError>(
         "invalid arguments, expected source.sourceReference to be set");
 
   lldb::SBProcess process = dap.target.GetProcess();
@@ -39,7 +39,7 @@ SourceRequestHandler::Run(const protocol::SourceArguments &args) const {
   // Lower 32 bits is the frame index
   lldb::SBFrame frame = thread.GetFrameAtIndex(GetLLDBFrameID(source));
   if (!frame.IsValid())
-    return llvm::createStringError("source not found");
+    return llvm::make_error<DAPError>("source not found");
 
   lldb::SBInstructionList insts = frame.GetSymbol().GetInstructions(dap.target);
   lldb::SBStream stream;

[JDevlieghere]

LGTM

[Jlalond]

Why the default to false for show_user? I can't think of an example of an error that I wouldn't want to surface (off the top of my head).

Additionally, I think this should probably not have a default and every construction should set it explicitly, but again I can't think of a scenario where I wouldn't want to show it

[ashgti]

I adjusted this to default to true. The field is optional in Message but I think your right that most of the time we're creating a DAPError we'd want to show the user.

For reference, when an error is shown, it looks like:

[ashgti]

I updated the DAPError to allow callers to set a few additional fields as well. I'm not sure if we'll use them all yet, but they're there if we want to add a URL link to an error message. For example, the launch request could link to https://github.com/llvm/llvm-project/blob/main/lldb/tools/lldb-dap/README.md#configuration-settings-reference if we have an error launching.

[Jlalond]

I think this is great!