[lldb] Inline expression evaluator error visualization #106470
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Adrian Prantl (adrian-prantl) ChangesThis patch is a reworking of Pete Lawrence's (@PortalPete) proposal Before: After: This eliminates the confusing Depends on #106442. Patch is 150.30 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106470.diff 70 Files Affected:
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/Interpreter/CommandAlias.h b/lldb/include/lldb/Interpreter/CommandAlias.h
index 7b59ea0a74bb9e..778d656a845506 100644
--- a/lldb/include/lldb/Interpreter/CommandAlias.h
+++ b/lldb/include/lldb/Interpreter/CommandAlias.h
@@ -56,7 +56,7 @@ class CommandAlias : public CommandObject {
void SetHelpLong(llvm::StringRef str) override;
- void Execute(const char *args_string, CommandReturnObject &result) override;
+ void Execute(const char *args_string, std::optional<uint16_t> offset_in_command,CommandReturnObject &result) override;
lldb::CommandObjectSP GetUnderlyingCommand() {
return m_underlying_command_sp;
diff --git a/lldb/include/lldb/Interpreter/CommandObject.h b/lldb/include/lldb/Interpreter/CommandObject.h
index 20c4769af90338..9319b15f1be4dc 100644
--- a/lldb/include/lldb/Interpreter/CommandObject.h
+++ b/lldb/include/lldb/Interpreter/CommandObject.h
@@ -340,7 +340,11 @@ class CommandObject : public std::enable_shared_from_this<CommandObject> {
return false;
}
+ /// \param offset_in_command is on what column \c args_string
+ /// appears, if applicable. This enables diagnostics that refer back
+ /// to the user input.
virtual void Execute(const char *args_string,
+ std::optional<uint16_t> offset_in_command,
CommandReturnObject &result) = 0;
protected:
@@ -421,10 +425,14 @@ class CommandObjectParsed : public CommandObject {
~CommandObjectParsed() override = default;
- void Execute(const char *args_string, CommandReturnObject &result) override;
+ void Execute(const char *args_string,
+ std::optional<uint16_t> offset_in_command,
+ CommandReturnObject &result) override;
protected:
- virtual void DoExecute(Args &command, CommandReturnObject &result) = 0;
+ virtual void DoExecute(Args &command,
+ std::optional<uint16_t> offset_in_command,
+ CommandReturnObject &result) = 0;
bool WantsRawCommandString() override { return false; }
};
@@ -438,10 +446,13 @@ class CommandObjectRaw : public CommandObject {
~CommandObjectRaw() override = default;
- void Execute(const char *args_string, CommandReturnObject &result) override;
+ void Execute(const char *args_string,
+ std::optional<uint16_t> offset_in_command,
+ CommandReturnObject &result) override;
protected:
virtual void DoExecute(llvm::StringRef command,
+ std::optional<uint16_t> offset_in_command,
CommandReturnObject &result) = 0;
bool WantsRawCommandString() override { return true; }
diff --git a/lldb/include/lldb/Interpreter/CommandObjectMultiword.h b/lldb/include/lldb/Interpreter/CommandObjectMultiword.h
index bceb7f0e51edb6..6f7c798dfeae69 100644
--- a/lldb/include/lldb/Interpreter/CommandObjectMultiword.h
+++ b/lldb/include/lldb/Interpreter/CommandObjectMultiword.h
@@ -59,7 +59,9 @@ class CommandObjectMultiword : public CommandObject {
std::optional<std::string> GetRepeatCommand(Args ¤t_command_args,
uint32_t index) override;
- void Execute(const char *args_string, CommandReturnObject &result) override;
+ void Execute(const char *args_string,
+ std::optional<uint16_t> offset_in_command,
+ CommandReturnObject &result) override;
bool IsRemovable() const override { return m_can_be_removed; }
@@ -129,7 +131,9 @@ class CommandObjectProxy : public CommandObject {
/// Execute is called) and \a GetProxyCommandObject returned null.
virtual llvm::StringRef GetUnsupportedError();
- void Execute(const char *args_string, CommandReturnObject &result) override;
+ void Execute(const char *args_string,
+ std::optional<uint16_t> offset_in_command,
+ CommandReturnObject &result) override;
protected:
// These two want to iterate over the subcommand dictionary.
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/API/SBCommandInterpreter.cpp b/lldb/source/API/SBCommandInterpreter.cpp
index 7a35473283684c..d9d769456ae594 100644
--- a/lldb/source/API/SBCommandInterpreter.cpp
+++ b/lldb/source/API/SBCommandInterpreter.cpp
@@ -71,7 +71,7 @@ class CommandPluginInterfaceImplementation : public CommandObjectParsed {
}
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
SBCommandReturnObject sb_return(result);
SBCommandInterpreter sb_interpreter(&m_interpreter);
SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
diff --git a/lldb/source/Commands/CommandObjectApropos.cpp b/lldb/source/Commands/CommandObjectApropos.cpp
index d663f2bd923fe2..13d1225f83fe6e 100644
--- a/lldb/source/Commands/CommandObjectApropos.cpp
+++ b/lldb/source/Commands/CommandObjectApropos.cpp
@@ -26,7 +26,7 @@ CommandObjectApropos::CommandObjectApropos(CommandInterpreter &interpreter)
CommandObjectApropos::~CommandObjectApropos() = default;
-void CommandObjectApropos::DoExecute(Args &args, CommandReturnObject &result) {
+void CommandObjectApropos::DoExecute(Args &args, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) {
const size_t argc = args.GetArgumentCount();
if (argc == 1) {
diff --git a/lldb/source/Commands/CommandObjectApropos.h b/lldb/source/Commands/CommandObjectApropos.h
index f43420c1815d90..da83701cb15a7b 100644
--- a/lldb/source/Commands/CommandObjectApropos.h
+++ b/lldb/source/Commands/CommandObjectApropos.h
@@ -23,7 +23,7 @@ class CommandObjectApropos : public CommandObjectParsed {
~CommandObjectApropos() override;
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override;
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override;
};
} // namespace lldb_private
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index abde27b2b53ad8..46aad494e27a4e 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -538,7 +538,7 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &args, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target =
m_dummy_options.m_use_dummy ? GetDummyTarget() : GetTarget();
@@ -839,7 +839,7 @@ class CommandObjectBreakpointModify : public CommandObjectParsed {
Options *GetOptions() override { return &m_options; }
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = m_dummy_opts.m_use_dummy ? GetDummyTarget() : GetTarget();
std::unique_lock<std::recursive_mutex> lock;
@@ -903,7 +903,7 @@ class CommandObjectBreakpointEnable : public CommandObjectParsed {
}
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
@@ -1010,7 +1010,7 @@ the second re-enables the first location.");
}
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
target.GetBreakpointList().GetListMutex(lock);
@@ -1148,7 +1148,7 @@ class CommandObjectBreakpointList : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = m_options.m_use_dummy ? GetDummyTarget() : GetTarget();
const BreakpointList &breakpoints =
@@ -1267,7 +1267,7 @@ class CommandObjectBreakpointClear : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = GetTarget();
// The following are the various types of breakpoints that could be
@@ -1416,7 +1416,7 @@ class CommandObjectBreakpointDelete : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = m_options.m_use_dummy ? GetDummyTarget() : GetTarget();
result.Clear();
@@ -1669,7 +1669,7 @@ class CommandObjectBreakpointNameConfigure : public CommandObjectParsed {
Options *GetOptions() override { return &m_option_group; }
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
const size_t argc = command.GetArgumentCount();
if (argc == 0) {
@@ -1758,7 +1758,7 @@ class CommandObjectBreakpointNameAdd : public CommandObjectParsed {
Options *GetOptions() override { return &m_option_group; }
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
if (!m_name_options.m_name.OptionWasSet()) {
result.AppendError("No name option provided.");
return;
@@ -1832,7 +1832,7 @@ class CommandObjectBreakpointNameDelete : public CommandObjectParsed {
Options *GetOptions() override { return &m_option_group; }
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
if (!m_name_options.m_name.OptionWasSet()) {
result.AppendError("No name option provided.");
return;
@@ -1896,7 +1896,7 @@ class CommandObjectBreakpointNameList : public CommandObjectParsed {
Options *GetOptions() override { return &m_option_group; }
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target =
m_name_options.m_use_dummy ? GetDummyTarget() : GetTarget();
@@ -2209,7 +2209,7 @@ class CommandObjectBreakpointRead : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
@@ -2319,7 +2319,7 @@ class CommandObjectBreakpointWrite : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = GetTarget();
std::unique_lock<std::recursive_mutex> lock;
diff --git a/lldb/source/Commands/CommandObjectBreakpointCommand.cpp b/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
index b668cd0f7c22f0..08b72bc26c3b75 100644
--- a/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
@@ -319,9 +319,8 @@ are no syntax errors may indicate that a function was declared but never called.
bool m_stop_on_error;
bool m_use_dummy;
};
-
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = m_options.m_use_dummy ? GetDummyTarget() : GetTarget();
const BreakpointList &breakpoints = target.GetBreakpointList();
@@ -479,7 +478,7 @@ class CommandObjectBreakpointCommandDelete : public CommandObjectParsed {
};
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = m_options.m_use_dummy ? GetDummyTarget() : GetTarget();
const BreakpointList &breakpoints = target.GetBreakpointList();
@@ -546,7 +545,7 @@ class CommandObjectBreakpointCommandList : public CommandObjectParsed {
~CommandObjectBreakpointCommandList() override = default;
protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
+ void DoExecute(Args &command, std::optional<uint16_t> offset_in_command, CommandReturnObject &result) override {
Target &target = GetTarget();
const BreakpointList &breakpoints = target.GetBreakpointList();
diff --git a/lldb/source/Commands/CommandObjectCommands.cpp b/lldb/source/Commands/CommandObjectCommands.cpp
index f8f2b97eb898fa..961bae2dd1d55b 100644
--- a/lldb/source/Commands/CommandO...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
b024714 to
2e7f046
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
2e7f046 to
697e5e6
Compare
medismailben
requested changes
Aug 29, 2024
|
This seems like it could be problematic for IDEs, if they don't print the error in the same window as the expression being evaluated. The arrows could end up pointing to nowhere, or to the wrong place in the expression (if we don't get the right offset). I think this could be made simpler and more robust by just printing a reproduction of the expression as the first line of the error message. It saving that one line output worth it? Also, how will this behave for multiline expressions? Or with errors that refer to multiple source locations (e.g inside macro expansions)? |
Eventually I want IDEs to get access to the same kind of machine-readable diagnostics, so they can format errors however they like (or, if they aren't interested, dump the pre-rendered textual error that is still available).
Yes. It's not about saving one line of output, but for removing the cognitive load of having to re-read the expression, shifted by a couple of characters and correlating that with what you just typed, so you know where to make an edit.
I haven't wired that up correctly yet (thanks for reminding!), but the idea is that a multi-line expression would get the prerendered diagnostic as previously. |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
697e5e6 to
bc83005
Compare
|
Rebased on top of #106442! |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
bc83005 to
6691c47
Compare
|
Ah, I thought you were echoing something and pointing into that, I missed that you were pointing into the original command line. Does this also work if I have an alias that scrambles the command arguments compared to how they are in the command that eventually got invoked.
If we're going to do errors this way, we should change the argument/option value setting to recognize which slot a bad option argument/option value came from (or unrecognized option flag for that matter) and point at those as well...
Jim
… On Aug 30, 2024, at 6:39 PM, Adrian Prantl ***@***.***> wrote:
@adrian-prantl commented on this pull request.
In lldb/source/Interpreter/CommandInterpreter.cpp <#106470 (comment)>:
> @@ -2076,7 +2077,11 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
}
ElapsedTime elapsed(execute_time);
- cmd_obj->Execute(remainder.c_str(), result);
+ size_t nchar = real_original_command_string.find(remainder);
+ std::optional<uint16_t> pos_in_cmd;
+ if (nchar != std::string::npos)
+ pos_in_cmd = nchar + GetDebugger().GetPrompt().size();
+ cmd_obj->Execute(remainder.c_str(), pos_in_cmd, result);
@jimingham <https://github.com/jimingham> @medismailben <https://github.com/medismailben> I hopefully have resolved both of your concerns by storing the original command string as a member in CommandObject. This way the expr -i 0 -u 0 -- not a valid expression works.
(lldb) expr -i 0 -u 0 -- not a valid expression
^
╰─ error: use of undeclared identifier 'not'
—
Reply to this email directly, view it on GitHub <#106470 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW5GTQTALYWMAUC3ZITZUENEJAVCNFSM6AAAAABNJIEXUGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZTG4YTCNBUGY>.
You are receiving this because you were mentioned.
|
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
2 times, most recently
from
81eba0a to
2c7ff3f
Compare
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
2c7ff3f to
f84f266
Compare
|
This is now ready for review. Out of scope for this PR, but the obvious next step: |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
2 times, most recently
from
7b7c6cf to
6c8e716
Compare
|
@medismailben @labath Would you mind taking another look at this? |
medismailben
approved these changes
Sep 24, 2024
I'm still worried about this. Yes, the IDEs can dump the pre-rendered error (and right now, that's all they can do), but this rendering assumes that the error message will be printed in a particular way (right under the original command, which will include the prompt and everything). I think that's fine for the commands entered through the LLDB command line, but I don't think it's reasonable to expect that every caller of (sure we can fix this so that the output makes sense, but I wonder how many other such callers are out there) |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
6c8e716 to
73b9ae3
Compare
Just to explain my motivation., in the end what I'm most interested in is exposing the machine-readable diagnostics through the SBAPI, so that is where this is going next. |
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
2 times, most recently
from
bad95e9 to
a51d8a6
Compare
|
Done. @labath, you can now opt into the inline diagnostics with |
labath
approved these changes
Sep 27, 2024
There was a problem hiding this comment.
The setting might be useful as well, though I think it would be even better if this was settable on a per-invocation basis (an argument to HandleCommand or something) -- the reason being that someone might still want to have this feature for commands which are input through the console (or from contexts where you can guarantee their placement), but not for commands which are executed e.g. from scripts. Here's one example:
(lldb) script lldb.debugger.HandleCommand('expression -- ""+2.5')
error: <user expression 0>:1:3: invalid operands to binary expression ('const char[1]' and 'double')
1 | ""+2.5
| ~~^~~~
You definitely won't point to the right place there. It's convoluted, I know, but it's not that unsimilar from I do when debugging lldb, where I sometimes run things like self.dbg.HandleCommand from the pdb prompt.
However this is workaroundable by changing the setting, so I don't want to hold this up further.
| @@ -592,7 +592,18 @@ lldb::DWIMPrintVerbosity Debugger::GetDWIMPrintVerbosity() const { | |||
| const uint32_t idx = ePropertyDWIMPrintVerbosity; | |||
| return GetPropertyAtIndexAs<lldb::DWIMPrintVerbosity>( | |||
| idx, static_cast<lldb::DWIMPrintVerbosity>( | |||
| g_debugger_properties[idx].default_uint_value)); | |||
| g_debugger_properties[idx].default_uint_value != 0)); | |||
There was a problem hiding this comment.
Did you intend to change DWIM code?
It has both. lldb-dap does this weird heuristic console multiplexing, where it guesses whether something should be treated like an expression (I guess that's because other VSCode debuggers do that), which is executed through EvaluateExpression, or an lldb command, which is executed through HandleCommand. So it's not very likely that a user will be running a "command" to evaluate an expression, but I think it still illustrates my point that HandleCommand can be (and probably is) invoked in contexts where one cannot guarantee the alignment of the output. |
|
This is a slightly orthogonal point, but I don't think we should confine this error visualization to just expressions. If the user passes a non-boolean string to an option in a command, it would be helpful to point at the option for them, much as pointing to the part of the expression that doesn't parse is useful. And if we're pointing at errors in expressions, it looks kind of odd that we don't do the same thing for other commands.
I'm not suggesting that that be part of this work, but when thinking about when this kind of thing would be appropriate I think would be unfortunate if we didn't allow the same feature at least for parsed commands.
Jim
… On Sep 27, 2024, at 2:00 AM, Pavel Labath ***@***.***> wrote:
I'm actually surprised that lldb-dap sees the new diagnostics because I thought it would run the expression through a lower-level API like SBFrame::EvaluateExpression(). But I guess it just provides a "terminal" window that passes everything through HandleCommand. To support this better, I'm thinking about adding a setting to opt into the inline diagnostics that only lldb sets, so we don't get unexpected results like this.
It has both. lldb-dap does this weird heuristic console multiplexing, where it guesses whether something should be treated like an expression (I guess that's because other VSCode debuggers do that), which is executed through EvaluateExpression, or an lldb command, which is executed through HandleCommand. So it's not very likely that a user will be running a "command" to evaluate an expression, but I think it still illustrates my point that HandleCommand can be (and probably is) invoked in contexts where one cannot guarantee the alignment of the output.
—
Reply to this email directly, view it on GitHub <#106470 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3LWDCV5NKNNMDEQOTZYUNELAVCNFSM6AAAAABNJIEXUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZYG44TQMZTGU>.
You are receiving this because you were mentioned.
|
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
a51d8a6 to
eee1fc7
Compare
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.
adrian-prantl
force-pushed
the
expression-diagnostics-2
branch
from
eee1fc7 to
4f57669
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/4599 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/5739 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/7343 Here is the relevant piece of the build log for the reference |
adrian-prantl
added a commit
that referenced
this pull request
Sep 28, 2024
…)" This reverts commit 49372d1.
adrian-prantl
added a commit
that referenced
this pull request
Sep 28, 2024
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.
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Oct 16, 2024
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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch is a reworking of Pete Lawrence's (@PortalPete) proposal
for better expression evaluator error messages:
#80938
Before:
After:
This eliminates the confusing
<user expression 0>:1:3sourcelocation 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.