Skip to content

Commit 81eba0a

Browse files
adrian-prantladrian-prantl
committed
[lldb] Inline expression evaluator error visualization
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.
1 parent 770bfeb commit 81eba0a

File tree

17 files changed

+220
-42
lines changed

17 files changed

+220
-42
lines changed

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,18 @@ struct DiagnosticDetail {
5454
/// An llvm::Error used to communicate diagnostics in Status. Multiple
5555
/// diagnostics may be chained in an llvm::ErrorList.
5656
class DetailedExpressionError
57-
: public llvm::ErrorInfo<DetailedExpressionError, llvm::ECError> {
57+
: public llvm::ErrorInfo<DetailedExpressionError, CloneableError> {
5858
DiagnosticDetail m_detail;
5959

6060
public:
61-
using llvm::ErrorInfo<DetailedExpressionError, llvm::ECError>::ErrorInfo;
61+
using llvm::ErrorInfo<DetailedExpressionError, CloneableError>::ErrorInfo;
6262
DetailedExpressionError(DiagnosticDetail detail) : m_detail(detail) {}
6363
std::string message() const override;
64+
DiagnosticDetail GetDetail() const { return m_detail; }
65+
std::error_code convertToErrorCode() const override;
66+
void log(llvm::raw_ostream &OS) const override;
67+
std::unique_ptr<CloneableError> Clone() const override;
68+
6469
static char ID;
6570
};
6671

‎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/Commands/CommandObjectExpression.cpp

Lines changed: 129 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
#include "CommandObjectExpression.h"
1212
#include "lldb/Core/Debugger.h"
13+
#include "lldb/Expression/DiagnosticManager.h"
1314
#include "lldb/Expression/ExpressionVariable.h"
1415
#include "lldb/Expression/REPL.h"
1516
#include "lldb/Expression/UserExpression.h"
@@ -398,6 +399,104 @@ CanBeUsedForElementCountPrinting(ValueObject &valobj) {
398399
return Status();
399400
}
400401

402+
static llvm::raw_ostream &PrintSeverity(Stream &stream,
403+
lldb::Severity severity) {
404+
llvm::HighlightColor color;
405+
llvm::StringRef text;
406+
switch (severity) {
407+
case eSeverityError:
408+
color = llvm::HighlightColor::Error;
409+
text = "error: ";
410+
break;
411+
case eSeverityWarning:
412+
color = llvm::HighlightColor::Warning;
413+
text = "warning: ";
414+
break;
415+
case eSeverityInfo:
416+
color = llvm::HighlightColor::Remark;
417+
text = "note: ";
418+
break;
419+
}
420+
return llvm::WithColor(stream.AsRawOstream(), color, llvm::ColorMode::Enable)
421+
<< text;
422+
}
423+
424+
namespace lldb_private {
425+
// Public for unittesting.
426+
std::string RenderDiagnosticDetails(std::optional<uint16_t> offset_in_command,
427+
llvm::ArrayRef<DiagnosticDetail> details) {
428+
if (details.empty())
429+
return {};
430+
431+
StreamString stream(true);
432+
if (!offset_in_command) {
433+
for (const DiagnosticDetail &detail : details) {
434+
PrintSeverity(stream, detail.severity);
435+
stream << detail.rendered << '\n';
436+
}
437+
return stream.GetData();
438+
}
439+
440+
// Print a line with caret indicator(s) below the lldb prompt + command.
441+
const size_t padding = *offset_in_command;
442+
stream << std::string(padding, ' ');
443+
444+
size_t offset = 1;
445+
std::vector<DiagnosticDetail> remaining_details, other_details;
446+
for (const DiagnosticDetail &detail : details) {
447+
if (!detail.source_location) {
448+
other_details.push_back(detail);
449+
continue;
450+
}
451+
452+
auto &loc = *detail.source_location;
453+
remaining_details.push_back(detail);
454+
if (offset > loc.column)
455+
continue;
456+
stream << std::string(loc.column - offset, ' ') << '^';
457+
if (loc.length > 1)
458+
stream << std::string(loc.length - 1, '~');
459+
offset = loc.column + 1;
460+
}
461+
stream << '\n';
462+
463+
// Work through each detail in reverse order using the vector/stack.
464+
for (auto detail = remaining_details.rbegin();
465+
detail != remaining_details.rend();
466+
++detail, remaining_details.pop_back()) {
467+
// Get the information to print this detail and remove it from the stack.
468+
// Print all the lines for all the other messages first.
469+
stream << std::string(padding, ' ');
470+
size_t offset = 1;
471+
for (auto &remaining_detail :
472+
llvm::ArrayRef(remaining_details).drop_back(1)) {
473+
uint16_t column = remaining_detail.source_location->column;
474+
stream << std::string(column - offset, ' ') << "│";
475+
offset = column + 1;
476+
}
477+
478+
// Print the line connecting the ^ with the error message.
479+
uint16_t column = detail->source_location->column;
480+
if (offset <= column)
481+
stream << std::string(column - offset, ' ') << "╰─ ";
482+
483+
// Print a colorized string based on the message's severity type.
484+
PrintSeverity(stream, detail->severity);
485+
486+
// Finally, print the message and start a new line.
487+
stream << detail->message << '\n';
488+
}
489+
490+
// Print the non-located details.
491+
for (const DiagnosticDetail &detail : other_details) {
492+
PrintSeverity(stream, detail.severity);
493+
stream << detail.message << '\n';
494+
}
495+
496+
return stream.GetData();
497+
}
498+
} // namespace lldb_private
499+
401500
bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
402501
Stream &output_stream,
403502
Stream &error_stream,
@@ -486,19 +585,37 @@ bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
486585

487586
result.SetStatus(eReturnStatusSuccessFinishResult);
488587
} 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();
498-
} else {
499-
error_stream.PutCString("error: unknown error\n");
588+
// Retrieve the diagnostics.
589+
std::vector<DiagnosticDetail> details;
590+
llvm::consumeError(
591+
llvm::handleErrors(result_valobj_sp->GetError().ToError(),
592+
[&](DetailedExpressionError &error) {
593+
details.push_back(error.GetDetail());
594+
}));
595+
// Find the position of the expression in the command.
596+
std::optional<uint16_t> expr_pos;
597+
size_t nchar = m_original_command.find(expr);
598+
if (nchar != std::string::npos)
599+
expr_pos = nchar + GetDebugger().GetPrompt().size();
600+
601+
std::string error = RenderDiagnosticDetails(expr_pos, details);
602+
if (!error.empty())
603+
error_stream.PutCString(error.c_str());
604+
else {
605+
const char *error_cstr = result_valobj_sp->GetError().AsCString();
606+
if (error_cstr && error_cstr[0]) {
607+
const size_t error_cstr_len = strlen(error_cstr);
608+
const bool ends_with_newline =
609+
error_cstr[error_cstr_len - 1] == '\n';
610+
if (strstr(error_cstr, "error:") != error_cstr)
611+
error_stream.PutCString("error: ");
612+
error_stream.Write(error_cstr, error_cstr_len);
613+
if (!ends_with_newline)
614+
error_stream.EOL();
615+
} else {
616+
error_stream.PutCString("error: unknown error\n");
617+
}
500618
}
501-
502619
result.SetStatus(eReturnStatusFailed);
503620
}
504621
}

