https://github.com/JDevlieghere created https://github.com/llvm/llvm-project/pull/196589
Add a debug-mode assert in the leaf CommandObjectParsed and CommandObjectRaw Execute paths that fires if DoExecute returns without calling SetStatus on the CommandReturnObject. Forgetting to set a status leaves the object in an undefined state and silently misreports success/failure to callers. rdar://176506732 >From 276aa8baf3d3fc6c2a06cf973650d4ece0a143de Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Fri, 8 May 2026 10:32:59 -0700 Subject: [PATCH] [lldb] Assert that CommandObject::DoExecute sets a return status Add a debug-mode assert in the leaf CommandObjectParsed and CommandObjectRaw Execute paths that fires if DoExecute returns without calling SetStatus on the CommandReturnObject. Forgetting to set a status leaves the object in an undefined state and silently misreports success/failure to callers. rdar://176506732 --- .../lldb/Interpreter/CommandReturnObject.h | 12 ++++ lldb/source/Interpreter/CommandObject.cpp | 25 ++++++++- .../Interpreter/CommandReturnObject.cpp | 6 +- lldb/unittests/Interpreter/CMakeLists.txt | 1 + .../Interpreter/TestCommandReturnObject.cpp | 55 +++++++++++++++++++ 5 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 lldb/unittests/Interpreter/TestCommandReturnObject.cpp diff --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h b/lldb/include/lldb/Interpreter/CommandReturnObject.h index 743bd94f5d73e..55fabe508e968 100644 --- a/lldb/include/lldb/Interpreter/CommandReturnObject.h +++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h @@ -163,6 +163,17 @@ class CommandReturnObject { void SetStatus(lldb::ReturnStatus status); + /// Returns true if SetStatus has been called since construction or the last + /// Clear(). Used to assert that commands explicitly report a status rather + /// than silently leaving the initial value in place. + bool WasStatusSet() const { return m_status_set; } + + /// Clears the "was set" flag without modifying the status value. Used by + /// the Execute machinery to scope the WasStatusSet() check to a single + /// DoExecute call without disturbing the status value seen by helpers + /// running inside DoExecute. + void ResetStatusSetFlag() { m_status_set = false; } + bool Succeeded() const; bool HasResult() const; @@ -190,6 +201,7 @@ class CommandReturnObject { std::optional<uint16_t> m_diagnostic_indent; lldb::ReturnStatus m_status = lldb::eReturnStatusStarted; + bool m_status_set = false; /// An optionally empty list of values produced by this command. ValueObjectList m_value_objects; diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp index 3b3b2d7a302d9..dec899a94970e 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 was set" flag on entry and +/// asserts on exit that DoExecute called SetStatus (directly or via +/// AppendError/SetError, which call SetStatus internally). +class DoExecuteStatusCheck { +public: + explicit DoExecuteStatusCheck(CommandReturnObject &result) + : m_result(result) { + m_result.ResetStatusSetFlag(); + } + ~DoExecuteStatusCheck() { + assert(m_result.WasStatusSet() && + "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..665153ed5972b 100644 --- a/lldb/source/Interpreter/CommandReturnObject.cpp +++ b/lldb/source/Interpreter/CommandReturnObject.cpp @@ -159,7 +159,10 @@ StructuredData::ObjectSP CommandReturnObject::GetErrorData() { return Serialize(m_diagnostics); } -void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; } +void CommandReturnObject::SetStatus(ReturnStatus status) { + m_status = status; + m_status_set = true; +} ReturnStatus CommandReturnObject::GetStatus() const { return m_status; } @@ -182,6 +185,7 @@ void CommandReturnObject::Clear() { static_cast<StreamString *>(stream_sp.get())->Clear(); m_diagnostics.clear(); m_status = eReturnStatusStarted; + m_status_set = false; m_did_change_process_state = false; m_suppress_immediate_output = false; m_interactive = true; 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..c4e23e10a45a6 --- /dev/null +++ b/lldb/unittests/Interpreter/TestCommandReturnObject.cpp @@ -0,0 +1,55 @@ +//===-- 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, WasStatusSetInitiallyFalse) { + CommandReturnObject result(/*colors=*/false); + EXPECT_FALSE(result.WasStatusSet()); +} + +TEST(CommandReturnObjectTest, SetStatusSetsFlag) { + CommandReturnObject result(false); + result.SetStatus(eReturnStatusSuccessFinishResult); + EXPECT_TRUE(result.WasStatusSet()); + EXPECT_EQ(result.GetStatus(), eReturnStatusSuccessFinishResult); +} + +TEST(CommandReturnObjectTest, AppendErrorSetsFlag) { + CommandReturnObject result(false); + result.AppendError("boom"); + EXPECT_TRUE(result.WasStatusSet()); + EXPECT_EQ(result.GetStatus(), eReturnStatusFailed); +} + +TEST(CommandReturnObjectTest, ClearResetsFlag) { + CommandReturnObject result(false); + result.SetStatus(eReturnStatusSuccessFinishResult); + ASSERT_TRUE(result.WasStatusSet()); + result.Clear(); + EXPECT_FALSE(result.WasStatusSet()); + EXPECT_EQ(result.GetStatus(), eReturnStatusStarted); +} + +TEST(CommandReturnObjectTest, ResetStatusSetFlagPreservesStatus) { + CommandReturnObject result(false); + result.SetStatus(eReturnStatusSuccessFinishResult); + ASSERT_TRUE(result.WasStatusSet()); + ASSERT_EQ(result.GetStatus(), eReturnStatusSuccessFinishResult); + + result.ResetStatusSetFlag(); + EXPECT_FALSE(result.WasStatusSet()); + // Status itself must be untouched so helpers reading Succeeded()/GetStatus() + // inside DoExecute still see what was pre-seeded by ParseOptions. + EXPECT_EQ(result.GetStatus(), eReturnStatusSuccessFinishResult); + EXPECT_TRUE(result.Succeeded()); +} _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
