Merge #5917

5917: Fixing various 'held lock while suspending' problems r=hkaiser a=hkaiser - flyby: introduce HPX__ASSERT_LOCKED() et.al. facilities that unlock a given lock before invoking the assert Co-authored-by: Hartmut Kaiser <[email protected]>
STEllAR-GROUP · Jun 15, 2022 · dbf82fe · dbf82fe
2 parents 44f30b4 + 298b021
commit dbf82fe
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 32 deletions.
diff --git a/libs/core/assertion/include/hpx/modules/assertion.hpp b/libs/core/assertion/include/hpx/modules/assertion.hpp
@@ -64,17 +64,30 @@ namespace hpx { namespace assertion {
                     HPX_CURRENT_SOURCE_LOCATION(), HPX_PP_STRINGIZE(expr),     \
                     msg)) /**/
 
+#define HPX_ASSERT_LOCKED_(l, expr, msg)                                       \
+    (!!(expr) ? void() :                                                       \
+                (l.unlock(),                                                   \
+                    ::hpx::assertion::detail::handle_assert(                   \
+                        HPX_CURRENT_SOURCE_LOCATION(), HPX_PP_STRINGIZE(expr), \
+                        msg))) /**/
+
 #if defined(HPX_DEBUG)
 #if defined(HPX_COMPUTE_DEVICE_CODE)
 #define HPX_ASSERT(expr) assert(expr)
 #define HPX_ASSERT_MSG(expr, msg) HPX_ASSERT(expr)
+#define HPX_ASSERT_LOCKED(l, expr) assert(expr)
+#define HPX_ASSERT_LOCKED_MSG(l, expr, msg) HPX_ASSERT(expr)
 #else
 #define HPX_ASSERT(expr) HPX_ASSERT_(expr, std::string())
 #define HPX_ASSERT_MSG(expr, msg) HPX_ASSERT_(expr, msg)
+#define HPX_ASSERT_LOCKED(l, expr) HPX_ASSERT_LOCKED_(l, expr, std::string())
+#define HPX_ASSERT_LOCKED_MSG(l, expr, msg) HPX_ASSERT_LOCKED_(l, expr, msg)
 #endif
 #else
 #define HPX_ASSERT(expr)
 #define HPX_ASSERT_MSG(expr, msg)
+#define HPX_ASSERT_LOCKED(l, expr)
+#define HPX_ASSERT_LOCKED_MSG(l, expr, msg)
 #endif
 
 #define HPX_UNREACHABLE                                                        \

