jingham created this revision.
jingham added reviewers: teemperor, labath.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
jingham requested review of this revision.
This was originally done by checking the private state to see if it was
"eStateRunning" or not. If it is running we need mark the stop event we're
currently processing as "restarted" so the code like Process::ResumeSynchronous
will know to keep waiting for a real stop.
But that is racy because between the time that the process gets restarted and
the time we do the check, the process could have stopped again.
I used the private state because where the stop-hooks are run we are in the
process of computing Public state so it won't show the effect of the restart
yet.
Instead, this patch switches to just asking the stop hook whether it ran or
not, and then marking the stop event as restarted or not based on the answer.
The CommandLine stop hooks know this definitively - or at least they do
provided the command return status is returned correctly. The Python stop
hooks shouldn't be directly restarting the target, they should use the return
values to request a restart.
I'll deal later on with the cases where (a) a command returns the wrong status
and (b) somebody runs Continue/etc in a scripted stop hook even though they
shouldn't by adding a mode to the Process where the high level functions that
could run the target don't but instead mark an intention to continue the
target. We will turn that on when we start processing stop actions, and then
when we're done with all the stop actions, check whether everybody agrees we
should continue.
That will also solve the unfairness problem in the stop actions, where the
first stop action that runs a "continue" command, or an SB API that continues
the target causes all the other stop actions don't get a chance to run. I've
added auto-continue flags and for python return values to request running so
that you CAN do the right thing. But it would be better to make the right
thing happen automatically.
But I'm leaving that for a separate patch because it's a much bigger chunk of
work, and this should be sufficient for now - only failing when people do
things they shouldn't do...
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D88753
Files:
lldb/include/lldb/Target/Target.h
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp
lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
Index: lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
===================================================================
--- lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
+++ lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
@@ -71,8 +71,6 @@
"""Test that the returning False from a stop hook works"""
self.do_test_auto_continue(True)
- # Test is flakey on Linux.
- @skipIfLinux
def do_test_auto_continue(self, return_true):
"""Test that auto-continue works."""
# We set auto-continue to 1 but the stop hook only applies to step_out_of_me,
Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2541,25 +2541,26 @@
}
}
-void Target::RunStopHooks() {
+bool Target::RunStopHooks() {
if (m_suppress_stop_hooks)
- return;
+ return false;
if (!m_process_sp)
- return;
+ return false;
// Somebody might have restarted the process:
+ // Still return false, the return value is about US restarting the target.
if (m_process_sp->GetState() != eStateStopped)
- return;
+ return false;
// <rdar://problem/12027563> make sure we check that we are not stopped
// because of us running a user expression since in that case we do not want
// to run the stop-hooks
if (m_process_sp->GetModIDRef().IsLastResumeForUserExpression())
- return;
+ return false;
if (m_stop_hooks.empty())
- return;
+ return false;
// If there aren't any active stop hooks, don't bother either.
bool any_active_hooks = false;
@@ -2570,7 +2571,7 @@
}
}
if (!any_active_hooks)
- return;
+ return false;
std::vector<ExecutionContext> exc_ctx_with_reasons;
@@ -2588,7 +2589,7 @@
// If no threads stopped for a reason, don't run the stop-hooks.
size_t num_exe_ctx = exc_ctx_with_reasons.size();
if (num_exe_ctx == 0)
- return;
+ return false;
StreamSP output_sp = m_debugger.GetAsyncOutputStream();
@@ -2636,22 +2637,27 @@
output_sp->Printf("-- Thread %d\n",
exc_ctx.GetThreadPtr()->GetIndexID());
- bool this_should_stop = cur_hook_sp->HandleStop(exc_ctx, output_sp);
- // If this hook is set to auto-continue that should override the
- // HandleStop result...
- if (cur_hook_sp->GetAutoContinue())
- this_should_stop = false;
+ StopHook::StopHookResult this_result =
+ cur_hook_sp->HandleStop(exc_ctx, output_sp);
+ bool this_should_stop = true;
- // If anybody wanted to stop, we should all stop.
- if (!should_stop)
- should_stop = this_should_stop;
+ switch (this_result) {
+ case StopHook::StopHookResult::KeepStopped:
+ // If this hook is set to auto-continue that should override the
+ // HandleStop result...
+ if (cur_hook_sp->GetAutoContinue())
+ this_should_stop = false;
+ else
+ this_should_stop = true;
- // We don't have a good way to prohibit people from restarting the target
- // willy nilly in a stop hook. So see if the private state is running
- // here and bag out if it is.
- // FIXME: when we are doing non-stop mode for realz we'll have to instead
- // track each thread, and only bag out if a thread is set running.
- if (m_process_sp->GetPrivateState() != eStateStopped) {
+ break;
+ case StopHook::StopHookResult::RequestContinue:
+ this_should_stop = false;
+ break;
+ case StopHook::StopHookResult::AlreadyContinued:
+ // We don't have a good way to prohibit people from restarting the
+ // target willy nilly in a stop hook. If the hook did so, give a
+ // gentle suggestion here and bag out if the hook processing.
output_sp->Printf("\nAborting stop hooks, hook %" PRIu64
" set the program running.\n"
" Consider using '-G true' to make "
@@ -2660,16 +2666,42 @@
somebody_restarted = true;
break;
}
+ // If we're already restarted, stop processing stop hooks.
+ // FIXME: if we are doing non-stop mode for real, we would have to
+ // check that OUR thread was restarted, otherwise we should keep
+ // processing stop hooks.
+ if (somebody_restarted)
+ break;
+
+ // If anybody wanted to stop, we should all stop.
+ if (!should_stop)
+ should_stop = this_should_stop;
}
}
output_sp->Flush();
+ // If one of the commands in the stop hook already restarted the target,
+ // report that fact.
+ if (somebody_restarted)
+ return true;
+
// Finally, if auto-continue was requested, do it now:
// We only compute should_stop against the hook results if a hook got to run
// which is why we have to do this conjoint test.
- if (!somebody_restarted && ((hooks_ran && !should_stop) || auto_continue))
- m_process_sp->PrivateResume();
+ if ((hooks_ran && !should_stop) || auto_continue) {
+ Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+ Status error = m_process_sp->PrivateResume();
+ if (error.Success()) {
+ LLDB_LOG(log, "Resuming from RunStopHooks");
+ return true;
+ } else {
+ LLDB_LOG(log, "Resuming from RunStopHooks failed: {0}", error);
+ return false;
+ }
+ }
+
+ return false;
}
const TargetPropertiesSP &Target::GetGlobalProperties() {
@@ -3235,13 +3267,14 @@
GetCommands().AppendString(string.c_str());
}
-bool Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
- StreamSP output_sp) {
+Target::StopHook::StopHookResult
+Target::StopHookCommandLine::HandleStop(ExecutionContext &exc_ctx,
+ StreamSP output_sp) {
assert(exc_ctx.GetTargetPtr() && "Can't call PerformAction on a context "
"with no target");
if (!m_commands.GetSize())
- return true;
+ return StopHookResult::KeepStopped;
CommandReturnObject result(false);
result.SetImmediateOutputStream(output_sp);
@@ -3260,8 +3293,11 @@
debugger.GetCommandInterpreter().HandleCommands(GetCommands(), &exc_ctx,
options, result);
debugger.SetAsyncExecution(old_async);
-
- return true;
+ lldb::ReturnStatus status = result.GetStatus();
+ if (status == eReturnStatusSuccessContinuingNoResult ||
+ status == eReturnStatusSuccessContinuingResult)
+ return StopHookResult::AlreadyContinued;
+ return StopHookResult::KeepStopped;
}
// Target::StopHookScripted
@@ -3289,20 +3325,22 @@
return error;
}
-bool Target::StopHookScripted::HandleStop(ExecutionContext &exc_ctx,
- StreamSP output_sp) {
+Target::StopHook::StopHookResult
+Target::StopHookScripted::HandleStop(ExecutionContext &exc_ctx,
+ StreamSP output_sp) {
assert(exc_ctx.GetTargetPtr() && "Can't call HandleStop on a context "
"with no target");
ScriptInterpreter *script_interp =
GetTarget()->GetDebugger().GetScriptInterpreter();
if (!script_interp)
- return true;
+ return StopHookResult::KeepStopped;
bool should_stop = script_interp->ScriptedStopHookHandleStop(
m_implementation_sp, exc_ctx, output_sp);
- return should_stop;
+ return should_stop ? StopHookResult::KeepStopped
+ : StopHookResult::RequestContinue;
}
void Target::StopHookScripted::GetSubclassDescription(
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -4178,8 +4178,7 @@
// public (or SyncResume) broadcasters. StopHooks are just for
// real public stops. They might also restart the target,
// so watch for that.
- process_sp->GetTarget().RunStopHooks();
- if (process_sp->GetPrivateState() == eStateRunning)
+ if (process_sp->GetTarget().RunStopHooks())
SetRestarted(true);
}
}
Index: lldb/include/lldb/Target/Target.h
===================================================================
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -1145,6 +1145,11 @@
virtual ~StopHook() = default;
enum class StopHookKind : uint32_t { CommandBased = 0, ScriptBased };
+ enum class StopHookResult : uint32_t {
+ KeepStopped = 0,
+ RequestContinue,
+ AlreadyContinued
+ };
lldb::TargetSP &GetTarget() { return m_target_sp; }
@@ -1160,8 +1165,8 @@
// with a reason" thread. It should add to the stream whatever text it
// wants to show the user, and return False to indicate it wants the target
// not to stop.
- virtual bool HandleStop(ExecutionContext &exe_ctx,
- lldb::StreamSP output) = 0;
+ virtual StopHookResult HandleStop(ExecutionContext &exe_ctx,
+ lldb::StreamSP output) = 0;
// Set the Thread Specifier. The stop hook will own the thread specifier,
// and is responsible for deleting it when we're done.
@@ -1201,8 +1206,8 @@
void SetActionFromString(const std::string &strings);
void SetActionFromStrings(const std::vector<std::string> &strings);
- bool HandleStop(ExecutionContext &exc_ctx,
- lldb::StreamSP output_sp) override;
+ StopHookResult HandleStop(ExecutionContext &exc_ctx,
+ lldb::StreamSP output_sp) override;
void GetSubclassDescription(Stream *s,
lldb::DescriptionLevel level) const override;
@@ -1219,7 +1224,8 @@
class StopHookScripted : public StopHook {
public:
virtual ~StopHookScripted() = default;
- bool HandleStop(ExecutionContext &exc_ctx, lldb::StreamSP output) override;
+ StopHookResult HandleStop(ExecutionContext &exc_ctx,
+ lldb::StreamSP output) override;
Status SetScriptCallback(std::string class_name,
StructuredData::ObjectSP extra_args_sp);
@@ -1254,7 +1260,9 @@
/// remove the stop hook, as it will also reset the stop hook counter.
void UndoCreateStopHook(lldb::user_id_t uid);
- void RunStopHooks();
+ // Runs the stop hooks that have been registered for this target.
+ // Returns true if the stop hooks cause the target to resume.
+ bool RunStopHooks();
size_t GetStopHookSize();
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
