Skip to content

Commit 8a50c39

Browse files
adrian-prantladrian-prantl
committed
[lldb] Inline expression evaluator error visualization (llvm#106470)
This patch is a reworking of Pete Lawrence's (@PortalPete) proposal for better expression evaluator error messages: llvm#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 llvm#106442. (cherry picked from commit d33fa70)
1 parent 9cdeec4 commit 8a50c39

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
@@ -368,6 +368,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
368368

369369
const std::string &GetInstanceName() { return m_instance_name; }
370370

371+
bool GetShowInlineDiagnostics() const;
372+
373+
bool SetShowInlineDiagnostics(bool);
374+
371375
bool LoadPlugin(const FileSpec &spec, Status &error);
372376

373377
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
@@ -1485,6 +1485,12 @@ bool SBDebugger::GetUseColor() const {
14851485
return (m_opaque_sp ? m_opaque_sp->GetUseColor() : false);
14861486
}
14871487

1488+
bool SBDebugger::SetShowInlineDiagnostics(bool value) {
1489+
LLDB_INSTRUMENT_VA(this, value);
1490+
1491+
return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
1492+
}
1493+
14881494
bool SBDebugger::SetUseSourceCache(bool value) {
14891495
LLDB_INSTRUMENT_VA(this, value);
14901496

‎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"
@@ -510,19 +510,34 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
510510

511511
result.SetStatus(eReturnStatusSuccessFinishResult);
512512
} else {
513-
const char *error_cstr = result_valobj_sp->GetError().AsCString();
514-
if (error_cstr && error_cstr[0]) {
515-
const size_t error_cstr_len = strlen(error_cstr);
516-
const bool ends_with_newline = error_cstr[error_cstr_len - 1] == '\n';
517-
if (strstr(error_cstr, "error:") != error_cstr)
518-
error_stream.PutCString("error: ");
519-
error_stream.Write(error_cstr, error_cstr_len);
520-
if (!ends_with_newline)
521-
error_stream.EOL();
513+
// Retrieve the diagnostics.
514+
std::vector<DiagnosticDetail> details;
515+
llvm::consumeError(llvm::handleErrors(
516+
result_valobj_sp->GetError().ToError(),
517+
[&](ExpressionError &error) { details = error.GetDetails(); }));
518+
// Find the position of the expression in the command.
519+
std::optional<uint16_t> expr_pos;
520+
size_t nchar = m_original_command.find(expr);
521+
if (nchar != std::string::npos)
522+
expr_pos = nchar + GetDebugger().GetPrompt().size();
523+
524+
if (!details.empty()) {
525+
bool show_inline =
526+
GetDebugger().GetShowInlineDiagnostics() && !expr.contains('\n');
527+
RenderDiagnosticDetails(error_stream, expr_pos, show_inline, details);
522528
} else {
523-
error_stream.PutCString("error: unknown error\n");
529+
const char *error_cstr = result_valobj_sp->GetError().AsCString();
530+
llvm::StringRef error(error_cstr);
531+
if (!error.empty()) {
532+
if (!error.starts_with("error:"))
533+
error_stream << "error: ";
534+
error_stream << error;
535+
if (!error.ends_with('\n'))
536+
error_stream.EOL();
537+
} else {
538+
error_stream << "error: unknown error\n";
539+
}
524540
}
525-
526541
result.SetStatus(eReturnStatusFailed);
527542
}
528543
}

‎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
@@ -270,4 +270,8 @@ let Definition = "debugger" in {
270270
DefaultEnumValue<"eDWIMPrintVerbosityNone">,
271271
EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
272272
Desc<"The verbosity level used by dwim-print.">;
273+
def ShowInlineDiagnostics: Property<"show-inline-diagnostics", "Boolean">,
274+
Global,
275+
DefaultFalse,
276+
Desc<"Controls whether diagnostics can refer directly to the command input, drawing arrows to it. If false, diagnostics will echo the input.">;
273277
}

‎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
@@ -1899,7 +1899,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
18991899
CommandReturnObject &result,
19001900
bool force_repeat_command) {
19011901
std::string command_string(command_line);
1902-
std::string original_command_string(command_line);
1902+
std::string original_command_string(command_string);
1903+
std::string real_original_command_string(command_string);
19031904

19041905
Log *log = GetLog(LLDBLog::Commands);
19051906
llvm::PrettyStackTraceFormat stack_trace("HandleCommand(command = \"%s\")",
@@ -2088,6 +2089,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
20882089
}
20892090

20902091
ElapsedTime elapsed(execute_time);
2092+
cmd_obj->SetOriginalCommandString(real_original_command_string);
20912093
cmd_obj->Execute(remainder.c_str(), result);
20922094
}
20932095

‎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,
@@ -215,8 +222,6 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
215222
m_os->flush();
216223

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

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

301303
m_manager->AddDiagnostic(std::move(new_diagnostic));
302-
}
303304
}
304305

305306
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)