diff --git a/libs/core/futures/include/hpx/futures/detail/future_data.hpp b/libs/core/futures/include/hpx/futures/detail/future_data.hpp
@@ -494,6 +494,7 @@ namespace hpx { namespace lcos { namespace detail {
 
             // Note: cv.notify_one() above 'consumes' the lock 'l' and leaves
             //       it unlocked when returning.
+            HPX_ASSERT_DOESNT_OWN_LOCK(l);
 
             // invoke the callback (continuation) function
             if (!on_completed.empty())
@@ -553,6 +554,7 @@ namespace hpx { namespace lcos { namespace detail {
 
             // Note: cv.notify_one() above 'consumes' the lock 'l' and leaves
             //       it unlocked when returning.
+            HPX_ASSERT_DOESNT_OWN_LOCK(l);
 
             // invoke the callback (continuation) function
             if (!on_completed.empty())

diff --git a/libs/core/futures/src/future_data.cpp b/libs/core/futures/src/future_data.cpp
@@ -1,4 +1,4 @@
-//  Copyright (c) 2015-2021 Hartmut Kaiser
+//  Copyright (c) 2015-2022 Hartmut Kaiser
 //
 //  SPDX-License-Identifier: BSL-1.0
 //  Distributed under the Boost Software License, Version 1.0. (See accompanying
@@ -8,7 +8,6 @@
 #include <hpx/futures/future.hpp>
 
 #include <hpx/config.hpp>
-#include <hpx/assert.hpp>
 #include <hpx/async_base/launch_policy.hpp>
 #include <hpx/errors/try_catch_exception_ptr.hpp>
 #include <hpx/execution_base/this_thread.hpp>

diff --git a/libs/core/synchronization/include/hpx/synchronization/barrier.hpp b/libs/core/synchronization/include/hpx/synchronization/barrier.hpp
@@ -144,7 +144,7 @@ namespace hpx {
             std::unique_lock<mutex_type>& l, std::ptrdiff_t update = 1)
         {
             HPX_ASSERT_OWNS_LOCK(l);
-            HPX_ASSERT(arrived_ >= update);
+            HPX_ASSERT_LOCKED(l, arrived_ >= update);
 
             bool const old_phase = phase_;
             std::ptrdiff_t const result = (arrived_ -= update);
@@ -209,7 +209,7 @@ namespace hpx {
             if (phase_ == old_phase)
             {
                 cond_.wait(l, "barrier::wait");
-                HPX_ASSERT(phase_ != old_phase);
+                HPX_ASSERT_LOCKED(l, phase_ != old_phase);
             }
         }
 
@@ -221,7 +221,7 @@ namespace hpx {
             if (phase_ == old_phase)
             {
                 cond_.wait(l, "barrier::wait");
-                HPX_ASSERT(phase_ != old_phase);
+                HPX_ASSERT_LOCKED(l, phase_ != old_phase);
             }
         }
 
@@ -245,7 +245,7 @@ namespace hpx {
         void arrive_and_drop()
         {
             std::unique_lock<mutex_type> l(mtx_);
-            HPX_ASSERT(expected_ > 0);
+            HPX_ASSERT_LOCKED(l, expected_ > 0);
             --expected_;
             HPX_UNUSED(arrive_locked(l, 1));
         }

diff --git a/libs/core/synchronization/include/hpx/synchronization/detail/condition_variable.hpp b/libs/core/synchronization/include/hpx/synchronization/detail/condition_variable.hpp
@@ -1,4 +1,4 @@
-//  Copyright (c) 2007-2013 Hartmut Kaiser
+//  Copyright (c) 2007-2022 Hartmut Kaiser
 //  Copyright (c) 2013-2015 Agustin Berge
 //
 //  SPDX-License-Identifier: BSL-1.0
@@ -84,11 +84,10 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
 
         HPX_CORE_EXPORT ~condition_variable();
 
-        HPX_CORE_EXPORT bool empty(
-            std::unique_lock<mutex_type> const& lock) const;
+        HPX_CORE_EXPORT bool empty(std::unique_lock<mutex_type>& lock) const;
 
         HPX_CORE_EXPORT std::size_t size(
-            std::unique_lock<mutex_type> const& lock) const;
+            std::unique_lock<mutex_type>& lock) const;
 
         // Return false if no more threads are waiting (returns true if queue
         // is non-empty).

diff --git a/libs/core/synchronization/include/hpx/synchronization/latch.hpp b/libs/core/synchronization/include/hpx/synchronization/latch.hpp
@@ -131,8 +131,9 @@ namespace hpx {
             {
                 cond_.data_.wait(l, "hpx::latch::wait");
 
-                HPX_ASSERT(counter_.load(std::memory_order_relaxed) == 0);
-                HPX_ASSERT(notified_);
+                HPX_ASSERT_LOCKED(
+                    l, counter_.load(std::memory_order_relaxed) == 0);
+                HPX_ASSERT_LOCKED(l, notified_);
             }
         }
 
@@ -147,14 +148,15 @@ namespace hpx {
 
             std::ptrdiff_t old_count =
                 counter_.fetch_sub(update, std::memory_order_relaxed);
-            HPX_ASSERT(old_count >= update);
+            HPX_ASSERT_LOCKED(l, old_count >= update);
 
             if (old_count > update)
             {
                 cond_.data_.wait(l, "hpx::latch::arrive_and_wait");
 
-                HPX_ASSERT(counter_.load(std::memory_order_relaxed) == 0);
-                HPX_ASSERT(notified_);
+                HPX_ASSERT_LOCKED(
+                    l, counter_.load(std::memory_order_relaxed) == 0);
+                HPX_ASSERT_LOCKED(l, notified_);
             }
             else
             {

diff --git a/libs/core/synchronization/src/detail/condition_variable.cpp b/libs/core/synchronization/src/detail/condition_variable.cpp
@@ -13,6 +13,7 @@
 #include <hpx/synchronization/detail/condition_variable.hpp>
 #include <hpx/synchronization/no_mutex.hpp>
 #include <hpx/synchronization/spinlock.hpp>
+#include <hpx/thread_support/assert_owns_lock.hpp>
 #include <hpx/thread_support/unlock_guard.hpp>
 #include <hpx/threading_base/thread_helpers.hpp>
 #include <hpx/timing/steady_clock.hpp>
@@ -26,7 +27,7 @@
 namespace hpx { namespace lcos { namespace local { namespace detail {
 
     ///////////////////////////////////////////////////////////////////////////
-    condition_variable::condition_variable() {}
+    condition_variable::condition_variable() = default;
 
     condition_variable::~condition_variable()
     {
@@ -41,19 +42,18 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
         }
     }
 
-    bool condition_variable::empty(
-        std::unique_lock<mutex_type> const& lock) const
+    bool condition_variable::empty(std::unique_lock<mutex_type>& lock) const
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
         HPX_UNUSED(lock);
 
         return queue_.empty();
     }
 
     std::size_t condition_variable::size(
-        std::unique_lock<mutex_type> const& lock) const
+        std::unique_lock<mutex_type>& lock) const
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
         HPX_UNUSED(lock);
 
         return queue_.size();
@@ -64,7 +64,7 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
     bool condition_variable::notify_one(std::unique_lock<mutex_type> lock,
         threads::thread_priority /* priority */, error_code& ec)
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
 
         if (!queue_.empty())
         {
@@ -101,7 +101,7 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
     void condition_variable::notify_all(std::unique_lock<mutex_type> lock,
         threads::thread_priority /* priority */, error_code& ec)
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
 
         // swap the list
         queue_type queue;
@@ -146,7 +146,7 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
 
     void condition_variable::abort_all(std::unique_lock<mutex_type> lock)
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
 
         abort_all<mutex_type>(HPX_MOVE(lock));
     }
@@ -155,7 +155,7 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
         std::unique_lock<mutex_type>& lock, char const* /* description */,
         error_code& /* ec */)
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
 
         // enqueue the request and block this thread
         auto this_ctx = hpx::execution_base::this_thread::agent();
@@ -178,7 +178,7 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
         hpx::chrono::steady_time_point const& abs_time,
         char const* /* description */, error_code& /* ec */)
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
 
         // enqueue the request and block this thread
         auto this_ctx = hpx::execution_base::this_thread::agent();
@@ -241,7 +241,7 @@ namespace hpx { namespace lcos { namespace local { namespace detail {
     void condition_variable::prepend_entries(
         std::unique_lock<mutex_type>& lock, queue_type& queue)
     {
-        HPX_ASSERT(lock.owns_lock());
+        HPX_ASSERT_OWNS_LOCK(lock);
         HPX_UNUSED(lock);
 
         // splice is constant time only if it == end

diff --git a/libs/core/synchronization/src/stop_token.cpp b/libs/core/synchronization/src/stop_token.cpp
@@ -244,6 +244,11 @@ namespace hpx { namespace detail {
             return has_lock_;
         }
 
+        void unlock() noexcept
+        {
+            state_.unlock();
+        }
+
         stop_state& state_;
         bool has_lock_;
     };
@@ -255,7 +260,8 @@ namespace hpx { namespace detail {
         if (!l)
             return false;    // stop has already been requested.
 
-        HPX_ASSERT(stop_requested(state_.load(std::memory_order_acquire)));
+        HPX_ASSERT_LOCKED(
+            l, stop_requested(state_.load(std::memory_order_acquire)));
 
         signalling_thread_ = hpx::threads::get_self_id();
 

diff --git a/libs/core/thread_support/include/hpx/thread_support/assert_owns_lock.hpp b/libs/core/thread_support/include/hpx/thread_support/assert_owns_lock.hpp
@@ -1,4 +1,5 @@
 //  Copyright (c) 2013 Agustin Berge
+//  Copyright (c) 2022 Hartmut Kaiser
 //
 //  SPDX-License-Identifier: BSL-1.0
 //  Distributed under the Boost Software License, Version 1.0. (See accompanying
@@ -29,17 +30,17 @@ namespace hpx { namespace util { namespace detail {
 
     template <typename Lock>
     typename std::enable_if<has_owns_lock<Lock>::value>::type assert_owns_lock(
-        Lock const& l, long) noexcept
+        Lock& l, long) noexcept
     {
-        HPX_ASSERT(l.owns_lock());
+        HPX_ASSERT_LOCKED(l, l.owns_lock());
         HPX_UNUSED(l);
     }
 
     template <typename Lock>
     typename std::enable_if<has_owns_lock<Lock>::value>::type
-    assert_doesnt_own_lock(Lock const& l, long) noexcept
+    assert_doesnt_own_lock(Lock& l, long) noexcept
     {
-        HPX_ASSERT(!l.owns_lock());
+        HPX_ASSERT_LOCKED(l, !l.owns_lock());
         HPX_UNUSED(l);
     }
 }}}    // namespace hpx::util::detail