[lldb] Support CommandInterpreter print callbacks #125006
Conversation
JDevlieghere
requested review from
DavidSpickett,
adrian-prantl,
labath and
jimingham
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesXcode uses a pseudoterminal for the debugger console.
This patch adds support for registering a callback in the command interpreter that gives access to the We considered a few other alternatives to solve this problem:
rdar://141254310 Patch is 25.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/125006.diff 17 Files Affected:
diff --git a/lldb/bindings/python/python-swigsafecast.swig b/lldb/bindings/python/python-swigsafecast.swig
index 429baad158ca5d..4721dfdc17e6a0 100644
--- a/lldb/bindings/python/python-swigsafecast.swig
+++ b/lldb/bindings/python/python-swigsafecast.swig
@@ -9,6 +9,10 @@ PythonObject SWIGBridge::ToSWIGWrapper(std::unique_ptr<lldb::SBValue> value_sb)
return ToSWIGHelper(value_sb.release(), SWIGTYPE_p_lldb__SBValue);
}
+PythonObject SWIGBridge::ToSWIGWrapper(std::unique_ptr<lldb::SBCommandReturnObject> result_up) {
+ return ToSWIGHelper(result_up.release(), SWIGTYPE_p_lldb__SBCommandReturnObject);
+}
+
PythonObject SWIGBridge::ToSWIGWrapper(lldb::ValueObjectSP value_sp) {
return ToSWIGWrapper(std::unique_ptr<lldb::SBValue>(new lldb::SBValue(value_sp)));
}
diff --git a/lldb/bindings/python/python-typemaps.swig b/lldb/bindings/python/python-typemaps.swig
index f8c33e15c03e66..88b6cd9ef6b6e7 100644
--- a/lldb/bindings/python/python-typemaps.swig
+++ b/lldb/bindings/python/python-typemaps.swig
@@ -476,6 +476,25 @@ template <> bool SetNumberFromPyObject<double>(double &number, PyObject *obj) {
$1 = $1 || PyCallable_Check(reinterpret_cast<PyObject *>($input));
}
+// For lldb::SBCommandPrintCallback
+%typemap(in) (lldb::SBCommandPrintCallback callback, void *baton) {
+ if (!($input == Py_None ||
+ PyCallable_Check(reinterpret_cast<PyObject *>($input)))) {
+ PyErr_SetString(PyExc_TypeError, "Need a callable object or None!");
+ SWIG_fail;
+ }
+
+ // Don't lose the callback reference.
+ Py_INCREF($input);
+ $1 = LLDBSwigPythonCallPythonCommandPrintCallback;
+ $2 = $input;
+}
+
+%typemap(typecheck) (lldb::SBCommandPrintCallback callback, void *baton) {
+ $1 = $input == Py_None;
+ $1 = $1 || PyCallable_Check(reinterpret_cast<PyObject *>($input));
+}
+
%typemap(in) (lldb::CommandOverrideCallback callback, void *baton) {
if (!($input == Py_None ||
PyCallable_Check(reinterpret_cast<PyObject *>($input)))) {
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index b72a462d04643b..fcabf60008b17d 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -727,7 +727,7 @@ lldb_private::python::SWIGBridge::LLDBSwigPythonHandleOptionArgumentCompletionFo
dict_sp->AddBooleanItem("no-completion", true);
return dict_sp;
}
-
+
// Convert the return dictionary to a DictionarySP.
StructuredData::ObjectSP result_obj_sp = result.CreateStructuredObject();
@@ -753,7 +753,7 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
auto pfunc = self.ResolveName<PythonCallable>("__call__");
if (!pfunc.IsAllocated()) {
- cmd_retobj.AppendError("Could not find '__call__' method in implementation class");
+ cmd_retobj.AppendError("Could not find '__call__' method in implementation class");
return false;
}
@@ -1012,6 +1012,26 @@ static void LLDBSwigPythonCallPythonLogOutputCallback(const char *str,
}
}
+// For DebuggerTerminateCallback functions
+static CommandReturnObjectCallbackResult LLDBSwigPythonCallPythonCommandPrintCallback(SBCommandReturnObject& result, void *callback_baton) {
+ SWIG_Python_Thread_Block swig_thread_block;
+
+ PyErr_Cleaner py_err_cleaner(true);
+
+ PythonObject result_arg = SWIGBridge::ToSWIGWrapper(
+ std::make_unique<SBCommandReturnObject>(result));
+ PythonCallable callable =
+ Retain<PythonCallable>(reinterpret_cast<PyObject *>(callback_baton));
+
+ if (!callable.IsValid())
+ return eCommandReturnObjectPrintCallbackSkipped;
+
+ PythonObject callback_result = callable(result_arg);
+
+ long long ret_val = unwrapOrSetPythonException(As<long long>(callback_result));
+ return (CommandReturnObjectCallbackResult)ret_val;
+}
+
// For DebuggerTerminateCallback functions
static void LLDBSwigPythonCallPythonSBDebuggerTerminateCallback(lldb::user_id_t debugger_id,
void *baton) {
diff --git a/lldb/include/lldb/API/SBCommandInterpreter.h b/lldb/include/lldb/API/SBCommandInterpreter.h
index b7e39b78586168..dd475ddf6c1702 100644
--- a/lldb/include/lldb/API/SBCommandInterpreter.h
+++ b/lldb/include/lldb/API/SBCommandInterpreter.h
@@ -247,13 +247,13 @@ class SBCommandInterpreter {
lldb::SBStringList &matches,
lldb::SBStringList &descriptions);
- /// Returns whether an interrupt flag was raised either by the SBDebugger -
+ /// Returns whether an interrupt flag was raised either by the SBDebugger -
/// when the function is not running on the RunCommandInterpreter thread, or
/// by SBCommandInterpreter::InterruptCommand if it is. If your code is doing
- /// interruptible work, check this API periodically, and interrupt if it
+ /// interruptible work, check this API periodically, and interrupt if it
/// returns true.
bool WasInterrupted() const;
-
+
/// Interrupts the command currently executing in the RunCommandInterpreter
/// thread.
///
@@ -331,6 +331,8 @@ class SBCommandInterpreter {
/// this list. Otherwise this list is empty.
SBStructuredData GetTranscript();
+ void SetPrintCallback(lldb::SBCommandPrintCallback callback, void *baton);
+
protected:
friend class lldb_private::CommandPluginInterfaceImplementation;
diff --git a/lldb/include/lldb/API/SBCommandReturnObject.h b/lldb/include/lldb/API/SBCommandReturnObject.h
index e8e20a3f3016b8..377e4714f6d26b 100644
--- a/lldb/include/lldb/API/SBCommandReturnObject.h
+++ b/lldb/include/lldb/API/SBCommandReturnObject.h
@@ -17,6 +17,7 @@
namespace lldb_private {
class CommandPluginInterfaceImplementation;
+class CommandPrintCallbackBaton;
class SBCommandReturnObjectImpl;
namespace python {
class SWIGBridge;
@@ -138,6 +139,7 @@ class LLDB_API SBCommandReturnObject {
friend class lldb_private::CommandPluginInterfaceImplementation;
friend class lldb_private::python::SWIGBridge;
+ friend class lldb_private::CommandPrintCallbackBaton;
SBCommandReturnObject(lldb_private::CommandReturnObject &ref);
diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h
index 31e8c9279f8b8b..b7b5cc06546f86 100644
--- a/lldb/include/lldb/API/SBDefines.h
+++ b/lldb/include/lldb/API/SBDefines.h
@@ -144,6 +144,9 @@ typedef bool (*SBBreakpointHitCallback)(void *baton, lldb::SBProcess &process,
typedef void (*SBDebuggerDestroyCallback)(lldb::user_id_t debugger_id,
void *baton);
+typedef CommandReturnObjectCallbackResult (*SBCommandPrintCallback)(
+ lldb::SBCommandReturnObject &result, void *baton);
+
typedef lldb::SBError (*SBPlatformLocateModuleCallback)(
void *baton, const lldb::SBModuleSpec &module_spec,
lldb::SBFileSpec &module_file_spec, lldb::SBFileSpec &symbol_file_spec);
diff --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h b/lldb/include/lldb/Interpreter/CommandInterpreter.h
index 2bafc30cc8e23a..b0a2d7ed3ace05 100644
--- a/lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -16,6 +16,7 @@
#include "lldb/Interpreter/CommandObject.h"
#include "lldb/Interpreter/ScriptInterpreter.h"
#include "lldb/Utility/Args.h"
+#include "lldb/Utility/Baton.h"
#include "lldb/Utility/Broadcaster.h"
#include "lldb/Utility/CompletionRequest.h"
#include "lldb/Utility/Event.h"
@@ -100,8 +101,7 @@ class CommandInterpreterRunOptions {
LazyBool stop_on_error, LazyBool stop_on_crash,
LazyBool echo_commands, LazyBool echo_comments,
LazyBool print_results, LazyBool print_errors,
- LazyBool add_to_history,
- LazyBool handle_repeats)
+ LazyBool add_to_history, LazyBool handle_repeats)
: m_stop_on_continue(stop_on_continue), m_stop_on_error(stop_on_error),
m_stop_on_crash(stop_on_crash), m_echo_commands(echo_commands),
m_echo_comment_commands(echo_comments), m_print_results(print_results),
@@ -248,13 +248,16 @@ class CommandInterpreter : public Broadcaster,
enum CommandTypes {
eCommandTypesBuiltin = 0x0001, //< native commands such as "frame"
eCommandTypesUserDef = 0x0002, //< scripted commands
- eCommandTypesUserMW = 0x0004, //< multiword commands (command containers)
+ eCommandTypesUserMW = 0x0004, //< multiword commands (command containers)
eCommandTypesAliases = 0x0008, //< aliases such as "po"
- eCommandTypesHidden = 0x0010, //< commands prefixed with an underscore
+ eCommandTypesHidden = 0x0010, //< commands prefixed with an underscore
eCommandTypesAllThem = 0xFFFF //< all commands
};
- // The CommandAlias and CommandInterpreter both have a hand in
+ typedef lldb::CommandReturnObjectCallbackResult (
+ *CommandReturnObjectCallback)(CommandReturnObject &, void *);
+
+ // The CommandAlias and CommandInterpreter both have a hand in
// substituting for alias commands. They work by writing special tokens
// in the template form of the Alias command, and then detecting them when the
// command is executed. These are the special tokens:
@@ -334,9 +337,8 @@ class CommandInterpreter : public Broadcaster,
/// dummy "contains everything MWC, so we return null here, but
/// in this case error.Success is true.
- CommandObjectMultiword *VerifyUserMultiwordCmdPath(Args &path,
- bool leaf_is_command,
- Status &result);
+ CommandObjectMultiword *
+ VerifyUserMultiwordCmdPath(Args &path, bool leaf_is_command, Status &result);
CommandAlias *AddAlias(llvm::StringRef alias_name,
lldb::CommandObjectSP &command_obj_sp,
@@ -596,7 +598,7 @@ class CommandInterpreter : public Broadcaster,
void SetEchoCommentCommands(bool enable);
bool GetRepeatPreviousCommand() const;
-
+
bool GetRequireCommandOverwrite() const;
const CommandObject::CommandMap &GetUserCommands() const {
@@ -666,6 +668,9 @@ class CommandInterpreter : public Broadcaster,
++m_command_usages[cmd_obj.GetCommandName()];
}
+ void SetPrintCallback(CommandReturnObjectCallback callback,
+ lldb::BatonSP baton_sp);
+
llvm::json::Value GetStatistics();
const StructuredData::Array &GetTranscript() const;
@@ -776,6 +781,12 @@ class CommandInterpreter : public Broadcaster,
std::vector<uint32_t> m_command_source_flags;
CommandInterpreterRunResult m_result;
+ /// An optional callback to handle printing the CommandReturnObject.
+ /// @{
+ CommandReturnObjectCallback m_print_callback = nullptr;
+ lldb::BatonSP m_print_callback_baton_sp;
+ /// @}
+
// The exit code the user has requested when calling the 'quit' command.
// No value means the user hasn't set a custom exit code so far.
std::optional<int> m_quit_exit_code;
diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h
index 9fef59337016df..f96da34889a324 100644
--- a/lldb/include/lldb/Interpreter/CommandReturnObject.h
+++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -32,9 +32,9 @@ class CommandReturnObject {
~CommandReturnObject() = default;
/// Format any inline diagnostics with an indentation of \c indent.
- std::string GetInlineDiagnosticString(unsigned indent);
+ std::string GetInlineDiagnosticString(unsigned indent) const;
- llvm::StringRef GetOutputString() {
+ llvm::StringRef GetOutputString() const {
lldb::StreamSP stream_sp(m_out_stream.GetStreamAtIndex(eStreamStringIndex));
if (stream_sp)
return std::static_pointer_cast<StreamString>(stream_sp)->GetString();
@@ -46,7 +46,7 @@ class CommandReturnObject {
/// If \c with_diagnostics is true, all diagnostics are also
/// rendered into the string. Otherwise the expectation is that they
/// are fetched with \ref GetInlineDiagnosticString().
- std::string GetErrorString(bool with_diagnostics = true);
+ std::string GetErrorString(bool with_diagnostics = true) const;
StructuredData::ObjectSP GetErrorData();
Stream &GetOutputStream() {
@@ -95,11 +95,11 @@ class CommandReturnObject {
m_err_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
}
- lldb::StreamSP GetImmediateOutputStream() {
+ lldb::StreamSP GetImmediateOutputStream() const {
return m_out_stream.GetStreamAtIndex(eImmediateStreamIndex);
}
- lldb::StreamSP GetImmediateErrorStream() {
+ lldb::StreamSP GetImmediateErrorStream() const {
return m_err_stream.GetStreamAtIndex(eImmediateStreamIndex);
}
diff --git a/lldb/include/lldb/Utility/StreamTee.h b/lldb/include/lldb/Utility/StreamTee.h
index 5695586171f358..571548e2e23fe6 100644
--- a/lldb/include/lldb/Utility/StreamTee.h
+++ b/lldb/include/lldb/Utility/StreamTee.h
@@ -85,7 +85,7 @@ class StreamTee : public Stream {
return result;
}
- lldb::StreamSP GetStreamAtIndex(uint32_t idx) {
+ lldb::StreamSP GetStreamAtIndex(uint32_t idx) const {
lldb::StreamSP stream_sp;
std::lock_guard<std::recursive_mutex> guard(m_streams_mutex);
if (idx < m_streams.size())
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 50d2233509de6f..fecf9cbb765f71 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1368,6 +1368,15 @@ enum Severity {
eSeverityInfo, // Equivalent to Remark used in clang.
};
+/// Callback return value, indicating whether it handled printing the
+/// CommandReturnObject or deferred doing so to the CommandInterpreter.
+enum CommandReturnObjectCallbackResult {
+ /// The callback deferred printing the command return object.
+ eCommandReturnObjectPrintCallbackSkipped = 0,
+ /// The callback handled printing the command return object.
+ eCommandReturnObjectPrintCallbackHandled = 1,
+};
+
} // namespace lldb
#endif // LLDB_LLDB_ENUMERATIONS_H
diff --git a/lldb/source/API/SBCommandInterpreter.cpp b/lldb/source/API/SBCommandInterpreter.cpp
index 7a35473283684c..08bac1afc64e87 100644
--- a/lldb/source/API/SBCommandInterpreter.cpp
+++ b/lldb/source/API/SBCommandInterpreter.cpp
@@ -151,7 +151,7 @@ bool SBCommandInterpreter::WasInterrupted() const {
bool SBCommandInterpreter::InterruptCommand() {
LLDB_INSTRUMENT_VA(this);
-
+
return (IsValid() ? m_opaque_ptr->InterruptCommand() : false);
}
@@ -743,3 +743,41 @@ void SBCommand::SetFlags(uint32_t flags) {
if (IsValid())
m_opaque_sp->GetFlags().Set(flags);
}
+
+namespace lldb_private {
+struct CommandCallbackData {
+ SBCommandPrintCallback callback;
+ void *callback_baton;
+};
+
+class CommandPrintCallbackBaton
+ : public lldb_private::TypedBaton<CommandCallbackData> {
+public:
+ CommandPrintCallbackBaton(SBCommandPrintCallback callback, void *baton)
+ : TypedBaton(std::make_unique<CommandCallbackData>()) {
+ getItem()->callback = callback;
+ getItem()->callback_baton = baton;
+ }
+
+ static lldb::CommandReturnObjectCallbackResult
+ PrivateCallback(lldb_private::CommandReturnObject &result, void *baton) {
+ if (baton) {
+ CommandCallbackData *data = (CommandCallbackData *)baton;
+ SBCommandReturnObject sb_result(result);
+ return data->callback(sb_result, data->callback_baton);
+ }
+ return eCommandReturnObjectPrintCallbackSkipped;
+ }
+};
+} // namespace lldb_private
+
+void SBCommandInterpreter::SetPrintCallback(
+ lldb::SBCommandPrintCallback callback, void *baton) {
+ LLDB_INSTRUMENT_VA(this, callback, baton);
+
+ BatonSP baton_sp =
+ std::make_shared<CommandPrintCallbackBaton>(callback, baton);
+ if (m_opaque_ptr)
+ return m_opaque_ptr->SetPrintCallback(
+ &CommandPrintCallbackBaton::PrivateCallback, baton_sp);
+}
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index 764dcfd1903b19..cc74f17b287cf2 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -3182,30 +3182,44 @@ void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler,
if ((result.Succeeded() &&
io_handler.GetFlags().Test(eHandleCommandFlagPrintResult)) ||
io_handler.GetFlags().Test(eHandleCommandFlagPrintErrors)) {
- // Display any inline diagnostics first.
- const bool inline_diagnostics = !result.GetImmediateErrorStream() &&
- GetDebugger().GetShowInlineDiagnostics();
- if (inline_diagnostics) {
- unsigned prompt_len = m_debugger.GetPrompt().size();
- if (auto indent = result.GetDiagnosticIndent()) {
- std::string diags =
- result.GetInlineDiagnosticString(prompt_len + *indent);
- PrintCommandOutput(io_handler, diags, true);
+ auto DefaultPrintCallback = [&](const CommandReturnObject &result) {
+ // Display any inline diagnostics first.
+ const bool inline_diagnostics = !result.GetImmediateErrorStream() &&
+ GetDebugger().GetShowInlineDiagnostics();
+ if (inline_diagnostics) {
+ unsigned prompt_len = m_debugger.GetPrompt().size();
+ if (auto indent = result.GetDiagnosticIndent()) {
+ std::string diags =
+ result.GetInlineDiagnosticString(prompt_len + *indent);
+ PrintCommandOutput(io_handler, diags, true);
+ }
}
- }
- // Display any STDOUT/STDERR _prior_ to emitting the command result text.
- GetProcessOutput();
+ // Display any STDOUT/STDERR _prior_ to emitting the command result text.
+ GetProcessOutput();
- if (!result.GetImmediateOutputStream()) {
- llvm::StringRef output = result.GetOutputString();
- PrintCommandOutput(io_handler, output, true);
- }
+ if (!result.GetImmediateOutputStream()) {
+ llvm::StringRef output = result.GetOutputString();
+ PrintCommandOutput(io_handler, output, true);
+ }
- // Now emit the command error text from the command we just executed.
- if (!result.GetImmediateErrorStream()) {
- std::string error = result.GetErrorString(!inline_diagnostics);
- PrintCommandOutput(io_handler, error, false);
+ // Now emit the command error text from the command we just executed.
+ if (!result.GetImmediateErrorStream()) {
+ std::string error = result.GetErrorString(!inline_diagnostics);
+ PrintCommandOutput(io_handler, error, false);
+ }
+ };
+
+ if (m_print_callback) {
+ void *baton = m_print_callback_baton_sp
+ ? m_print_callback_baton_sp->data()
+ : nullptr;
+ lldb::CommandReturnObjectCallbackResult callback_result =
+ m_print_callback(result, baton);
+ if (callback_result == eCommandReturnObjectPrintCallbackSkipped)
+ DefaultPrintCallback(result);
+ } else {
+ DefaultPrintCallback(result);
}
}
@@ -3656,3 +3670,9 @@ llvm::json::Value CommandInterpreter::GetStatistics() {
const StructuredData::Array &CommandInterpreter::GetTranscript() const {
return m_transcript;
}
+
+void CommandInterpreter::SetPrintCallback(CommandReturnObjectCallback callback,
+ lldb::BatonSP baton_sp) {
+ m_print_callback = callback;
+ m_print_callback_baton_sp = baton_sp;
+}
diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp
index b99b2bc7b36ce4..0a2948e8e6ca44 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -147,7 +147,8 @@ void CommandReturnObject::SetError(llvm::Error error) {
}
}
-std::string CommandReturnObject::GetInlineDiagnosticString(unsigned indent) {
+std::string
+CommandReturnObject::GetInlineDiagnosticString(unsigned indent) const {
StreamString diag_stream(m_colors);
RenderDiagnosticDetails(diag_stream, indent, true, m_diagnostics);
// Duplex the diagnostics to the secondary stream (but not inlined).
@@ -157,7 +158,7 @@ std::string CommandReturnObject::GetInlineDiagnosticString(unsigned indent) {
return diag_stream.GetString().str();
}
-std::strin...
[truncated]
|
|
I recommend reviewing the last commit and ignoring the two NFC commits, which I will land separately anyway. |
lldb/test/API/python_api/interpreter_callback/TestCommandInterepterPrintCallback.py
Outdated
Show resolved
Hide resolved
|
Man there's a lot of boiler plate in hooking up to SWIG... But this all looks right to me. I wondered whether the callback would benefit from knowing whether the result had been dumped immediately or not, but I can't think of a good reason for that. If this result printing callback was also passed the command string (which it has from just above where the callback is called) then this would also be an easy way to do logging of command execution. That's not needed for your current purpose, but would add another strong motivation for this change... |
Adrian made a similar observation offline. What do you think about putting that in the |
There was a problem hiding this comment.
It's not clear from the patch description whether you actually need the ability to suppress the command output, or if you just want the ability to "access" the diagnostics. Because, if its the latter, then perhaps an (async) event would be a slightly more lightweight alternative?
| BatonSP baton_sp = | ||
| std::make_shared<CommandPrintCallbackBaton>(callback, baton); |
There was a problem hiding this comment.
I know this is here as a form of dependency inversion, and that we have this Baton thingy already, but it all fields unnecessarily elaborate. Couldn't the private callback type be a std::function<CommandReturnObjectCallbackResult(CommandReturnObject&)>, and just wrap things in a lambda?
There was a problem hiding this comment.
Good point, I was mimicking what we do for breakpoint callbacks. I suspect we could simplify that with the same trick. Updated the PR.
| # Test registering a callback that handles the printing. Make sure the | ||
| # result is passed to the callback and that we don't print the result. | ||
| def handling_callback(return_object): | ||
| nonlocal called |
Yes, part of the requirement is that the output isn't printed by lldb itself, although the implementer is still in control with the return value. |
That's a good idea. |
|
Alright, I have two follow-ups/action items:
|
Fix CommandInterpreter.{h,cpp} formatting in preparation for #125006.
There's no reason these methods cannot be `const`. Currently this prevents us from passing around a const ref. This patch is in preparation for #125006.
Xcode uses a pseudoterminal for the debugger console. - The upside of this apporach is that it means that it can rely on LLDB's IOHandlers for multiline and script input. - The downside of this approach is that the command output is printed to the PTY and you don't get a SBCommandReturnObject. Adrian added support for inline diagnostics in llvm#110901 and we'd like to access those from the IDE. This patch adds support for registering a callback in the command interpreter that gives access to the (SB)CommandReturnObject right before it will be printed. The callback implementation can choose whether it likes to handle printing the result or defer to lldb. If the callback indicated it handled the result, the command interpreter will skip printing the result. We considered a few other alternatives to solve this problem: - The most obvious one is using `HandleCommand`, which returns a `SBCommandReturnObject`. The problem with this approach is the multiline input mentioned above. We would need a way to tell the IDE that it should expect multiline input, which isn't known until LLDB starts handling the command. - To address the multiline issue,we considered exposing (some of the) IOHandler machinery through the SB API. To solve this particular issue, that would require reimplementing a ton of logic that already exists today in the CommandInterpeter. Furthermore that seems like overkill compared to the proposed solution. rdar://141254310
JDevlieghere
force-pushed
the
command-interpreter-callback
branch
from
869a1c2 to
e6d04c1
Compare
|
Landed the NFC changes and rebased the PR. |
There was a problem hiding this comment.
Yes, part of the requirement is that the output isn't printed by lldb itself, although the implementer is still in control with the return value.
Okay. I was afraid of that. :)
The code looks good, though I must admit that the functionality seems a bit.. random. Like, you're putting lldb in a terminal, but then you don't want it to behave like lldb in a terminal. I don't suppose there's anything you can say about what you intend to do with it?
One use case is that a UI might want to make a nicer presentation for some class of errors - for in particular the expression syntax errors that have more rich structure - making them disclosable or whatnot. Since those can be huge - particularly compile errors from expressions - you really don't want to duplicate the printing in that case. The way Xcode presents lldb is that it intermixes the tty output from RunCommandInterpreter with process I/O and the output from Darwin's system logging into segmented bins in the console output. They want the input part of the TTY interface to work just like lldb at the command line - particularly they don't want to have to reimplement the modal IOHandlers for |
|
There are other ways you could extend this that would be very fun. For instance, we could make any command that conses up its result just by calling the ValueObjectPrinter on some ValueObject to also stuff that ValueObject into the command return. Then we could add SBCommandResultObject::GetReturnValueObject to return that ValueObject. Then the callback could check whether there was a ValueObject in the command result, and if there is it would suppress that textual output and render the ValueObject as a live object in the result slot in the command output it is building up. Note, when we get to this, we probably want to allow the callback to say "I take over all ValueObject printing" so that internally we wouldn't waste time making up the rendered result. |
|
Jim & I have been discussing this offline. Thanks for adding the additional context to the PR! |
|
We have a bunch of other structured output that we could start decorating this way. For instance if |
There was a problem hiding this comment.
Thanks for the explanation. I understand what you mean now. The feature sounds cool, and the implementation sort of makes sense (sort of, because the HandleCommand idea is still "sort of" running around my head, but I do see how it could be very difficult to achieve that with it).
I do have my doubts about embedding the command string into the return object, but it sounds like the thought had occurred to at least two of you independently, so if you all think that's the best direction, then I guess I'm outvoted.
I don't feel strongly about storing the command in the Anyway, let's continue the discussion in #125132. We just branched for the 20.x release so we have plenty of time to refine the callback API if needed. If anyone (else) sees this and thinks about adopting it, this is your warning that it might change :-). |
As suggested in llvm#125006. Depending on which PR lands first, I'll update TestCommandInterepterPrintCallback.py to check that the CommandReturnObject passed to the callback has the correct command.
As suggested in #125006. Depending on which PR lands first, I'll update `TestCommandInterepterPrintCallback.py` to check that the `CommandReturnObject` passed to the callback has the correct command.
There's no reason these methods cannot be `const`. Currently this prevents us from passing around a const ref. This patch is in preparation for llvm#125006. (cherry picked from commit 7469032)
As suggested in llvm#125006. Depending on which PR lands first, I'll update `TestCommandInterepterPrintCallback.py` to check that the `CommandReturnObject` passed to the callback has the correct command. (cherry picked from commit 906eeed)
Xcode uses a pseudoterminal for the debugger console. - The upside of this apporach is that it means that it can rely on LLDB's IOHandlers for multiline and script input. - The downside of this approach is that the command output is printed to the PTY and you don't get a SBCommandReturnObject. Adrian added support for inline diagnostics (llvm#110901) and we'd like to access those from the IDE. This patch adds support for registering a callback in the command interpreter that gives access to the `(SB)CommandReturnObject` right before it will be printed. The callback implementation can choose whether it likes to handle printing the result or defer to lldb. If the callback indicated it handled the result, the command interpreter will skip printing the result. We considered a few other alternatives to solve this problem: - The most obvious one is using `HandleCommand`, which returns a `SBCommandReturnObject`. The problem with this approach is the multiline input mentioned above. We would need a way to tell the IDE that it should expect multiline input, which isn't known until LLDB starts handling the command. - To address the multiline issue,we considered exposing (some of the) IOHandler machinery through the SB API. To solve this particular issue, that would require reimplementing a ton of logic that already exists today in the CommandInterpeter. Furthermore that seems like overkill compared to the proposed solution. rdar://141254310 (cherry picked from commit 97f6e53)
As suggested in llvm#125006. Depending on which PR lands first, I'll update `TestCommandInterepterPrintCallback.py` to check that the `CommandReturnObject` passed to the callback has the correct command.
Xcode uses a pseudoterminal for the debugger console. - The upside of this apporach is that it means that it can rely on LLDB's IOHandlers for multiline and script input. - The downside of this approach is that the command output is printed to the PTY and you don't get a SBCommandReturnObject. Adrian added support for inline diagnostics (llvm#110901) and we'd like to access those from the IDE. This patch adds support for registering a callback in the command interpreter that gives access to the `(SB)CommandReturnObject` right before it will be printed. The callback implementation can choose whether it likes to handle printing the result or defer to lldb. If the callback indicated it handled the result, the command interpreter will skip printing the result. We considered a few other alternatives to solve this problem: - The most obvious one is using `HandleCommand`, which returns a `SBCommandReturnObject`. The problem with this approach is the multiline input mentioned above. We would need a way to tell the IDE that it should expect multiline input, which isn't known until LLDB starts handling the command. - To address the multiline issue,we considered exposing (some of the) IOHandler machinery through the SB API. To solve this particular issue, that would require reimplementing a ton of logic that already exists today in the CommandInterpeter. Furthermore that seems like overkill compared to the proposed solution. rdar://141254310
There are a lot of lldb commands whose result is really a ValueObject that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (llvm#125006), we can store the resultant ValueObject in the return object, allowing an IDE to access the SBValue and do its own rich formatting. rdar://143965453
There are a lot of lldb commands whose result is really a ValueObject that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (llvm#125006), we can store the resultant ValueObject in the return object, allowing an IDE to access the SBValue and do its own rich formatting. rdar://143965453
There are a lot of lldb commands whose result is really one or more ValueObjects that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (#125006), we can store the resultant ValueObjects in the return object, allowing an IDE to access the SBValues and do its own rich formatting. rdar://143965453
…127566) There are a lot of lldb commands whose result is really one or more ValueObjects that we then print with the ValueObjectPrinter. Now that we have the ability to access the SBCommandReturnObject through a callback (llvm#125006), we can store the resultant ValueObjects in the return object, allowing an IDE to access the SBValues and do its own rich formatting. rdar://143965453 Conflicts: lldb/include/lldb/Interpreter/CommandReturnObject.h
Xcode uses a pseudoterminal for the debugger console.
This patch adds support for registering a callback in the command interpreter that gives access to the
(SB)CommandReturnObjectright before it will be printed. The callback implementation can choose whether it likes to handle printing the result or defer to lldb. If the callback indicated it handled the result, the command interpreter will skip printing the result.We considered a few other alternatives to solve this problem:
HandleCommand, which returns aSBCommandReturnObject. The problem with this approach is the multiline input mentioned above. We would need a way to tell the IDE that it should expect multiline input, which isn't known until LLDB starts handling the command.rdar://141254310