Author: Jonas Devlieghere Date: 2026-05-11T13:58:02-05:00 New Revision: 78d124eb16aa62e02a465b0fd6c3c2cab0a26dd8
URL: https://github.com/llvm/llvm-project/commit/78d124eb16aa62e02a465b0fd6c3c2cab0a26dd8 DIFF: https://github.com/llvm/llvm-project/commit/78d124eb16aa62e02a465b0fd6c3c2cab0a26dd8.diff LOG: [lldb] Assert that CommandObject::DoExecute sets a return status (#196589) Change the default value of CommandReturnObject::m_status from eReturnStatusStarted to eReturnStatusInvalid, and add a debug-only RAII check in CommandObjectParsed::Execute and CommandObjectRaw::Execute that asserts the status is no longer Invalid after DoExecute returns. This catches commands that forget to call SetStatus on a success or failure path. Succeeded() still returns true when the status is Invalid (0 sorts below eReturnStatusSuccessContinuingResult), so helpers that read result.Succeeded() as a precondition before any explicit SetStatus (e.g. StopProcessIfNecessary) continue to work. rdar://176506732 Added: lldb/unittests/Interpreter/TestCommandReturnObject.cpp Modified: lldb/include/lldb/Interpreter/CommandReturnObject.h lldb/source/Commands/CommandObjectProcess.cpp lldb/source/Commands/CommandObjectProtocolServer.cpp lldb/source/Interpreter/CommandObject.cpp lldb/source/Interpreter/CommandReturnObject.cpp lldb/test/API/commands/command/script/TestCommandScript.py lldb/unittests/Interpreter/CMakeLists.txt Removed: ################################################################################ diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h index 743bd94f5d73e..dccb98cd2be90 100644 --- a/lldb/include/lldb/Interpreter/CommandReturnObject.h +++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h @@ -189,7 +189,10 @@ class CommandReturnObject { std::vector<DiagnosticDetail> m_diagnostics; std::optional<uint16_t> m_diagnostic_indent; - lldb::ReturnStatus m_status = lldb::eReturnStatusStarted; + /// The command's return status indicating success or failure. The default + /// value indicate no status has been set, which is enforced by an assert in + /// the CommandInterpreter. + lldb::ReturnStatus m_status = lldb::eReturnStatusInvalid; /// An optionally empty list of values produced by this command. ValueObjectList m_value_objects; diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index 551f98566a9a5..d7d6a9152e377 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -927,7 +927,9 @@ class CommandObjectProcessConnect : public CommandObjectParsed { error); if (error.Fail() || process_sp == nullptr) { result.AppendError(error.AsCString("Error connecting to the process")); + return; } + result.SetStatus(eReturnStatusSuccessFinishResult); } CommandOptions m_options; diff --git a/lldb/source/Commands/CommandObjectProtocolServer.cpp b/lldb/source/Commands/CommandObjectProtocolServer.cpp index 1a950899ea1c0..73f717660e47d 100644 --- a/lldb/source/Commands/CommandObjectProtocolServer.cpp +++ b/lldb/source/Commands/CommandObjectProtocolServer.cpp @@ -92,8 +92,8 @@ class CommandObjectProtocolServerStart : public CommandObjectParsed { result.AppendMessageWithFormatv( "{0} server started with connection listeners: {1}", protocol, address); - result.SetStatus(eReturnStatusSuccessFinishNoResult); } + result.SetStatus(eReturnStatusSuccessFinishNoResult); } }; @@ -128,6 +128,7 @@ class CommandObjectProtocolServerStop : public CommandObjectParsed { result.AppendErrorWithFormatv("{0}", llvm::fmt_consume(std::move(error))); return; } + result.SetStatus(eReturnStatusSuccessFinishNoResult); } }; diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp index 3b3b2d7a302d9..dbdfc66204660 100644 --- a/lldb/source/Interpreter/CommandObject.cpp +++ b/lldb/source/Interpreter/CommandObject.cpp @@ -37,6 +37,26 @@ using namespace lldb; using namespace lldb_private; +namespace { +/// RAII scope that resets the result's status to eReturnStatusInvalid on entry +/// and asserts on exit that DoExecute changed it (directly via SetStatus, or +/// indirectly via AppendError/SetError, which call SetStatus internally). +class DoExecuteStatusCheck { +public: + explicit DoExecuteStatusCheck(CommandReturnObject &result) + : m_result(result) { + m_result.SetStatus(eReturnStatusInvalid); + } + ~DoExecuteStatusCheck() { + assert(m_result.GetStatus() != eReturnStatusInvalid && + "DoExecute did not set a status on the CommandReturnObject"); + } + +private: + CommandReturnObject &m_result; +}; +} // namespace + // CommandObject CommandObject::CommandObject(CommandInterpreter &interpreter, @@ -825,6 +845,7 @@ void CommandObjectParsed::Execute(const char *args_string, return; } m_interpreter.IncreaseCommandUsage(*this); + DoExecuteStatusCheck check(result); DoExecute(cmd_args, result); } } @@ -845,8 +866,10 @@ void CommandObjectRaw::Execute(const char *args_string, handled = InvokeOverrideCallback(argv, result); } if (!handled) { - if (CheckRequirements(result)) + if (CheckRequirements(result)) { + DoExecuteStatusCheck check(result); DoExecute(args_string, result); + } Cleanup(); } diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp index a6b0d56c66cca..c21b9b3a92c05 100644 --- a/lldb/source/Interpreter/CommandReturnObject.cpp +++ b/lldb/source/Interpreter/CommandReturnObject.cpp @@ -181,7 +181,7 @@ void CommandReturnObject::Clear() { if (stream_sp) static_cast<StreamString *>(stream_sp.get())->Clear(); m_diagnostics.clear(); - m_status = eReturnStatusStarted; + m_status = eReturnStatusInvalid; m_did_change_process_state = false; m_suppress_immediate_output = false; m_interactive = true; diff --git a/lldb/test/API/commands/command/script/TestCommandScript.py b/lldb/test/API/commands/command/script/TestCommandScript.py index fdd5216a1c6cc..eb1584c64c90d 100644 --- a/lldb/test/API/commands/command/script/TestCommandScript.py +++ b/lldb/test/API/commands/command/script/TestCommandScript.py @@ -214,8 +214,8 @@ def test_persistence(self): # valid. self.expect("script str(persistence.debugger_copy)", substrs=[str(self.dbg)]) # The result object will be replaced by an empty result object (in the - # "Started" state). - self.expect("script str(persistence.result_copy)", substrs=["Started"]) + # default "Invalid" state). + self.expect("script str(persistence.result_copy)", substrs=["Invalid"]) def test_interactive(self): """ diff --git a/lldb/unittests/Interpreter/CMakeLists.txt b/lldb/unittests/Interpreter/CMakeLists.txt index d4ba5b3d58334..7eec76105aad2 100644 --- a/lldb/unittests/Interpreter/CMakeLists.txt +++ b/lldb/unittests/Interpreter/CMakeLists.txt @@ -1,5 +1,6 @@ add_lldb_unittest(InterpreterTests TestCommandPaths.cpp + TestCommandReturnObject.cpp TestCompletion.cpp TestOptionArgParser.cpp TestOptions.cpp diff --git a/lldb/unittests/Interpreter/TestCommandReturnObject.cpp b/lldb/unittests/Interpreter/TestCommandReturnObject.cpp new file mode 100644 index 0000000000000..a8c66958e5d06 --- /dev/null +++ b/lldb/unittests/Interpreter/TestCommandReturnObject.cpp @@ -0,0 +1,38 @@ +//===-- TestCommandReturnObject.cpp ---------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Interpreter/CommandReturnObject.h" +#include "gtest/gtest.h" + +using namespace lldb; +using namespace lldb_private; + +TEST(CommandReturnObjectTest, DefaultStatusIsInvalid) { + CommandReturnObject result(/*colors=*/false); + EXPECT_EQ(result.GetStatus(), eReturnStatusInvalid); +} + +TEST(CommandReturnObjectTest, SetStatusUpdatesStatus) { + CommandReturnObject result(false); + result.SetStatus(eReturnStatusSuccessFinishResult); + EXPECT_EQ(result.GetStatus(), eReturnStatusSuccessFinishResult); +} + +TEST(CommandReturnObjectTest, AppendErrorSetsFailed) { + CommandReturnObject result(false); + result.AppendError("boom"); + EXPECT_EQ(result.GetStatus(), eReturnStatusFailed); +} + +TEST(CommandReturnObjectTest, ClearResetsToInvalid) { + CommandReturnObject result(false); + result.SetStatus(eReturnStatusSuccessFinishResult); + ASSERT_EQ(result.GetStatus(), eReturnStatusSuccessFinishResult); + result.Clear(); + EXPECT_EQ(result.GetStatus(), eReturnStatusInvalid); +} _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
