Skip to content

Commit d33fa70

Browse files
adrian-prantladrian-prantl
committed
[lldb] Inline expression evaluator error visualization (#106470)
This patch is a reworking of Pete Lawrence's (@PortalPete) proposal for better expression evaluator error messages: #80938 Before: ``` $ lldb -o "expr a+b" (lldb) expr a+b error: <user expression 0>:1:1: use of undeclared identifier 'a' a+b ^ error: <user expression 0>:1:3: use of undeclared identifier 'b' a+b ^ ``` After: ``` (lldb) expr a+b ^ ^ │ ╰─ error: use of undeclared identifier 'b' ╰─ error: use of undeclared identifier 'a' ``` This eliminates the confusing `<user expression 0>:1:3` source location and avoids echoing the expression to the console again, which results in a cleaner presentation that makes it easier to grasp what's going on. You can't see it here, bug the word "error" is now also in color, if so desired. Depends on #106442.
1 parent df4d7d3 commit d33fa70

File tree

25 files changed

+327
-53
lines changed

25 files changed

+327
-53
lines changed

‎lldb/include/lldb/API/SBDebugger.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,8 @@ class LLDB_API SBDebugger {
304304

305305
bool GetUseColor() const;
306306

307+
bool SetShowInlineDiagnostics(bool);
308+
307309
bool SetUseSourceCache(bool use_source_cache);
308310

309311
bool GetUseSourceCache() const;

‎lldb/include/lldb/Core/Debugger.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
364364

365365
const std::string &GetInstanceName() { return m_instance_name; }
366366

367+
bool GetShowInlineDiagnostics() const;
368+
369+
bool SetShowInlineDiagnostics(bool);
370+
367371
bool LoadPlugin(const FileSpec &spec, Status &error);
368372

369373
void RunIOHandlers();

‎lldb/include/lldb/Expression/DiagnosticManager.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ struct DiagnosticDetail {
3939
unsigned line = 0;
4040
uint16_t column = 0;
4141
uint16_t length = 0;
42+
bool hidden = false;
4243
bool in_user_input = false;
4344
};
4445
/// Contains {{}, 1, 3, 3, true} in the example above.

‎lldb/include/lldb/Interpreter/CommandObject.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,13 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
340340
return false;
341341
}
342342

343+
/// Set the command input as it appeared in the terminal. This
344+
/// is used to have errors refer directly to the original command.
345+
void SetOriginalCommandString(std::string s) { m_original_command = s; }
346+
347+
/// \param offset_in_command is on what column \c args_string
348+
/// appears, if applicable. This enables diagnostics that refer back
349+
/// to the user input.
343350
virtual void Execute(const char *args_string,
344351
CommandReturnObject &result) = 0;
345352

@@ -404,6 +411,7 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
404411
std::string m_cmd_help_short;
405412
std::string m_cmd_help_long;
406413
std::string m_cmd_syntax;
414+
std::string m_original_command;
407415
Flags m_flags;
408416
std::vector<CommandArgumentEntry> m_arguments;
409417
lldb::CommandOverrideCallback m_deprecated_command_override_callback;

‎lldb/source/API/SBDebugger.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1483,6 +1483,12 @@ bool SBDebugger::GetUseColor() const {
14831483
return (m_opaque_sp ? m_opaque_sp->GetUseColor() : false);
14841484
}
14851485

1486+
bool SBDebugger::SetShowInlineDiagnostics(bool value) {
1487+
LLDB_INSTRUMENT_VA(this, value);
1488+
1489+
return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
1490+
}
1491+
14861492
bool SBDebugger::SetUseSourceCache(bool value) {
14871493
LLDB_INSTRUMENT_VA(this, value);
14881494

‎lldb/source/Commands/CommandObjectExpression.cpp

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@
66
//
77
//===----------------------------------------------------------------------===//
88

9-
#include "llvm/ADT/StringRef.h"
10-
119
#include "CommandObjectExpression.h"
10+
#include "DiagnosticRendering.h"
1211
#include "lldb/Core/Debugger.h"
12+
#include "lldb/Expression/DiagnosticManager.h"
1313
#include "lldb/Expression/ExpressionVariable.h"
1414
#include "lldb/Expression/REPL.h"
1515
#include "lldb/Expression/UserExpression.h"
@@ -486,19 +486,34 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
486486

487487
result.SetStatus(eReturnStatusSuccessFinishResult);
488488
} else {
489-
const char *error_cstr = result_valobj_sp->GetError().AsCString();
490-
if (error_cstr && error_cstr[0]) {
491-
const size_t error_cstr_len = strlen(error_cstr);
492-
const bool ends_with_newline = error_cstr[error_cstr_len - 1] == '\n';
493-
if (strstr(error_cstr, "error:") != error_cstr)
494-
error_stream.PutCString("error: ");
495-
error_stream.Write(error_cstr, error_cstr_len);
496-
if (!ends_with_newline)
497-
error_stream.EOL();
489+
// Retrieve the diagnostics.
490+
std::vector<DiagnosticDetail> details;
491+
llvm::consumeError(llvm::handleErrors(
492+
result_valobj_sp->GetError().ToError(),
493+
[&](ExpressionError &error) { details = error.GetDetails(); }));
494+
// Find the position of the expression in the command.
495+
std::optional<uint16_t> expr_pos;
496+
size_t nchar = m_original_command.find(expr);
497+
if (nchar != std::string::npos)
498+
expr_pos = nchar + GetDebugger().GetPrompt().size();
499+
500+
if (!details.empty()) {
501+
bool show_inline =
502+
GetDebugger().GetShowInlineDiagnostics() && !expr.contains('\n');
503+
RenderDiagnosticDetails(error_stream, expr_pos, show_inline, details);
498504
} else {
499-
error_stream.PutCString("error: unknown error\n");
505+
const char *error_cstr = result_valobj_sp->GetError().AsCString();
506+
llvm::StringRef error(error_cstr);
507+
if (!error.empty()) {
508+
if (!error.starts_with("error:"))
509+
error_stream << "error: ";
510+
error_stream << error;
511+
if (!error.ends_with('\n'))
512+
error_stream.EOL();
513+
} else {
514+
error_stream << "error: unknown error\n";
515+
}
500516
}
501-
502517
result.SetStatus(eReturnStatusFailed);
503518
}
504519
}

‎lldb/source/Commands/DiagnosticRendering.h

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
//===-- DiagnosticRendering.h -----------------------------------*- C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLDB_SOURCE_COMMANDS_DIAGNOSTICRENDERING_H
10+
#define LLDB_SOURCE_COMMANDS_DIAGNOSTICRENDERING_H
11+
12+
#include "lldb/Expression/DiagnosticManager.h"
13+
#include "lldb/Utility/Stream.h"
14+
#include "llvm/Support/WithColor.h"
15+
16+
namespace lldb_private {
17+
18+
static llvm::raw_ostream &PrintSeverity(Stream &stream,
19+
lldb::Severity severity) {
20+
llvm::HighlightColor color;
21+
llvm::StringRef text;
22+
switch (severity) {
23+
case lldb::eSeverityError:
24+
color = llvm::HighlightColor::Error;
25+
text = "error: ";
26+
break;
27+
case lldb::eSeverityWarning:
28+
color = llvm::HighlightColor::Warning;
29+
text = "warning: ";
30+
break;
31+
case lldb::eSeverityInfo:
32+
color = llvm::HighlightColor::Remark;
33+
text = "note: ";
34+
break;
35+
}
36+
return llvm::WithColor(stream.AsRawOstream(), color, llvm::ColorMode::Enable)
37+
<< text;
38+
}
39+
40+
// Public for unittesting.
41+
static void RenderDiagnosticDetails(Stream &stream,
42+
std::optional<uint16_t> offset_in_command,
43+
bool show_inline,
44+
llvm::ArrayRef<DiagnosticDetail> details) {
45+
if (details.empty())
46+
return;
47+
48+
if (!offset_in_command) {
49+
for (const DiagnosticDetail &detail : details) {
50+
PrintSeverity(stream, detail.severity);
51+
stream << detail.rendered << '\n';
52+
}
53+
return;
54+
}
55+
56+
// Print a line with caret indicator(s) below the lldb prompt + command.
57+
const size_t padding = *offset_in_command;
58+
stream << std::string(padding, ' ');
59+
60+
size_t offset = 1;
61+
std::vector<DiagnosticDetail> remaining_details, other_details,
62+
hidden_details;
63+
for (const DiagnosticDetail &detail : details) {
64+
if (!show_inline || !detail.source_location) {
65+
other_details.push_back(detail);
66+
continue;
67+
}
68+
if (detail.source_location->hidden) {
69+
hidden_details.push_back(detail);
70+
continue;
71+
}
72+
if (!detail.source_location->in_user_input) {
73+
other_details.push_back(detail);
74+
continue;
75+
}
76+
77+
auto &loc = *detail.source_location;
78+
remaining_details.push_back(detail);
79+
if (offset > loc.column)
80+
continue;
81+
stream << std::string(loc.column - offset, ' ') << '^';
82+
if (loc.length > 1)
83+
stream << std::string(loc.length - 1, '~');
84+
offset = loc.column + 1;
85+
}
86+
stream << '\n';
87+
88+
// Work through each detail in reverse order using the vector/stack.
89+
bool did_print = false;
90+
for (auto detail = remaining_details.rbegin();
91+
detail != remaining_details.rend();
92+
++detail, remaining_details.pop_back()) {
93+
// Get the information to print this detail and remove it from the stack.
94+
// Print all the lines for all the other messages first.
95+
stream << std::string(padding, ' ');
96+
size_t offset = 1;
97+
for (auto &remaining_detail :
98+
llvm::ArrayRef(remaining_details).drop_back(1)) {
99+
uint16_t column = remaining_detail.source_location->column;
100+
stream << std::string(column - offset, ' ') << "│";
101+
offset = column + 1;
102+
}
103+
104+
// Print the line connecting the ^ with the error message.
105+
uint16_t column = detail->source_location->column;
106+
if (offset <= column)
107+
stream << std::string(column - offset, ' ') << "╰─ ";
108+
109+
// Print a colorized string based on the message's severity type.
110+
PrintSeverity(stream, detail->severity);
111+
112+
// Finally, print the message and start a new line.
113+
stream << detail->message << '\n';
114+
did_print = true;
115+
}
116+
117+
// Print the non-located details.
118+
for (const DiagnosticDetail &detail : other_details) {
119+
PrintSeverity(stream, detail.severity);
120+
stream << detail.rendered << '\n';
121+
did_print = true;
122+
}
123+
124+
// Print the hidden details as a last resort.
125+
if (!did_print)
126+
for (const DiagnosticDetail &detail : hidden_details) {
127+
PrintSeverity(stream, detail.severity);
128+
stream << detail.rendered << '\n';
129+
}
130+
}
131+
132+
} // namespace lldb_private
133+
#endif

‎lldb/source/Core/CoreProperties.td

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,4 +225,8 @@ let Definition = "debugger" in {
225225
DefaultEnumValue<"eDWIMPrintVerbosityNone">,
226226
EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
227227
Desc<"The verbosity level used by dwim-print.">;
228+
def ShowInlineDiagnostics: Property<"show-inline-diagnostics", "Boolean">,
229+
Global,
230+
DefaultFalse,
231+
Desc<"Controls whether diagnostics can refer directly to the command input, drawing arrows to it. If false, diagnostics will echo the input.">;
228232
}

‎lldb/source/Core/Debugger.cpp

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -592,7 +592,18 @@ lldb::DWIMPrintVerbosity Debugger::GetDWIMPrintVerbosity() const {
592592
const uint32_t idx = ePropertyDWIMPrintVerbosity;
593593
return GetPropertyAtIndexAs<lldb::DWIMPrintVerbosity>(
594594
idx, static_cast<lldb::DWIMPrintVerbosity>(
595-
g_debugger_properties[idx].default_uint_value));
595+
g_debugger_properties[idx].default_uint_value != 0));
596+
}
597+
598+
bool Debugger::GetShowInlineDiagnostics() const {
599+
const uint32_t idx = ePropertyShowInlineDiagnostics;
600+
return GetPropertyAtIndexAs<bool>(
601+
idx, g_debugger_properties[idx].default_uint_value);
602+
}
603+
604+
bool Debugger::SetShowInlineDiagnostics(bool b) {
605+
const uint32_t idx = ePropertyShowInlineDiagnostics;
606+
return SetPropertyAtIndex(idx, b);
596607
}
597608

