Address Pavel's feedback

Author: JDevlieghere

lldb/include/lldb/API/LLDB.h

@@ -46,6 +46,7 @@
 #include "lldb/API/SBLaunchInfo.h"
 #include "lldb/API/SBLineEntry.h"
 #include "lldb/API/SBListener.h"
+#include "lldb/API/SBLock.h"
 #include "lldb/API/SBMemoryRegionInfo.h"
 #include "lldb/API/SBMemoryRegionInfoList.h"
 #include "lldb/API/SBModule.h"

lldb/include/lldb/API/SBLock.h

@@ -11,7 +11,6 @@
 
 #include "lldb/API/SBDefines.h"
 #include "lldb/lldb-forward.h"
-
 #include <mutex>
 
 namespace lldb_private {
@@ -32,7 +31,7 @@ class LLDB_API SBLock {
 
   // Private constructor used by SBTarget to create the Target API lock.
   // Requires a friend declaration.
-  SBLock(std::recursive_mutex &mutex, lldb::TargetSP target_sp);
+  SBLock(lldb::TargetSP target_sp);
   friend class SBTarget;
 
   SBLock(const SBLock &rhs) = delete;

lldb/include/lldb/Target/Target.h

@@ -1702,8 +1702,8 @@ struct APILock {
 /// The private implementation used by SBLock to hand out the target API mutex.
 /// It has a TargetSP to ensure the lock cannot outlive the target.
 struct TargetAPILock : public APILock {
-  TargetAPILock(std::recursive_mutex &mutex, lldb::TargetSP target_sp)
-      : APILock(mutex), target_sp(target_sp) {}
+  TargetAPILock(lldb::TargetSP target_sp)
+      : APILock(target_sp->GetAPIMutex()), target_sp(target_sp) {}
   ~TargetAPILock() override = default;
   lldb::TargetSP target_sp;
 };

lldb/source/API/SBLock.cpp

@@ -10,17 +10,14 @@
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Instrumentation.h"
 #include "lldb/lldb-forward.h"
-
 #include <memory>
 #include <mutex>
 
 using namespace lldb;
 using namespace lldb_private;
 
-#ifndef SWIG
-
-SBLock::SBLock(std::recursive_mutex &mutex, lldb::TargetSP target_sp)
-    : m_opaque_up(std::make_unique<TargetAPILock>(mutex, target_sp)) {
+SBLock::SBLock(lldb::TargetSP target_sp)
+    : m_opaque_up(std::make_unique<TargetAPILock>(target_sp)) {
   LLDB_INSTRUMENT_VA(this);
 }
 
@@ -31,5 +28,3 @@ bool SBLock::IsValid() const {
 
   return static_cast<bool>(m_opaque_up);
 }
-
-#endif

lldb/source/API/SBTarget.cpp

@@ -2439,7 +2439,7 @@ lldb::SBLock SBTarget::AcquireAPILock() const {
   LLDB_INSTRUMENT_VA(this);
 
   if (TargetSP target_sp = GetSP())
-    return lldb::SBLock(target_sp->GetAPIMutex(), target_sp);
+    return lldb::SBLock(target_sp);
   return lldb::SBLock();
 }
 #endif

lldb/unittests/API/SBLockTest.cpp

@@ -6,59 +6,50 @@
 //
 //===----------------------------------------------------------------------===/
 
-#include "lldb/API/SBLock.h"
+// Use the umbrella header for -Wdocumentation.
+#include "lldb/API/LLDB.h"
+
+#include "TestingSupport/SubsystemRAII.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBTarget.h"
 #include "gtest/gtest.h"
 #include <atomic>
 #include <chrono>
-#include <thread>
-
-TEST(SBLockTest, LockTest) {
-  lldb::SBDebugger debugger = lldb::SBDebugger::Create();
-  lldb::SBTarget target = debugger.GetDummyTarget();
+#include <future>
 
-  std::mutex m;
-  std::condition_variable cv;
-  bool wakeup = false;
-  std::atomic<bool> locked = false;
+using namespace lldb;
+using namespace lldb_private;
 
-  std::thread test_thread([&]() {
-    {
-      {
-        std::unique_lock lk(m);
-        cv.wait(lk, [&] { return wakeup; });
-      }
+class SBLockTest : public testing::Test {
+  SubsystemRAII<lldb::SBDebugger> subsystems;
+};
 
-      ASSERT_TRUE(locked);
-      target.BreakpointCreateByName("foo", "bar");
-      ASSERT_FALSE(locked);
-
-      cv.notify_one();
-    }
-  });
+TEST_F(SBLockTest, LockTest) {
+  lldb::SBDebugger debugger = lldb::SBDebugger::Create();
+  lldb::SBTarget target = debugger.GetDummyTarget();
 
-  // Take the API lock.
+  std::future<void> f;
   {
+    std::atomic<bool> locked = false;
     lldb::SBLock lock = target.AcquireAPILock();
     ASSERT_FALSE(locked.exchange(true));
 
-    // Wake up the test thread.
-    {
-      std::lock_guard lk(m);
-      wakeup = true;
-    }
-    cv.notify_one();
+    f = std::async(std::launch::async, [&]() {
+      {
+        ASSERT_TRUE(locked);
+        target.BreakpointCreateByName("foo", "bar");
+        ASSERT_FALSE(locked);
+      }
+    });
+    ASSERT_TRUE(f.valid());
 
     // Wait 500ms to confirm the thread is blocked.
-    {
-      std::unique_lock<std::mutex> lk(m);
-      auto result = cv.wait_for(lk, std::chrono::milliseconds(500));
-      ASSERT_EQ(result, std::cv_status::timeout);
-    }
+    auto status = f.wait_for(std::chrono::milliseconds(500));
+    ASSERT_EQ(status, std::future_status::timeout);
 
     ASSERT_TRUE(locked.exchange(false));
   }
+  f.wait();
 
-  test_thread.join();
+  lldb::SBDebugger::Destroy(debugger);
 }