‎lldb/source/Expression/DiagnosticManager.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,18 @@ std::string DetailedExpressionError::message() const {
3636
return str;
3737
}
3838

39+
std::error_code DetailedExpressionError::convertToErrorCode() const {
40+
return llvm::inconvertibleErrorCode();
41+
}
42+
43+
void DetailedExpressionError::log(llvm::raw_ostream &OS) const {
44+
OS << message();
45+
}
46+
47+
std::unique_ptr<CloneableError> DetailedExpressionError::Clone() const {
48+
return std::make_unique<DetailedExpressionError>(m_detail);
49+
}
50+
3951
std::string DiagnosticManager::GetString(char separator) {
4052
std::string str;
4153
llvm::raw_string_ostream stream(str);

‎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/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
};

‎lldb/source/Utility/Status.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include "lldb/Utility/Status.h"
1010

11+
#include "lldb/Expression/DiagnosticManager.h"
1112
#include "lldb/Utility/LLDBLog.h"
1213
#include "lldb/Utility/Log.h"
1314
#include "lldb/Utility/VASPrintf.h"
@@ -275,8 +276,6 @@ ErrorType Status::GetType() const {
275276
else if (error.convertToErrorCode().category() == lldb_generic_category() ||
276277
error.convertToErrorCode() == llvm::inconvertibleErrorCode())
277278
result = eErrorTypeGeneric;
278-
else
279-
result = eErrorTypeInvalid;
280279
});
281280
return result;
282281
}