598609
#pragma mark Debugger

‎lldb/source/Expression/DiagnosticManager.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ ExpressionError::ExpressionError(lldb::ExpressionResults result,
3737
: ErrorInfo(std::error_code(result, expression_category())), m_message(msg),
3838
m_details(details) {}
3939

40-
static const char *StringForSeverity(lldb::Severity severity) {
40+
static llvm::StringRef StringForSeverity(lldb::Severity severity) {
4141
switch (severity) {
4242
// this should be exhaustive
4343
case lldb::eSeverityError:

‎lldb/source/Interpreter/CommandInterpreter.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1887,7 +1887,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
18871887
CommandReturnObject &result,
18881888
bool force_repeat_command) {
18891889
std::string command_string(command_line);
1890-
std::string original_command_string(command_line);
1890+
std::string original_command_string(command_string);
1891+
std::string real_original_command_string(command_string);
18911892

18921893
Log *log = GetLog(LLDBLog::Commands);
18931894
llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
@@ -2076,6 +2077,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
20762077
}
20772078

20782079
ElapsedTime elapsed(execute_time);
2080+
cmd_obj->SetOriginalCommandString(real_original_command_string);
20792081
cmd_obj->Execute(remainder.c_str(), result);
20802082
}
20812083

‎lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -176,15 +176,22 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
176176
m_manager = manager;
177177
}
178178

179-
/// Returns the last ClangDiagnostic message that the DiagnosticManager
180-
/// received or a nullptr if the DiagnosticMangager hasn't seen any
181-
/// Clang diagnostics yet.
179+
/// Returns the last error ClangDiagnostic message that the
180+
/// DiagnosticManager received or a nullptr.
182181
ClangDiagnostic *MaybeGetLastClangDiag() const {
183182
if (m_manager->Diagnostics().empty())
184183
return nullptr;
185-
lldb_private::Diagnostic *diag = m_manager->Diagnostics().back().get();
186-
ClangDiagnostic *clang_diag = dyn_cast<ClangDiagnostic>(diag);
187-
return clang_diag;
184+
auto &diags = m_manager->Diagnostics();
185+
for (auto it = diags.rbegin(); it != diags.rend(); it++) {
186+
lldb_private::Diagnostic *diag = it->get();
187+
if (ClangDiagnostic *clang_diag = dyn_cast<ClangDiagnostic>(diag)) {
188+
if (clang_diag->GetSeverity() == lldb::eSeverityWarning)
189+
return nullptr;
190+
if (clang_diag->GetSeverity() == lldb::eSeverityError)
191+
return clang_diag;
192+
}
193+
}
194+
return nullptr;
188195
}
189196

190197
void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
@@ -214,8 +221,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
214221
m_passthrough->HandleDiagnostic(DiagLevel, Info);
215222

216223
DiagnosticDetail detail;
217-
bool make_new_diagnostic = true;
218-
219224
switch (DiagLevel) {
220225
case DiagnosticsEngine::Level::Fatal:
221226
case DiagnosticsEngine::Level::Error:
@@ -229,9 +234,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
229234
detail.severity = lldb::eSeverityInfo;
230235
break;
231236
case DiagnosticsEngine::Level::Note:
232-
m_manager->AppendMessageToDiagnostic(m_output);
233-
make_new_diagnostic = false;
234-
235237
// 'note:' diagnostics for errors and warnings can also contain Fix-Its.
236238
// We add these Fix-Its to the last error diagnostic to make sure
237239
// that we later have all Fix-Its related to an 'error' diagnostic when
@@ -249,7 +251,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
249251
AddAllFixIts(clang_diag, Info);
250252
break;
251253
}
252-
if (make_new_diagnostic) {
253254
// ClangDiagnostic messages are expected to have no whitespace/newlines
254255
// around them.
255256
std::string stripped_output =
@@ -269,6 +270,7 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
269270
loc.line = fsloc.getSpellingLineNumber();
270271
loc.column = fsloc.getSpellingColumnNumber();
271272
loc.in_user_input = filename == m_filename;
273+
loc.hidden = filename.starts_with("<lldb wrapper ");
272274

273275
// Find the range of the primary location.
274276
for (const auto &range : Info.getRanges()) {
@@ -298,7 +300,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
298300
AddAllFixIts(new_diagnostic.get(), Info);
299301

300302
m_manager->AddDiagnostic(std::move(new_diagnostic));
301-
}
302303
}
303304

304305
void BeginSourceFile(const LangOptions &LO, const Preprocessor *PP) override {

‎lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class CommandObjectThreadTraceExportCTF : public CommandObjectParsed {
4848
Options *GetOptions() override { return &m_options; }
4949

5050
protected:
51-
void DoExecute(Args &command, CommandReturnObject &result) override;
51+
void DoExecute(Args &args, CommandReturnObject &result) override;
5252

5353
CommandOptions m_options;
5454
};

0 commit comments

Comments
 (0)