llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Med Ismail Bennani (medismailben) <details> <summary>Changes</summary> `CommandObjectTargetFrameProviderRegister::DoExecute` never called `CommandReturnObject::SetStatus()` on its success path. `CommandObject.cpp` has a `DoExecuteStatusCheck` RAII guard that resets the result's status to `eReturnStatusInvalid` before `DoExecute` runs, and asserts on exit that `DoExecute` changed it. `AppendMessage()`/`AppendMessageWithFormatv()` don't touch status (unlike AppendError()/SetError(), which call `SetStatus(eReturnStatusFailed)`), so on the success path the status stayed eReturnStatusInvalid, tripping the assert. This went unnoticed because every existing `scripted_frame_provider` test uses `SBTarget::RegisterScriptedFrameProvider` directly, bypassing the `target frame-provider register` command entirely. Add a regression test that exercises the command instead. --- Full diff: https://github.com/llvm/llvm-project/pull/208232.diff 5 Files Affected: - (modified) lldb/source/Commands/CommandObjectTarget.cpp (+1) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_status/Makefile (+3) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_status/TestFrameProviderRegisterCommandStatus.py (+41) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_status/frame_provider.py (+16) - (added) lldb/test/API/functionalities/scripted_frame_provider/register_command_status/main.c (+7) ``````````diff diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp index 28065a47fd413..4ef3a6fe82115 100644 --- a/lldb/source/Commands/CommandObjectTarget.cpp +++ b/lldb/source/Commands/CommandObjectTarget.cpp @@ -6175,6 +6175,7 @@ class CommandObjectTargetFrameProviderRegister : public CommandObjectParsed { result.AppendMessageWithFormatv( "successfully registered scripted frame provider '{0}' for target", m_class_options.GetName().c_str()); + result.SetStatus(eReturnStatusSuccessFinishResult); } OptionGroupPythonClassWithDict m_class_options; diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/Makefile b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/Makefile new file mode 100644 index 0000000000000..0b710c6e298ae --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/Makefile @@ -0,0 +1,3 @@ +C_SOURCES := main.c +CFLAGS_EXTRAS := -std=c99 +include Makefile.rules diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/TestFrameProviderRegisterCommandStatus.py b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/TestFrameProviderRegisterCommandStatus.py new file mode 100644 index 0000000000000..06287cadcf182 --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/TestFrameProviderRegisterCommandStatus.py @@ -0,0 +1,41 @@ +""" +Test that `target frame-provider register` succeeds without asserting. + +CommandObjectTargetFrameProviderRegister::DoExecute never called +CommandReturnObject::SetStatus() on its success path. CommandObject.cpp +has a DoExecuteStatusCheck RAII guard that asserts DoExecute always sets a +status; since AppendMessage()/AppendMessageWithFormatv() don't touch +status (unlike AppendError()/SetError(), which do), the status stayed +eReturnStatusInvalid on success, tripping the assert. + +This regression went unnoticed because every other scripted_frame_provider +test calls SBTarget::RegisterScriptedFrameProvider directly, bypassing +this command entirely. +""" + +import os +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class TestFrameProviderRegisterCommandStatus(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + def test_register_command_succeeds(self): + """ + `target frame-provider register` should complete successfully + (and not assert) when given a valid scripted frame provider class. + """ + self.build() + + lldbutil.run_to_name_breakpoint(self, "frame3") + + provider_path = os.path.join(self.getSourceDir(), "frame_provider.py") + self.runCmd("command script import " + provider_path) + + self.expect( + "target frame-provider register -C frame_provider.MinimalProvider", + substrs=["successfully registered scripted frame provider"], + ) diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/frame_provider.py b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/frame_provider.py new file mode 100644 index 0000000000000..b66060bc8a77f --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/frame_provider.py @@ -0,0 +1,16 @@ +""" +Minimal scripted frame provider used only to exercise `target +frame-provider register`. get_frame_at_index is never invoked by this +test: registration alone is enough to trigger the bug under test. +""" + +from lldb.plugins.scripted_frame_provider import ScriptedFrameProvider + + +class MinimalProvider(ScriptedFrameProvider): + @staticmethod + def get_description(): + return "minimal provider" + + def get_frame_at_index(self, index): + return None diff --git a/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/main.c b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/main.c new file mode 100644 index 0000000000000..1aa56e3eddf7a --- /dev/null +++ b/lldb/test/API/functionalities/scripted_frame_provider/register_command_status/main.c @@ -0,0 +1,7 @@ +int frame3() { return 3; } + +int frame2() { return frame3(); } + +int frame1() { return frame2(); } + +int main() { return frame1(); } `````````` </details> https://github.com/llvm/llvm-project/pull/208232 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
