JDevlieghere created this revision.
JDevlieghere added reviewers: jingham, vsk.
Herald added a subscriber: jfb.
JDevlieghere requested review of this revision.
This is a speculative fix when looking at the finalization code in Process. It
tackles the following issues:
- Adds synchronization to prevent races between threads.
- Marks the process as finalized/invalid as soon as `Finalize` is called rather
than at the end.
- Simplifies the code by using only a single instance variable to track
finalization.
https://reviews.llvm.org/D93479
Files:
lldb/include/lldb/Target/Process.h
lldb/source/Target/Process.cpp
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -556,7 +556,7 @@
m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
m_memory_cache(*this), m_allocated_memory_cache(*this),
m_should_detach(false), m_next_event_action_up(), m_public_run_lock(),
- m_private_run_lock(), m_finalizing(false), m_finalize_called(false),
+ m_private_run_lock(), m_finalized(false),
m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
m_can_interpret_function_calls(false), m_warnings_issued(),
@@ -632,7 +632,8 @@
}
void Process::Finalize() {
- m_finalizing = true;
+ if (m_finalized.exchange(true))
+ return;
// Destroy this process if needed
switch (GetPrivateState()) {
@@ -644,7 +645,7 @@
case eStateStepping:
case eStateCrashed:
case eStateSuspended:
- Destroy(false);
+ DestroyImpl(false);
break;
case eStateInvalid:
@@ -707,7 +708,6 @@
m_private_run_lock.TrySetRunning(); // This will do nothing if already locked
m_private_run_lock.SetStopped();
m_structured_data_plugin_map.clear();
- m_finalize_called = true;
}
void Process::RegisterNotificationCallbacks(const Notifications &callbacks) {
@@ -1516,7 +1516,7 @@
StateType Process::GetPrivateState() { return m_private_state.GetValue(); }
void Process::SetPrivateState(StateType new_state) {
- if (m_finalize_called)
+ if (!IsValid())
return;
Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STATE |
@@ -1567,10 +1567,7 @@
}
// Use our target to get a shared pointer to ourselves...
- if (m_finalize_called && !PrivateStateThreadIsValid())
- BroadcastEvent(event_sp);
- else
- m_private_state_broadcaster.BroadcastEvent(event_sp);
+ m_private_state_broadcaster.BroadcastEvent(event_sp);
} else {
LLDB_LOGF(log,
"Process::SetPrivateState (%s) state didn't change. Ignoring...",
@@ -1597,7 +1594,7 @@
std::vector<LanguageRuntime *> Process::GetLanguageRuntimes() {
std::vector<LanguageRuntime *> language_runtimes;
- if (m_finalizing)
+ if (m_finalized)
return language_runtimes;
std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
@@ -1615,7 +1612,7 @@
}
LanguageRuntime *Process::GetLanguageRuntime(lldb::LanguageType language) {
- if (m_finalizing)
+ if (m_finalized)
return nullptr;
LanguageRuntime *runtime = nullptr;
@@ -1643,7 +1640,7 @@
}
bool Process::IsPossibleDynamicValue(ValueObject &in_value) {
- if (m_finalizing)
+ if (m_finalized)
return false;
if (in_value.IsDynamic())
@@ -3345,9 +3342,12 @@
Status Process::Destroy(bool force_kill) {
// If we've already called Process::Finalize then there's nothing useful to
// be done here. Finalize has actually called Destroy already.
- if (m_finalize_called)
+ if (m_finalized)
return {};
+ return DestroyImpl(force_kill);
+}
+Status Process::DestroyImpl(bool force_kill) {
// Tell ourselves we are in the process of destroying the process, so that we
// don't do any unnecessary work that might hinder the destruction. Remember
// to set this back to false when we are done. That way if the attempt
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -594,7 +594,7 @@
/// \return
/// Returns \b true if this Process has not been finalized
/// and \b false otherwise.
- bool IsValid() const { return !m_finalize_called; }
+ bool IsValid() const { return !m_finalized; }
/// Return a multi-word command object that can be used to expose plug-in
/// specific commands.
@@ -2831,10 +2831,11 @@
// m_currently_handling_do_on_removals are true,
// Resume will only request a resume, using this
// flag to check.
- bool m_finalizing; // This is set at the beginning of Process::Finalize() to
- // stop functions from looking up or creating things
- // during a finalize call
- bool m_finalize_called; // This is set at the end of Process::Finalize()
+
+ /// This is set at the beginning of Process::Finalize() to stop functions
+ /// from looking up or creating things during or after a finalize call.
+ std::atomic<bool> m_finalized;
+
bool m_clear_thread_plans_on_stop;
bool m_force_next_event_delivery;
lldb::StateType m_last_broadcast_state; /// This helps with the Public event
@@ -2938,6 +2939,8 @@
void LoadOperatingSystemPlugin(bool flush);
private:
+ Status DestroyImpl(bool force_kill);
+
/// This is the part of the event handling that for a process event. It
/// decides what to do with the event and returns true if the event needs to
/// be propagated to the user, and false otherwise. If the event is not
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
