Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashgti merged 3 commits into from
Mar 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a696c1c
[lldb-dap] Adding a DAPError for showing users error messages.
ashgti Mar 20, 2025
b437151
Adjusting 'showUser' to default to true for a DAPError and adding a m…
ashgti Mar 20, 2025
d0e3535
Also allowing the url and urlLabel to be configured in the DAPError.
ashgti Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lldb/tools/lldb-dap/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ add_lldb_tool(lldb-dap
Breakpoint.cpp
BreakpointBase.cpp
DAP.cpp
DAPError.cpp
DAPLog.cpp
EventHelper.cpp
ExceptionBreakpoint.cpp
Expand Down
26 changes: 26 additions & 0 deletions lldb/tools/lldb-dap/DAPError.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
33 changes: 33 additions & 0 deletions lldb/tools/lldb-dap/DAPError.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
Copy link
Contributor

@Jlalond Jlalond Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ashgti ashgti Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screenshot 2025-03-20 at 12 03 03 PM

Copy link
Contributor Author

@ashgti ashgti Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Jlalond Jlalond Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great!


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
28 changes: 21 additions & 7 deletions lldb/tools/lldb-dap/Handler/RequestHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>)
Expand Down
4 changes: 2 additions & 2 deletions lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
Expand Down