This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbe66987e2047: Fix raciness in the StopHook check for
"has the target run". (authored by jingham).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88753/new/
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
