[Lldb-commits] [lldb] [lldb] Add Status::Detail to store expression evaluator diagnostics [… (PR #106442)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Aug 28 12:30:41 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Adrian Prantl (adrian-prantl)
<details>
<summary>Changes</summary>
…NFC]
This patch is the first patch in a series reworking of Pete Lawrence's (@<!-- -->PortalPete) amazing proposal for better expression evaluator error messages (https://github.com/llvm/llvm-project/pull/80938)
This patch is preparatory patch for improving the rendering of expression evaluator diagnostics. Currently diagnostics are rendered into a string and the command interpreter layer then textually parses words like "error:" to (sometimes) color the output accordingly. In order to enable user interfaces to do better with diagnostics, we need to store them in a machine-readable fromat. This patch does this by adding a Status::Detail vector to Status that is used when the error type is eErrorTypeExpression. I tried my best to minimize the overhead this adds to Status in all other use-cases.
Right now the extra information is not used by the CommandInterpreter, this will be added in a follow-up patch!
---
Full diff: https://github.com/llvm/llvm-project/pull/106442.diff
9 Files Affected:
- (modified) lldb/include/lldb/Expression/DiagnosticManager.h (+22-28)
- (modified) lldb/include/lldb/Utility/Status.h (+48-2)
- (modified) lldb/source/Expression/DiagnosticManager.cpp (+27)
- (modified) lldb/source/Expression/UserExpression.cpp (+13-13)
- (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h (+2-3)
- (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+40-7)
- (modified) lldb/source/Utility/Status.cpp (+21-4)
- (modified) lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py (+1-1)
- (modified) lldb/unittests/Expression/DiagnosticManagerTest.cpp (+6-6)
``````````diff
diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h
index d49b7c99b114fb..252492ce8f776a 100644
--- a/lldb/include/lldb/Expression/DiagnosticManager.h
+++ b/lldb/include/lldb/Expression/DiagnosticManager.h
@@ -12,6 +12,8 @@
#include "lldb/lldb-defines.h"
#include "lldb/lldb-types.h"
+#include "lldb/Utility/Status.h"
+
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
@@ -49,37 +51,28 @@ class Diagnostic {
}
}
- Diagnostic(llvm::StringRef message, lldb::Severity severity,
- DiagnosticOrigin origin, uint32_t compiler_id)
- : m_message(message), m_severity(severity), m_origin(origin),
- m_compiler_id(compiler_id) {}
-
- Diagnostic(const Diagnostic &rhs)
- : m_message(rhs.m_message), m_severity(rhs.m_severity),
- m_origin(rhs.m_origin), m_compiler_id(rhs.m_compiler_id) {}
+ Diagnostic(DiagnosticOrigin origin, uint32_t compiler_id,
+ Status::Detail detail)
+ : m_origin(origin), m_compiler_id(compiler_id), m_detail(detail) {}
virtual ~Diagnostic() = default;
virtual bool HasFixIts() const { return false; }
- lldb::Severity GetSeverity() const { return m_severity; }
+ lldb::Severity GetSeverity() const { return m_detail.severity; }
uint32_t GetCompilerID() const { return m_compiler_id; }
- llvm::StringRef GetMessage() const { return m_message; }
+ llvm::StringRef GetMessage() const { return m_detail.message; }
+ Status::Detail GetDetail() const { return m_detail; }
- void AppendMessage(llvm::StringRef message,
- bool precede_with_newline = true) {
- if (precede_with_newline)
- m_message.push_back('\n');
- m_message += message;
- }
+ void AppendMessage(llvm::StringRef message, bool precede_with_newline = true);
protected:
- std::string m_message;
- lldb::Severity m_severity;
DiagnosticOrigin m_origin;
- uint32_t m_compiler_id; // Compiler-specific diagnostic ID
+ /// Compiler-specific diagnostic ID.
+ uint32_t m_compiler_id;
+ Status::Detail m_detail;
};
typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList;
@@ -102,10 +95,7 @@ class DiagnosticManager {
void AddDiagnostic(llvm::StringRef message, lldb::Severity severity,
DiagnosticOrigin origin,
- uint32_t compiler_id = LLDB_INVALID_COMPILER_ID) {
- m_diagnostics.emplace_back(
- std::make_unique<Diagnostic>(message, severity, origin, compiler_id));
- }
+ uint32_t compiler_id = LLDB_INVALID_COMPILER_ID);
void AddDiagnostic(std::unique_ptr<Diagnostic> diagnostic) {
if (diagnostic)
@@ -130,13 +120,17 @@ class DiagnosticManager {
m_diagnostics.back()->AppendMessage(str);
}
- // Returns a string containing errors in this format:
- //
- // "error: error text\n
- // warning: warning text\n
- // remark text\n"
+ /// Returns a string containing errors in this format:
+ ///
+ /// "error: error text\n
+ /// warning: warning text\n
+ /// remark text\n"
+ LLVM_DEPRECATED("Use GetAsStatus instead", "GetAsStatus()")
std::string GetString(char separator = '\n');
+ /// Copies the diagnostics into an lldb::Status.
+ Status GetAsStatus(lldb::ExpressionResults result);
+
void Dump(Log *log);
const std::string &GetFixedExpression() { return m_fixed_expression; }
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index b304291ffae00e..5a9683e09c6e53 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -9,6 +9,7 @@
#ifndef LLDB_UTILITY_STATUS_H
#define LLDB_UTILITY_STATUS_H
+#include "lldb/Utility/FileSpec.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-enumerations.h"
#include "llvm/ADT/StringRef.h"
@@ -47,7 +48,34 @@ class Status {
/// into ValueType.
typedef uint32_t ValueType;
- Status();
+ /// A customizable "detail" for an error. For example, expression
+ /// evaluation failures often have more than one diagnostic that the
+ /// UI layer might want to render differently.
+ ///
+ /// Running example:
+ /// (lldb) expr 1+x
+ /// error: <user expression 0>:1:3: use of undeclared identifier 'foo'
+ /// 1+foo
+ /// ^
+ struct Detail {
+ struct SourceLocation {
+ FileSpec file;
+ unsigned line = 0;
+ uint16_t column = 0;
+ uint16_t length = 0;
+ bool in_user_input = false;
+ };
+ /// Contains {{}, 1, 3, 3, true} in the example above.
+ std::optional<SourceLocation> source_location;
+ /// Contains eSeverityError in the example above.
+ lldb::Severity severity = lldb::eSeverityInfo;
+ /// Contains "use of undeclared identifier 'x'" in the example above.
+ std::string message;
+ /// Contains the fully rendered error message.
+ std::string rendered;
+ };
+
+ Status() = default;
/// Initialize the error object with a generic success value.
///
@@ -57,7 +85,8 @@ class Status {
/// \param[in] type
/// The type for \a err.
explicit Status(ValueType err, lldb::ErrorType type = lldb::eErrorTypeGeneric,
- std::string msg = {});
+ std::string msg = {})
+ : m_code(err), m_type(type), m_string(std::move(msg)) {}
Status(std::error_code EC);
@@ -83,6 +112,12 @@ class Status {
return Status(result, lldb::eErrorTypeExpression, msg);
}
+ static Status
+ FromExpressionErrorDetails(lldb::ExpressionResults result,
+ llvm::SmallVectorImpl<Detail> &&details) {
+ return Status(result, lldb::eErrorTypeExpression, std::move(details));
+ }
+
/// Set the current error to errno.
///
/// Update the error value to be \c errno and update the type to be \c
@@ -144,13 +179,24 @@ class Status {
/// success (non-erro), \b false otherwise.
bool Success() const;
+ /// Get the list of details for this error. If this is non-empty,
+ /// clients can use this to render more appealing error messages
+ /// from the details. If you just want a pre-rendered string, use
+ /// AsCString() instead. Currently details are only used for
+ /// eErrorTypeExpression.
+ llvm::ArrayRef<Detail> GetDetails() const;
+
protected:
+ /// Separate so the main constructor doesn't need to deal with the array.
+ Status(ValueType err, lldb::ErrorType type,
+ llvm::SmallVectorImpl<Detail> &&details);
/// Status code as an integer value.
ValueType m_code = 0;
/// The type of the above error code.
lldb::ErrorType m_type = lldb::eErrorTypeInvalid;
/// A string representation of the error code.
mutable std::string m_string;
+ llvm::SmallVector<Detail, 0> m_details;
};
} // namespace lldb_private
diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp
index a8330138f3d53b..5c20eba1dbd07e 100644
--- a/lldb/source/Expression/DiagnosticManager.cpp
+++ b/lldb/source/Expression/DiagnosticManager.cpp
@@ -65,6 +65,23 @@ std::string DiagnosticManager::GetString(char separator) {
return ret;
}
+Status DiagnosticManager::GetAsStatus(lldb::ExpressionResults result) {
+ llvm::SmallVector<Status::Detail, 0> details;
+ details.reserve(m_diagnostics.size());
+ for (const auto &diagnostic : m_diagnostics)
+ details.push_back(diagnostic->GetDetail());
+ return Status::FromExpressionErrorDetails(result, std::move(details));
+}
+
+void DiagnosticManager::AddDiagnostic(llvm::StringRef message,
+ lldb::Severity severity,
+ DiagnosticOrigin origin,
+ uint32_t compiler_id) {
+ m_diagnostics.emplace_back(std::make_unique<Diagnostic>(
+ origin, compiler_id,
+ Status::Detail{{}, severity, message.str(), message.str()}));
+}
+
size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format,
...) {
StreamString ss;
@@ -85,3 +102,13 @@ void DiagnosticManager::PutString(lldb::Severity severity,
return;
AddDiagnostic(str, severity, eDiagnosticOriginLLDB);
}
+
+void Diagnostic::AppendMessage(llvm::StringRef message,
+ bool precede_with_newline) {
+ if (precede_with_newline) {
+ m_detail.message.push_back('\n');
+ m_detail.rendered.push_back('\n');
+ }
+ m_detail.message += message;
+ m_detail.rendered += message;
+}
diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp
index c2889e4c986bfe..159714489ad111 100644
--- a/lldb/source/Expression/UserExpression.cpp
+++ b/lldb/source/Expression/UserExpression.cpp
@@ -328,18 +328,19 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
}
if (!parse_success) {
- std::string msg;
- {
- llvm::raw_string_ostream os(msg);
- if (!diagnostic_manager.Diagnostics().empty())
- os << diagnostic_manager.GetString();
- else
- os << "expression failed to parse (no further compiler diagnostics)";
- if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
- !fixed_expression->empty())
- os << "\nfixed expression suggested:\n " << *fixed_expression;
+ if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
+ !fixed_expression->empty()) {
+ std::string fixit =
+ "fixed expression suggested:\n " + *fixed_expression;
+ diagnostic_manager.AddDiagnostic(fixit, lldb::eSeverityInfo,
+ eDiagnosticOriginLLDB);
}
- error = Status::FromExpressionError(execution_results, msg);
+ if (!diagnostic_manager.Diagnostics().size())
+ error = Status::FromExpressionError(
+ execution_results,
+ "expression failed to parse (no further compiler diagnostics)");
+ else
+ error = diagnostic_manager.GetAsStatus(execution_results);
}
}
@@ -384,8 +385,7 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
error = Status::FromExpressionError(
execution_results, "expression failed to execute, unknown error");
else
- error = Status::FromExpressionError(execution_results,
- diagnostic_manager.GetString());
+ error = diagnostic_manager.GetAsStatus(execution_results);
} else {
if (expr_result) {
result_valobj_sp = expr_result->GetValueObject();
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
index 21abd71cc34eeb..f6e635afbf68ca 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
@@ -29,9 +29,8 @@ class ClangDiagnostic : public Diagnostic {
return diag->getKind() == eDiagnosticOriginClang;
}
- ClangDiagnostic(llvm::StringRef message, lldb::Severity severity,
- uint32_t compiler_id)
- : Diagnostic(message, severity, eDiagnosticOriginClang, compiler_id) {}
+ ClangDiagnostic(Status::Detail detail, uint32_t compiler_id)
+ : Diagnostic(eDiagnosticOriginClang, compiler_id, detail) {}
~ClangDiagnostic() override = default;
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 90f26de939a478..6b524250bbf4de 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -26,6 +26,7 @@
#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "clang/Frontend/FrontendPluginRegistry.h"
+#include "clang/Frontend/TextDiagnostic.h"
#include "clang/Frontend/TextDiagnosticBuffer.h"
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Lex/Preprocessor.h"
@@ -212,20 +213,20 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
m_passthrough->HandleDiagnostic(DiagLevel, Info);
m_os->flush();
- lldb::Severity severity;
+ Status::Detail detail;
bool make_new_diagnostic = true;
switch (DiagLevel) {
case DiagnosticsEngine::Level::Fatal:
case DiagnosticsEngine::Level::Error:
- severity = lldb::eSeverityError;
+ detail.severity = lldb::eSeverityError;
break;
case DiagnosticsEngine::Level::Warning:
- severity = lldb::eSeverityWarning;
+ detail.severity = lldb::eSeverityWarning;
break;
case DiagnosticsEngine::Level::Remark:
case DiagnosticsEngine::Level::Ignored:
- severity = lldb::eSeverityInfo;
+ detail.severity = lldb::eSeverityInfo;
break;
case DiagnosticsEngine::Level::Note:
m_manager->AppendMessageToDiagnostic(m_output);
@@ -254,14 +255,46 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
std::string stripped_output =
std::string(llvm::StringRef(m_output).trim());
- auto new_diagnostic = std::make_unique<ClangDiagnostic>(
- stripped_output, severity, Info.getID());
+ // Translate the source location.
+ if (Info.hasSourceManager()) {
+ Status::Detail::SourceLocation loc;
+ clang::SourceManager &sm = Info.getSourceManager();
+ const clang::SourceLocation sloc = Info.getLocation();
+ if (sloc.isValid()) {
+ const clang::FullSourceLoc fsloc(sloc, sm);
+ clang::PresumedLoc PLoc = fsloc.getPresumedLoc(true);
+ StringRef filename =
+ PLoc.isValid() ? PLoc.getFilename() : StringRef{};
+ loc.file = FileSpec(filename);
+ loc.line = fsloc.getSpellingLineNumber();
+ loc.column = fsloc.getSpellingColumnNumber();
+ // A heuristic detecting the #line 1 "<user expression 1>".
+ loc.in_user_input = filename.starts_with("<user expression ");
+ // Find the range of the primary location.
+ for (const auto &range : Info.getRanges()) {
+ if (range.getBegin() == sloc) {
+ // FIXME: This is probably not handling wide characters correctly.
+ unsigned end_col = sm.getSpellingColumnNumber(range.getEnd());
+ if (end_col > loc.column)
+ loc.length = end_col - loc.column;
+ break;
+ }
+ }
+ detail.source_location = loc;
+ }
+ }
+ llvm::SmallString<0> msg;
+ Info.FormatDiagnostic(msg);
+ detail.message = msg.str();
+ detail.rendered = stripped_output;
+ auto new_diagnostic =
+ std::make_unique<ClangDiagnostic>(detail, Info.getID());
// Don't store away warning fixits, since the compiler doesn't have
// enough context in an expression for the warning to be useful.
// FIXME: Should we try to filter out FixIts that apply to our generated
// code, and not the user's expression?
- if (severity == lldb::eSeverityError)
+ if (detail.severity == lldb::eSeverityError)
AddAllFixIts(new_diagnostic.get(), Info);
m_manager->AddDiagnostic(std::move(new_diagnostic));
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 7260b7b3e0a03e..3b552e5540df61 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -37,10 +37,9 @@ class raw_ostream;
using namespace lldb;
using namespace lldb_private;
-Status::Status() {}
-
-Status::Status(ValueType err, ErrorType type, std::string msg)
- : m_code(err), m_type(type), m_string(std::move(msg)) {}
+Status::Status(ValueType err, ErrorType type,
+ llvm::SmallVectorImpl<Status::Detail> &&details)
+ : m_code(err), m_type(type), m_details(std::move(details)) {}
// This logic is confusing because c++ calls the traditional (posix) errno codes
// "generic errors", while we use the term "generic" to mean completely
@@ -133,6 +132,19 @@ static std::string RetrieveWin32ErrorString(uint32_t error_code) {
}
#endif
+static const char *StringForSeverity(lldb::Severity severity) {
+ switch (severity) {
+ // this should be exhaustive
+ case lldb::eSeverityError:
+ return "error: ";
+ case lldb::eSeverityWarning:
+ return "warning: ";
+ case lldb::eSeverityInfo:
+ return "";
+ }
+ llvm_unreachable("switch needs another case for lldb::Severity enum");
+}
+
// Get the error value as a NULL C string. The error string will be fetched and
// cached on demand. The cached error string value will remain until the error
// value is changed or cleared.
@@ -159,6 +171,11 @@ const char *Status::AsCString(const char *default_error_str) const {
#endif
break;
+ case eErrorTypeExpression: {
+ llvm::raw_string_ostream stream(m_string);
+ for (const auto &detail : m_details)
+ stream << StringForSeverity(detail.severity) << detail.rendered << '\n';
+ } break;
default:
break;
}
diff --git a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
index 620b6e44fc852a..36e302be2525b5 100644
--- a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
+++ b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
@@ -21,7 +21,7 @@ def test(self):
"expr @import LLDBTestModule",
error=True,
substrs=[
- "module.h:4:1: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
+ "module.h:4:1: error: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
"syntax_error_for_lldb_to_find // comment that tests source printing",
"could not build module 'LLDBTestModule'",
],
diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
index 05fe7c164d6818..bf554020ad8924 100644
--- a/lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -19,8 +19,8 @@ class FixItDiag : public Diagnostic {
public:
FixItDiag(llvm::StringRef msg, bool has_fixits)
- : Diagnostic(msg, lldb::eSeverityError,
- DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id),
+ : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
+ Status::Detail{{}, lldb::eSeverityError, msg.str(), {}}),
m_has_fixits(has_fixits) {}
bool HasFixIts() const override { return m_has_fixits; }
};
@@ -30,8 +30,8 @@ namespace {
class TextDiag : public Diagnostic {
public:
TextDiag(llvm::StringRef msg, lldb::Severity severity)
- : Diagnostic(msg, severity, DiagnosticOrigin::eDiagnosticOriginLLDB,
- custom_diag_id) {}
+ : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
+ Status::Detail{{}, severity, msg.str(), {}}) {}
};
} // namespace
@@ -42,8 +42,8 @@ TEST(DiagnosticManagerTest, AddDiagnostic) {
std::string msg = "foo bar has happened";
lldb::Severity severity = lldb::eSeverityError;
DiagnosticOrigin origin = DiagnosticOrigin::eDiagnosticOriginLLDB;
- auto diag =
- std::make_unique<Diagnostic>(msg, severity, origin, custom_diag_id);
+ auto diag = std::make_unique<Diagnostic>(
+ origin, custom_diag_id, Status::Detail{{}, severity, msg, {}});
mgr.AddDiagnostic(std::move(diag));
EXPECT_EQ(1U, mgr.Diagnostics().size());
const Diagnostic *got = mgr.Diagnostics().front().get();
``````````
</details>
https://github.com/llvm/llvm-project/pull/106442
More information about the lldb-commits
mailing list