[lldb] Store the command in the CommandReturnObject #125132
Conversation
JDevlieghere
force-pushed
the
store-command-in-commandreturnobject
branch
from
014ba7b to
aa41718
Compare
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesAs suggested in #125006. Depending on which PR lands first, I'll update Full diff: https://github.com/llvm/llvm-project/pull/125132.diff 14 Files Affected:
diff --git a/lldb/include/lldb/API/SBCommandReturnObject.h b/lldb/include/lldb/API/SBCommandReturnObject.h
index e8e20a3f3016b8..4096c5bafdcfc9 100644
--- a/lldb/include/lldb/API/SBCommandReturnObject.h
+++ b/lldb/include/lldb/API/SBCommandReturnObject.h
@@ -42,6 +42,8 @@ class LLDB_API SBCommandReturnObject {
bool IsValid() const;
+ const char *GetCommand();
+
const char *GetOutput();
const char *GetError();
diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h
index f96da34889a324..f588032c8acab0 100644
--- a/lldb/include/lldb/Interpreter/CommandReturnObject.h
+++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -27,10 +27,14 @@ namespace lldb_private {
class CommandReturnObject {
public:
- CommandReturnObject(bool colors);
+ CommandReturnObject(std::string command, bool colors);
~CommandReturnObject() = default;
+ const std::string &GetCommand() const { return m_command; }
+
+ void SetCommand(std::string command) { m_command = std::move(command); }
+
/// Format any inline diagnostics with an indentation of \c indent.
std::string GetInlineDiagnosticString(unsigned indent) const;
@@ -182,6 +186,8 @@ class CommandReturnObject {
private:
enum { eStreamStringIndex = 0, eImmediateStreamIndex = 1 };
+ std::string m_command;
+
StreamTee m_out_stream;
StreamTee m_err_stream;
std::vector<DiagnosticDetail> m_diagnostics;
diff --git a/lldb/source/API/SBCommandReturnObject.cpp b/lldb/source/API/SBCommandReturnObject.cpp
index 9df8aa48b99366..07355f9318e8dc 100644
--- a/lldb/source/API/SBCommandReturnObject.cpp
+++ b/lldb/source/API/SBCommandReturnObject.cpp
@@ -23,7 +23,7 @@ using namespace lldb_private;
class lldb_private::SBCommandReturnObjectImpl {
public:
- SBCommandReturnObjectImpl() : m_ptr(new CommandReturnObject(false)) {}
+ SBCommandReturnObjectImpl() : m_ptr(new CommandReturnObject("", false)) {}
SBCommandReturnObjectImpl(CommandReturnObject &ref)
: m_ptr(&ref), m_owned(false) {}
SBCommandReturnObjectImpl(const SBCommandReturnObjectImpl &rhs)
@@ -84,6 +84,13 @@ SBCommandReturnObject::operator bool() const {
return true;
}
+const char *SBCommandReturnObject::GetCommand() {
+ LLDB_INSTRUMENT_VA(this);
+
+ ConstString output(ref().GetCommand());
+ return output.AsCString(/*value_if_empty*/ "");
+}
+
const char *SBCommandReturnObject::GetOutput() {
LLDB_INSTRUMENT_VA(this);
diff --git a/lldb/source/Breakpoint/BreakpointOptions.cpp b/lldb/source/Breakpoint/BreakpointOptions.cpp
index 09abcf5e081d28..e825ca7b6d7d1e 100644
--- a/lldb/source/Breakpoint/BreakpointOptions.cpp
+++ b/lldb/source/Breakpoint/BreakpointOptions.cpp
@@ -616,7 +616,8 @@ bool BreakpointOptions::BreakpointOptionsCallbackFunction(
Target *target = exe_ctx.GetTargetPtr();
if (target) {
Debugger &debugger = target->GetDebugger();
- CommandReturnObject result(debugger.GetUseColor());
+ CommandReturnObject result("<breakpoint callback>",
+ debugger.GetUseColor());
// Rig up the results secondary output stream to the debugger's, so the
// output will come out synchronously if the debugger is set up that way.
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index 13491b5c794425..84ec1f37cdf9e8 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -504,7 +504,7 @@ void CommandObjectExpression::IOHandlerInputComplete(IOHandler &io_handler,
StreamFileSP error_sp = io_handler.GetErrorStreamFileSP();
CommandReturnObject return_obj(
- GetCommandInterpreter().GetDebugger().GetUseColor());
+ line, GetCommandInterpreter().GetDebugger().GetUseColor());
EvaluateExpression(line.c_str(), *output_sp, *error_sp, return_obj);
if (output_sp)
diff --git a/lldb/source/Commands/CommandObjectWatchpointCommand.cpp b/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
index ab1a2b390936c4..d9b1dc8f5d51f5 100644
--- a/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -243,7 +243,8 @@ are no syntax errors may indicate that a function was declared but never called.
Target *target = exe_ctx.GetTargetPtr();
if (target) {
Debugger &debugger = target->GetDebugger();
- CommandReturnObject result(debugger.GetUseColor());
+ CommandReturnObject result("<watchpoint callback>",
+ debugger.GetUseColor());
// Rig up the results secondary output stream to the debugger's, so the
// output will come out synchronously if the debugger is set up that
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 2df2aeb20aa26a..9c62600994c3a5 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -799,7 +799,7 @@ void Debugger::Destroy(DebuggerSP &debugger_sp) {
CommandInterpreter &cmd_interpreter = debugger_sp->GetCommandInterpreter();
if (cmd_interpreter.GetSaveSessionOnQuit()) {
- CommandReturnObject result(debugger_sp->GetUseColor());
+ CommandReturnObject result("<save transcript>", debugger_sp->GetUseColor());
cmd_interpreter.SaveTranscript(result);
if (result.Succeeded())
(*debugger_sp->GetAsyncOutputStream())
diff --git a/lldb/source/Expression/REPL.cpp b/lldb/source/Expression/REPL.cpp
index 4b53537e50e62a..75a16a1ef2ce42 100644
--- a/lldb/source/Expression/REPL.cpp
+++ b/lldb/source/Expression/REPL.cpp
@@ -255,7 +255,7 @@ void REPL::IOHandlerInputComplete(IOHandler &io_handler, std::string &code) {
ci.SetPromptOnQuit(false);
// Execute the command
- CommandReturnObject result(debugger.GetUseColor());
+ CommandReturnObject result(code, debugger.GetUseColor());
result.SetImmediateOutputStream(output_sp);
result.SetImmediateErrorStream(error_sp);
ci.HandleCommand(code.c_str(), eLazyBoolNo, result);
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index 4869b811f99e71..422fe03efa8224 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1893,6 +1893,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
LLDB_LOGF(log, "Processing command: %s", command_line);
LLDB_SCOPED_TIMERF("Processing command: %s.", command_line);
+ result.SetCommand(command_line);
+
if (INTERRUPT_REQUESTED(GetDebugger(), "Interrupted initiating command")) {
result.AppendError("... Interrupted");
return false;
@@ -2616,7 +2618,7 @@ void CommandInterpreter::HandleCommands(
m_debugger.GetPrompt().str().c_str(), cmd);
}
- CommandReturnObject tmp_result(m_debugger.GetUseColor());
+ CommandReturnObject tmp_result(cmd, m_debugger.GetUseColor());
tmp_result.SetInteractive(result.GetInteractive());
tmp_result.SetSuppressImmediateOutput(true);
@@ -2644,7 +2646,8 @@ void CommandInterpreter::HandleCommands(
(uint64_t)idx, cmd, error_msg);
m_debugger.SetAsyncExecution(old_async_execution);
return;
- } else if (options.GetPrintResults()) {
+ }
+ if (options.GetPrintResults()) {
result.AppendMessageWithFormatv("Command #{0} '{1}' failed with {2}",
(uint64_t)idx + 1, cmd, error_msg);
}
@@ -3177,7 +3180,7 @@ void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler,
RestoreExecutionContext();
});
- lldb_private::CommandReturnObject result(m_debugger.GetUseColor());
+ lldb_private::CommandReturnObject result(line, m_debugger.GetUseColor());
HandleCommand(line.c_str(), eLazyBoolCalculate, result);
// Now emit the command output text from the command we just executed
diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp
index 0a2948e8e6ca44..85bbbda8fda971 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -47,8 +47,9 @@ static void DumpStringToStreamWithNewline(Stream &strm, const std::string &s) {
strm.EOL();
}
-CommandReturnObject::CommandReturnObject(bool colors)
- : m_out_stream(colors), m_err_stream(colors), m_colors(colors) {}
+CommandReturnObject::CommandReturnObject(std::string command, bool colors)
+ : m_command(std::move(command)), m_out_stream(colors), m_err_stream(colors),
+ m_colors(colors) {}
void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
SetStatus(eReturnStatusFailed);
diff --git a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
index 82f18c5fe37a21..81f5444a49b997 100644
--- a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -1034,7 +1034,8 @@ bool RunEnableCommand(CommandInterpreter &interpreter) {
}
// Run the command.
- CommandReturnObject return_object(interpreter.GetDebugger().GetUseColor());
+ CommandReturnObject return_object("<enable command>",
+ interpreter.GetDebugger().GetUseColor());
interpreter.HandleCommand(command_stream.GetData(), eLazyBoolNo,
return_object);
return return_object.Succeeded();
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 8d77097477651c..b5c2db606e355c 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3933,7 +3933,7 @@ Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
if (!m_commands.GetSize())
return StopHookResult::KeepStopped;
- CommandReturnObject result(false);
+ CommandReturnObject result("<stop hook>", false);
result.SetImmediateOutputStream(output_sp);
result.SetInteractive(false);
Debugger &debugger = exc_ctx.GetTargetPtr()->GetDebugger();
diff --git a/lldb/test/API/python_api/commandreturnobject/TestSBCommandReturnObject.py b/lldb/test/API/python_api/commandreturnobject/TestSBCommandReturnObject.py
new file mode 100644
index 00000000000000..b0d0b7a8dfe4e7
--- /dev/null
+++ b/lldb/test/API/python_api/commandreturnobject/TestSBCommandReturnObject.py
@@ -0,0 +1,17 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SBCommandReturnObjectTest(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test(self):
+ res = lldb.SBCommandReturnObject()
+ self.assertEqual(res.GetCommand(), "")
+
+ ci = self.dbg.GetCommandInterpreter()
+ ci.HandleCommand("help help", res)
+ self.assertTrue(res.Succeeded())
+ self.assertEqual(res.GetCommand(), "help help")
diff --git a/lldb/tools/lldb-test/lldb-test.cpp b/lldb/tools/lldb-test/lldb-test.cpp
index 1960240dc41514..a372ed00baa014 100644
--- a/lldb/tools/lldb-test/lldb-test.cpp
+++ b/lldb/tools/lldb-test/lldb-test.cpp
@@ -442,7 +442,7 @@ int opts::breakpoint::evaluateBreakpoints(Debugger &Dbg) {
std::string Command = substitute(Line);
P.formatLine("Command: {0}", Command);
- CommandReturnObject Result(/*colors*/ false);
+ CommandReturnObject Result("", /*colors*/ false);
if (!Dbg.GetCommandInterpreter().HandleCommand(
Command.c_str(), /*add_to_history*/ eLazyBoolNo, Result)) {
P.formatLine("Failed: {0}", Result.GetErrorString());
@@ -1189,7 +1189,7 @@ int opts::irmemorymap::evaluateMemoryMapCommands(Debugger &Dbg) {
// Set up a Process. In order to allocate memory within a target, this
// process must be alive and must support JIT'ing.
- CommandReturnObject Result(/*colors*/ false);
+ CommandReturnObject Result("", /*colors*/ false);
Dbg.SetAsyncExecution(false);
CommandInterpreter &CI = Dbg.GetCommandInterpreter();
auto IssueCmd = [&](const char *Cmd) -> bool {
@@ -1258,7 +1258,7 @@ int main(int argc, const char *argv[]) {
auto Dbg = lldb_private::Debugger::CreateInstance();
ModuleList::GetGlobalModuleListProperties().SetEnableExternalLookup(false);
- CommandReturnObject Result(/*colors*/ false);
+ CommandReturnObject Result("", /*colors*/ false);
Dbg->GetCommandInterpreter().HandleCommand(
"settings set plugin.process.gdb-remote.packet-timeout 60",
/*add_to_history*/ eLazyBoolNo, Result);
|
|
This looks good. I wonder if we ought to do something a little less ad hoc about the cases where our command return objects are from HandleCommands, so they won't necessarily have the result of a single command. In the case where a UI is parsing these, it might very well need to know that fact. They all currently have |
I can add an enum to indicate whether the command was a user command or something LLDB put together and have two constructors that each do the appropriate thing. |
There was a problem hiding this comment.
This idea has bothered me from the beginning, but I didn't want to say anything, as it was mostly just a feeling. After seeing this patch, I think I can put some words behind it.
The thing I don't like about this patch is the repetition/redundancy it creates. Like, you first create the CommandReturnObject, give it the command it's hold the result of. Then, you pass that object into HandleCommand, but that's not all you give it -- the function takes the string of the command, again.
It's not the end of the world, but I think it's unfortunate, and while it works nice for your current use case, I don't think it makes that much sense for other usages. For example, if the CommandReturnObject had always contained the command part, would the signature of a python command still be def cmd(debugger, command, result, dict):, or would we have expected the command to retrieve it from the result object?
It's hard to say, but what I think we can say is that if you make your current callback take the "command" and the "result of that command" as two separate arguments, then it will at least be consistent with the signature above.
One difference here is that what we're putting in the stored command string is the command as the user typed it. That isn't what we pass to the command - the command gets the result after alias substitution. Also, though I was late providing it in the future when possible we really want command writers to use the parsed form for everything but raw commands. So that's even further from the raw string we're adding to the SBCommandReturnObject. The string we're putting in there is really only useful for logging. |
|
One way to make the intent of the command string in the return object clear is to ONLY set it after we've called HandleCommand (or HandleCommands). That way it's explicitly NOT meant to help execute the command - after all, commands really should NOT be operating on the un-alias-resolved invocation. Then the intent will be clear. We could even call the API |
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.
JDevlieghere
force-pushed
the
store-command-in-commandreturnobject
branch
from
aa41718 to
ac8fe96
Compare
|
Rebased and implemented @jimingham's latest suggestion. |
|
Other than the comment comment, LGTM. |
|
I think this is better. As much as I like initializing things in the constructor, I don't think it makes sense when the whole CommandReturnObject is built around using it as a by-ref return value. |
Now that we store the command in the CommandReturnObject (#125132) we can check the command in the print callback.
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)
Now that we store the command in the CommandReturnObject (llvm#125132) we can check the command in the print callback. (cherry picked from commit 53d6e59)
|
Btw, there are now two tests in the LLDB test-suite called which Could you rename one of them please? |
In #125132, Michael pointed out that there are now two tests with the same name: ./lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py ./lldb/test/API/python_api/commandreturnobject/TestSBCommandReturnObject.py
|
Merged them in 4722200 |
In llvm#125132, Michael pointed out that there are now two tests with the same name: ./lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py ./lldb/test/API/python_api/commandreturnobject/TestSBCommandReturnObject.py (cherry picked from commit 4722200)
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.
Now that we store the command in the CommandReturnObject (llvm#125132) we can check the command in the print callback.
In llvm#125132, Michael pointed out that there are now two tests with the same name: ./lldb/test/API/api/command-return-object/TestSBCommandReturnObject.py ./lldb/test/API/python_api/commandreturnobject/TestSBCommandReturnObject.py
As suggested in #125006. Depending on which PR lands first, I'll update
TestCommandInterepterPrintCallback.pyto check that theCommandReturnObjectpassed to the callback has the correct command.