[lldb] Avoid deadlock by unlocking before invoking callbacks

[JDevlieghere]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/86888.diff

1 Files Affected:

• (modified) lldb/source/Host/common/Alarm.cpp (+45-39)

diff --git a/lldb/source/Host/common/Alarm.cpp b/lldb/source/Host/common/Alarm.cpp
index 245cdc7ae5c2da..60ac91db3ccf59 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)
+            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 {};
 }

[bulbazord]

Makes sense to me.