Fix a crash when a stop hook deletes itself in its callback. #160416
Conversation
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesWe're iterating over the stop hooks so if one of them changes the stop hook list by deleting itself or another stop hook, that invalidates our iterator. I chose to fix this by making a copy of the stop hook list and iterating over that. That's a cheap operation since this is just an array of shared pointers. But more importantly doing it this way means that if on a stop where one stop hook deletes another, the deleted hook will always get a chance to run. If you iterated over the list itself, then whether that to be deleted hook gets to run would be dependent on whether it was before or after the deleting stop hook, which would be confusing. Full diff: https://github.com/llvm/llvm-project/pull/160416.diff 3 Files Affected:
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index fa98c24606492..e94fe854f8157 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3171,7 +3171,13 @@ bool Target::RunStopHooks(bool at_initial_stop) {
bool should_stop = false;
bool requested_continue = false;
- for (auto stop_entry : m_stop_hooks) {
+ // A stop hook might get deleted while running stop hooks.
+ // We have to decide what that means. We will follow the rule that deleting
+ // a stop hook while processing these stop hooks will delete it for FUTURE
+ // stops but not this stop. The easiest way to do that is to copy the
+ // stop hooks and iterate over the copy.
+ StopHookCollection stop_hooks_copy = m_stop_hooks;
+ for (auto stop_entry : stop_hooks_copy) {
StopHookSP cur_hook_sp = stop_entry.second;
if (!cur_hook_sp->IsActive())
continue;
diff --git a/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py b/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
index 954cac1592435..c0e3c2a8fcf31 100644
--- a/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
+++ b/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
@@ -48,6 +48,39 @@ def test_bad_handler(self):
"Got the right error",
)
+ def test_self_deleting(self):
+ """Test that we can handle a stop hook that deletes itself"""
+ self.script_setup()
+ # Run to the first breakpoint before setting the stop hook
+ # so we don't have to figure out where it showed up in the new
+ # target.
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "Stop here first", self.main_source_file
+ )
+
+ # Now add our stop hook and register it:
+ result = lldb.SBCommandReturnObject()
+ command = "target stop-hook add -P stop_hook.self_deleting_stop"
+ self.interp.HandleCommand(command, result)
+ self.assertCommandReturn(result, f"Added my stop hook: {result.GetError()}")
+
+ result_str = result.GetOutput()
+ p = re.compile("Stop hook #([0-9]+) added.")
+ m = p.match(result_str)
+ current_stop_hook_id = m.group(1)
+ command = "command script add -o -f stop_hook.handle_stop_hook_id handle_id"
+ self.interp.HandleCommand(command, result)
+ self.assertCommandReturn(result, "Added my command")
+
+ command = f"handle_id {current_stop_hook_id}"
+ self.interp.HandleCommand(command, result)
+ self.assertCommandReturn(result, "Registered my stop ID")
+
+ # Now step the process and make sure the stop hook was deleted.
+ thread.StepOver()
+ self.interp.HandleCommand("target stop-hook list", result)
+ self.assertEqual(result.GetOutput().rstrip(), "No stop hooks.", "Deleted hook")
+
def test_stop_hooks_scripted(self):
"""Test that a scripted stop hook works with no specifiers"""
self.stop_hooks_scripted(5, "-I false")
diff --git a/lldb/test/API/commands/target/stop-hooks/stop_hook.py b/lldb/test/API/commands/target/stop-hooks/stop_hook.py
index cb7a4337c40d4..142c4ab4c5d89 100644
--- a/lldb/test/API/commands/target/stop-hooks/stop_hook.py
+++ b/lldb/test/API/commands/target/stop-hooks/stop_hook.py
@@ -48,3 +48,27 @@ def handle_stop(self):
class no_handle_stop:
def __init__(self, target, extra_args, dict):
print("I am okay")
+
+class self_deleting_stop:
+ def __init__(self, target, extra_args, dict):
+ self.target = target
+
+ def handle_stop(self, exe_ctx, stream):
+ interp = exe_ctx.target.debugger.GetCommandInterpreter()
+ result = lldb.SBCommandReturnObject()
+ interp.HandleCommand("handle_id", result)
+ id_str = result.GetOutput().rstrip()
+
+ command = f"target stop-hook delete {id_str}"
+ interp.HandleCommand(command, result)
+
+stop_hook_id = 0
+def handle_stop_hook_id(debugger, command, exe_ctx, result, extra_args):
+ global stop_hook_id
+ if command == "":
+ result.AppendMessage(str(stop_hook_id))
+ else:
+ stop_hook_id = int(command)
+
+
+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
|
I think it could be a good idea to write a little blurb about this behavior in the documentation somewhere. If you asked me what I thought the behavior of stop hooks would be when one removes another, I don't know if I could guess correctly. A cursor search gave me a "Extending Target Stop-Hooks" tutorial. There may be a better place to put it, I'm not sure though. |
That's a good idea. I put it in the "target stop-hook delete" help, since stop hooks can be defined just with commands, and so could delete one another w/o any Python in the picture. |
|
This dropped off my queue somehow. In that time, in the process of adding support for internal stop hooks, the iteration over stop hooks was changed so that first we build a list of the "active stop hooks" and then only iterate over those when running the hooks. That has the same effect as this patch of iterating over a copy, not over the ivar list, and thus always iterating over the whole set of hooks valid at the time we stopped. |
That is to say, this patch is now purely documentative? I looked at the changes and my approval still stands. |
…0416) We're iterating over the stop hooks so if one of them changes the stop hook list by deleting itself or another stop hook, that invalidates our iterator. I chose to fix this by making a copy of the stop hook list and iterating over that. That's a cheap operation since this is just an array of shared pointers. But more importantly doing it this way means that if on a stop where one stop hook deletes another, the deleted hook will always get a chance to run. If you iterated over the list itself, then whether that to be deleted hook gets to run would be dependent on whether it was before or after the deleting stop hook, which would be confusing.
4 participants
We're iterating over the stop hooks so if one of them changes the stop hook list by deleting itself or another stop hook, that invalidates our iterator.
I chose to fix this by making a copy of the stop hook list and iterating over that. That's a cheap operation since this is just an array of shared pointers. But more importantly doing it this way means that if on a stop where one stop hook deletes another, the deleted hook will always get a chance to run. If you iterated over the list itself, then whether that to be deleted hook gets to run would be dependent on whether it was before or after the deleting stop hook, which would be confusing.