[Lldb-commits] [lldb] [lldb] Avoid deadlock by unlocking before invoking callbacks (PR #86888)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/86888

>From 793f6332ddf9059d999a7236770363e226ef8dd4 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
Date: Wed, 27 Mar 2024 16:05:21 -0700
Subject: [PATCH] [lldb] Avoid deadlock by unlocking before invoking callbacks

Avoid deadlocks in the Alarm class by releasing the lock before invoking
callbacks. This deadlock manifested itself in the ProgressManager:

  1. On the main thread, the ProgressManager acquires its lock in
     ProgressManager::Decrement and calls Alarm::Create.
  2. On the main thread, the Alarm acquires its lock in Alarm::Create.
  3. On the alarm thread, the Alarm acquires its lock after waiting on
     the condition variable and calls ProgressManager::Expire.
  4. On the alarm thread, the ProgressManager acquires its lock in
     ProgressManager::Expire.

Note how the two threads are acquiring the locks in different orders.
Deadlocks can be avoided by always acquiring locks in the same order,
but since the two mutexes here are private implementation details,
belong to different classes, that's not straightforward. Luckily, we
don't need to have the Alarm mutex locked when invoking the callbacks.
That exactly how this patch solves the issue.
---
 lldb/source/Host/common/Alarm.cpp | 84 +++++++++++++++++--------------
 1 file changed, 45 insertions(+), 39 deletions(-)

diff --git a/lldb/source/Host/common/Alarm.cpp b/lldb/source/Host/common/Alarm.cpp
index 245cdc7ae5c2da..afc770d20d7b1e 100644
--- a/lldb/source/Host/common/Alarm.cpp
+++ b/lldb/source/Host/common/Alarm.cpp
@@ -154,54 +154,60 @@ lldb::thread_result_t Alarm::AlarmThread() {
     //
     // Below we only deal with the timeout expiring and fall through for dealing
     // with the rest.
-    std::unique_lock<std::mutex> alarm_lock(m_alarm_mutex);
-    if (next_alarm) {
-      if (!m_alarm_cv.wait_until(alarm_lock, *next_alarm, predicate)) {
-        // The timeout for the next alarm expired.
-
-        // Clear the next timeout to signal that we need to recompute the next
-        // timeout.
-        next_alarm.reset();
-
-        // Iterate over all the callbacks. Call the ones that have expired
-        // and remove them from the list.
-        const TimePoint now = std::chrono::system_clock::now();
-        auto it = m_entries.begin();
-        while (it != m_entries.end()) {
-          if (it->expiration <= now) {
-            it->callback();
-            it = m_entries.erase(it);
-          } else {
-            it++;
+    llvm::SmallVector<Callback, 1> callbacks;
+    {
+      std::unique_lock<std::mutex> alarm_lock(m_alarm_mutex);
+      if (next_alarm) {
+        if (!m_alarm_cv.wait_until(alarm_lock, *next_alarm, predicate)) {
+          // The timeout for the next alarm expired.
+
+          // Clear the next timeout to signal that we need to recompute the next
+          // timeout.
+          next_alarm.reset();
+
+          // Iterate over all the callbacks. Call the ones that have expired
+          // and remove them from the list.
+          const TimePoint now = std::chrono::system_clock::now();
+          auto it = m_entries.begin();
+          while (it != m_entries.end()) {
+            if (it->expiration <= now) {
+              callbacks.emplace_back(std::move(it->callback));
+              it = m_entries.erase(it);
+            } else {
+              it++;
+            }
           }
         }
+      } else {
+        m_alarm_cv.wait(alarm_lock, predicate);
       }
-    } else {
-      m_alarm_cv.wait(alarm_lock, predicate);
-    }
 
-    // Fall through after waiting on the condition variable. At this point
-    // either the predicate is true or we woke up because an alarm expired.
+      // Fall through after waiting on the condition variable. At this point
+      // either the predicate is true or we woke up because an alarm expired.
 
-    // The alarm thread is shutting down.
-    if (m_exit) {
-      exit = true;
-      if (m_run_callbacks_on_exit) {
-        for (Entry &entry : m_entries)
-          entry.callback();
+      // The alarm thread is shutting down.
+      if (m_exit) {
+        exit = true;
+        if (m_run_callbacks_on_exit) {
+          for (Entry &entry : m_entries)
+            callbacks.emplace_back(std::move(entry.callback));
+        }
       }
-      continue;
-    }
 
-    // A new alarm was added or an alarm expired. Either way we need to
-    // recompute when this thread should wake up for the next alarm.
-    if (m_recompute_next_alarm || !next_alarm) {
-      for (Entry &entry : m_entries) {
-        if (!next_alarm || entry.expiration < *next_alarm)
-          next_alarm = entry.expiration;
+      // A new alarm was added or an alarm expired. Either way we need to
+      // recompute when this thread should wake up for the next alarm.
+      if (m_recompute_next_alarm || !next_alarm) {
+        for (Entry &entry : m_entries) {
+          if (!next_alarm || entry.expiration < *next_alarm)
+            next_alarm = entry.expiration;
+        }
+        m_recompute_next_alarm = false;
       }
-      m_recompute_next_alarm = false;
     }
+
+    // Outside the lock, call the callbacks.
+    for (Callback &callback : callbacks)
+      callback();
   }
   return {};
 }