llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: alexey-gusarov <details> <summary>Changes</summary> Fixes part (1) of #<!-- -->215189. `ScriptedThreadPlanPythonInterface::GetRunState()` reads the script's `should_step` answer with `GetUnsignedIntegerValue()`. A Python `bool` arrives as a `StructuredData::Boolean` -- `PythonObject::GetObjectType()` tests `PythonBoolean::Check` before `PythonInteger::Check` -- and that accessor returns its fail value for anything that is not an `Integer`. The answer was discarded on every call and every scripted plan reported `eStateStepping`, so a plan returning `False` single-stepped exactly like one returning `True`. @<!-- -->jimingham on the issue: *"The ScriptedInterface changed the return type of should_step from a boolean to an lldb::StateType. `should_anythings` should return bools. That was probably just a little thinko when doing the conversion."* That is exactly it, and it is what the replaced code did: before 9a9ec228cdcf the SWIG bridge compared the result against `Py_True`/`Py_False` and returned a bool, which `ScriptedThreadPlanGetRunState()` mapped with `should_step ? eStateStepping : eStateRunning`. This keeps `GetRunState()`'s signature -- its callers want a `StateType` -- and reads the answer as the bool it is. The contract is documented in `lldb/docs/use/tutorials/automating-stepping-logic.md`: *"Return `True` if you want lldb to instruction step one instruction, or False to continue till the next breakpoint is hit."* — and `lldb/examples/python/templates/scripted_thread_plan.py` says the same in its own words, down to the `-> bool` annotation. ### Every return type, measured | `should_step` returns | before | after | |---|---|---| | `True` | step | step | | `False` | step | **run** | | `0` or `lldb.eStateRunning` | **run** | step | | `None`, or no such method | step | step | **Row 3 is a behaviour this changes rather than fixes, and it is the one thing to weigh.** An integer return used to be reinterpreted as a `StateType`, which before this fix was the only way to make a scripted plan run at all -- i.e. the natural workaround for this very bug. The documented contract is a bool, so it goes, but it now leaves a log line instead of being dropped silently. If honouring integers matters, read `GetAsBoolean()` first and fall back to `GetUnsignedIntegerValue()`; say the word and I will. One adjacent difference deliberately left alone: 9a9ec228cdcf also inverted the *missing* `should_step` case -- the old bridge returned `false` (run) when the method was absent (`if (!pfunc.IsAllocated()) return false;`, without setting `got_error`), and today the early return gives `eStateStepping`. This PR keeps stepping there; it is a separate behaviour question from the one the issue reports. ### Testing `lldb/test/API/functionalities/step_scripted/` gains three methods: a pair of top-of-stack plans differing only in whether `should_step` returns `True` or `False`, and one pinning the deliberate change in row 3 — a plan returning an int now steps. Before the fix the `False` one fails with `plancomplete (8) != breakpoint (3)` and the int one with `1 != 0`, on Linux and on Windows alike. Whole `check-lldb` on Linux x86_64 with this commit alone, compared per test id against `main`: 34465 ids, **no id changed verdict**, 0 failed, 0 unresolved. (An earlier round of this arm hit one unrelated flake once -- `python_api/sbplatform/TestSBPlatform.py`, a `platform connect` socket race; 10 re-runs on that build, a second full suite run, and the final arm are all green.) ### Note on landing order This commit wakes the `SetAutoContinue(true)` branch in `Thread::SetupToStepOverBreakpointIfNeeded()` for scripted plans -- it is reached only when the plan below wants to run, which a scripted plan could not say before. That is the path #<!-- -->215522 (part (2) of #<!-- -->215189) fixes. Nothing in the in-tree suite moves either way (measured, above), but the two are best landed together. --- Full diff: https://github.com/llvm/llvm-project/pull/215521.diff 4 Files Affected: - (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp (+26-2) - (modified) lldb/test/API/functionalities/step_scripted/Steps.py (+32) - (modified) lldb/test/API/functionalities/step_scripted/TestStepScripted.py (+72) - (modified) lldb/test/API/functionalities/step_scripted/main.c (+1-1) ``````````diff diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp index b18823ea60960..b887ede843e5b 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.cpp @@ -10,7 +10,9 @@ #include "lldb/Core/PluginManager.h" #include "lldb/Target/ThreadPlan.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" +#include "lldb/Utility/StreamString.h" #include "lldb/lldb-enumerations.h" #include "../SWIGPythonBridge.h" @@ -86,8 +88,30 @@ lldb::StateType ScriptedThreadPlanPythonInterface::GetRunState() { error)) return lldb::eStateStepping; - return static_cast<lldb::StateType>(obj->GetUnsignedIntegerValue( - static_cast<uint32_t>(lldb::eStateStepping))); + // `should_step` answers a question, it does not return a StateType: the + // documented contract is "Return `True` if you want lldb to instruction step + // one instruction, or False to continue till the next breakpoint is hit" + // (lldb/docs/use/tutorials/automating-stepping-logic.md). A Python bool + // arrives here as a + // StructuredData::Boolean, and GetUnsignedIntegerValue() returns its fail + // value for anything that is not an Integer -- so reading the answer as a + // StateType discarded it, and a plan returning False single-stepped exactly + // like one returning True. + if (StructuredData::Boolean *should_step = obj->GetAsBoolean()) + return should_step->GetValue() ? lldb::eStateStepping : lldb::eStateRunning; + + // Anything else is not an answer to the question that was asked. Step, as + // this function has always done when it could not read a reply -- but say so + // in the log, because an int return used to be reinterpreted as a StateType + // and is the one thing whose behaviour changes here. + if (Log *log = GetLog(LLDBLog::Script)) { + StreamString returned; + obj->Dump(returned, /*pretty_print=*/false); + LLDB_LOG(log, + "{0}: should_step returned {1}, which is not a bool; stepping.", + LLVM_PRETTY_FUNCTION, returned.GetData()); + } + return lldb::eStateStepping; } llvm::Error diff --git a/lldb/test/API/functionalities/step_scripted/Steps.py b/lldb/test/API/functionalities/step_scripted/Steps.py index b36bb5f5f9048..768733bd48865 100644 --- a/lldb/test/API/functionalities/step_scripted/Steps.py +++ b/lldb/test/API/functionalities/step_scripted/Steps.py @@ -140,3 +140,35 @@ def should_step(self): def explains_stop(self, event): return True + + +# These two are top of the plan stack -- they queue no child plan -- so the +# thread's run state comes from their should_step, and that is the only thing +# that differs between them. +class RunToNextBreakpoint: + def __init__(self, thread_plan, args_data): + self.thread_plan = thread_plan + + def explains_stop(self, event): + return False + + def should_stop(self, event): + self.thread_plan.SetPlanComplete(True) + return True + + def should_step(self): + return False + + +class StepOneInstruction(RunToNextBreakpoint): + def should_step(self): + return True + + +class ReturnsAnIntFromShouldStep(RunToNextBreakpoint): + """should_step's contract is a bool. An int is not an answer, and the plan + steps -- which it did not always do: the int used to be reinterpreted as a + StateType.""" + + def should_step(self): + return 0 diff --git a/lldb/test/API/functionalities/step_scripted/TestStepScripted.py b/lldb/test/API/functionalities/step_scripted/TestStepScripted.py index a08a39a710596..126c619a4acf2 100644 --- a/lldb/test/API/functionalities/step_scripted/TestStepScripted.py +++ b/lldb/test/API/functionalities/step_scripted/TestStepScripted.py @@ -45,6 +45,78 @@ def test_constructor_error_preserves_traceback(self): "ValueError: scripted plan construction failed", result.GetError() ) + def test_should_step_false_runs_to_the_next_breakpoint(self): + """A scripted plan whose should_step returns False must let the process + run until the next breakpoint, not single-step one instruction.""" + self.build() + (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( + self, "Set a breakpoint here", self.main_source_file + ) + second = target.BreakpointCreateBySourceRegex( + "Run to this breakpoint", self.main_source_file + ) + self.assertTrue(second.GetNumLocations() > 0, VALID_BREAKPOINT) + + err = thread.StepUsingScriptedThreadPlan("Steps.RunToNextBreakpoint") + self.assertSuccess(err) + + self.assertStopReason( + thread.GetStopReason(), + lldb.eStopReasonBreakpoint, + "should_step returned False, so the thread should have run on to " + "the next breakpoint", + ) + self.assertEqual(second.GetHitCount(), 1) + + def test_should_step_true_steps_one_instruction(self): + """The sibling of the test above: returning True from should_step still + single-steps, so the later breakpoint is not reached.""" + self.build() + (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( + self, "Set a breakpoint here", self.main_source_file + ) + second = target.BreakpointCreateBySourceRegex( + "Run to this breakpoint", self.main_source_file + ) + self.assertTrue(second.GetNumLocations() > 0, VALID_BREAKPOINT) + + err = thread.StepUsingScriptedThreadPlan("Steps.StepOneInstruction") + self.assertSuccess(err) + + self.assertEqual( + second.GetHitCount(), + 0, + "should_step returned True, so the thread should not have run as " + "far as the next breakpoint", + ) + self.assertStopReason( + thread.GetStopReason(), + lldb.eStopReasonPlanComplete, + "the scripted plan should have completed after one instruction", + ) + + def test_should_step_returning_an_int_steps(self): + """should_step answers a question with a bool; anything else is not an + answer, and the plan steps rather than running free.""" + self.build() + (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( + self, "Set a breakpoint here", self.main_source_file + ) + second = target.BreakpointCreateBySourceRegex( + "Run to this breakpoint", self.main_source_file + ) + self.assertTrue(second.GetNumLocations() > 0, VALID_BREAKPOINT) + + err = thread.StepUsingScriptedThreadPlan("Steps.ReturnsAnIntFromShouldStep") + self.assertSuccess(err) + + self.assertEqual(second.GetHitCount(), 0) + self.assertStopReason( + thread.GetStopReason(), + lldb.eStopReasonPlanComplete, + "the plan should have completed after one instruction", + ) + def step_out_with_scripted_plan(self, name): (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint( self, "Set a breakpoint here", self.main_source_file diff --git a/lldb/test/API/functionalities/step_scripted/main.c b/lldb/test/API/functionalities/step_scripted/main.c index 9023120c44312..75497be1e377a 100644 --- a/lldb/test/API/functionalities/step_scripted/main.c +++ b/lldb/test/API/functionalities/step_scripted/main.c @@ -4,7 +4,7 @@ void foo() { int foo = 10; printf("%d\n", foo); // Set a breakpoint here. foo = 20; - printf("%d\n", foo); + printf("%d\n", foo); // Run to this breakpoint. } int main() { `````````` </details> https://github.com/llvm/llvm-project/pull/215521 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