‎lldb/test/API/commands/expression/persistent_variables/TestPersistentVariables.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ def test_persistent_variables(self):
5656
self.expect(
5757
"expr int $i = 123",
5858
error=True,
59-
substrs=["error: redefinition of persistent variable '$i'"],
59+
substrs=["redefinition of persistent variable '$i'"],
6060
)
6161
self.expect_expr("$i", result_type="int", result_value="5")
6262

@@ -65,7 +65,7 @@ def test_persistent_variables(self):
6565
self.expect(
6666
"expr long $i = 123",
6767
error=True,
68-
substrs=["error: redefinition of persistent variable '$i'"],
68+
substrs=["redefinition of persistent variable '$i'"],
6969
)
7070
self.expect_expr("$i", result_type="int", result_value="5")
7171

‎lldb/test/API/commands/expression/static-initializers/TestStaticInitializers.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ def test_failing_init(self):
3535
self.expect(
3636
"expr -p -- struct Foo2 { Foo2() { do_abort(); } }; Foo2 f;",
3737
error=True,
38-
substrs=["error: couldn't run static initializer:"],
38+
substrs=["couldn't run static initializer:"],
3939
)
4040

4141
def test_without_process(self):

‎lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ def do_test_template_function(self, add_cast):
2121
"expr b1 <=> b2",
2222
error=True,
2323
substrs=[
24-
"warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior"
24+
"warning:", "'<=>' is a single token in C++20; add a space to avoid a change in behavior"
2525
],
2626
)
2727

‎lldb/test/API/lang/mixed/TestMixedLanguages.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ def cleanup():
4040
self.runCmd("run")
4141
self.expect("thread backtrace", substrs=["`main", "lang=c"])
4242
# Make sure evaluation of C++11 fails.
43-
self.expect("expr foo != nullptr", error=True, startstr="error")
43+
self.expect("expr foo != nullptr", error=True, substrs=["error"])
4444

4545
# Run to BP at foo (in foo.cpp) and test that the language is C++.
4646
self.runCmd("breakpoint set -n foo")

‎lldb/test/Shell/Expr/TestObjCIDCast.test

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,4 @@
66
// RUN: 2>&1 | FileCheck %s
77

88
// CHECK: (lldb) expression --language objc -- *(id)0x1
9-
// CHECK: error: Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory
9+
// CHECK: error:{{.*}}Couldn't apply expression side effects : Couldn't dematerialize a result variable: couldn't read its memory

‎lldb/test/Shell/Expr/TestObjCInCXXContext.test

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,4 @@
1818
// CHECK-NEXT: (NSString *){{.*}}= nil
1919

2020
// CHECK: (lldb) expression NSString
21-
// CHECK-NEXT: error:{{.*}} use of undeclared identifier 'NSString'
21+
// CHECK: error:{{.*}}use of undeclared identifier 'NSString'

‎lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@
1313
// CHECK: (lldb) expression d
1414
// CHECK: (D) $1 = {}
1515
// CHECK: (lldb) expression static_e_ref
16-
// CHECK: error: {{.*}}incomplete type 'E' where a complete type is required
17-
// CHECK: static_e_ref
18-
// CHECK: ^
16+
// CHECK: error:{{.*}}incomplete type 'E' where a complete type is required
1917

2018
// Complete base class.
2119
struct A { int x; A(); };

0 commit comments

Comments
 (0)