From bea74f4b77683cd340a99fbabb4a04903fc3b2ab Mon Sep 17 00:00:00 2001 From: Brian Gianforcaro Date: Sat, 7 Aug 2021 04:19:39 -0700 Subject: [PATCH] Kernel: Reduce LOCK_DEBUG ifdefs by utilizing Kernel::LockLocation The LOCK_DEBUG conditional code is pretty ugly for a feature that we only use rarely. We can remove a significant amount of this code by utilizing a zero sized fake type when not building in LOCK_DEBUG mode. This lets us keep the same API, but just let the compiler optimize it away when don't actually care about the location the caller came from. --- Kernel/Locking/Mutex.cpp | 16 +++------------- Kernel/Locking/Mutex.h | 40 ++++++---------------------------------- Kernel/Thread.cpp | 2 +- Kernel/Thread.h | 8 +++----- 4 files changed, 13 insertions(+), 53 deletions(-) diff --git a/Kernel/Locking/Mutex.cpp b/Kernel/Locking/Mutex.cpp index 70298e5b0d..82cbc45725 100644 --- a/Kernel/Locking/Mutex.cpp +++ b/Kernel/Locking/Mutex.cpp @@ -4,22 +4,16 @@ * SPDX-License-Identifier: BSD-2-Clause */ -#ifdef LOCK_DEBUG -# include -#endif #include #include +#include #include #include #include namespace Kernel { -#if LOCK_DEBUG -void Mutex::lock(Mode mode, const SourceLocation& location) -#else -void Mutex::lock(Mode mode) -#endif +void Mutex::lock(Mode mode, [[maybe_unused]] const LockLocation& location) { // NOTE: This may be called from an interrupt handler (not an IRQ handler) // and also from within critical sections! @@ -318,11 +312,7 @@ auto Mutex::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode return current_mode; } -#if LOCK_DEBUG -void Mutex::restore_lock(Mode mode, u32 lock_count, const SourceLocation& location) -#else -void Mutex::restore_lock(Mode mode, u32 lock_count) -#endif +void Mutex::restore_lock(Mode mode, u32 lock_count, [[maybe_unused]] const LockLocation& location) { VERIFY(mode != Mode::Unlocked); VERIFY(lock_count > 0); diff --git a/Kernel/Locking/Mutex.h b/Kernel/Locking/Mutex.h index a9277caddf..802d7eff44 100644 --- a/Kernel/Locking/Mutex.h +++ b/Kernel/Locking/Mutex.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -31,13 +32,8 @@ public: } ~Mutex() = default; -#if LOCK_DEBUG - void lock(Mode mode = Mode::Exclusive, const SourceLocation& location = SourceLocation::current()); - void restore_lock(Mode, u32, const SourceLocation& location = SourceLocation::current()); -#else - void lock(Mode = Mode::Exclusive); - void restore_lock(Mode, u32); -#endif + void lock(Mode mode = Mode::Exclusive, const LockLocation& location = LockLocation::current()); + void restore_lock(Mode, u32, const LockLocation& location = LockLocation::current()); void unlock(); [[nodiscard]] Mode force_unlock_if_locked(u32&); @@ -115,18 +111,10 @@ public: { } -#if LOCK_DEBUG - ALWAYS_INLINE explicit MutexLocker(Mutex& l, Mutex::Mode mode = Mutex::Mode::Exclusive, const SourceLocation& location = SourceLocation::current()) -#else - ALWAYS_INLINE explicit MutexLocker(Mutex& l, Mutex::Mode mode = Mutex::Mode::Exclusive) -#endif + ALWAYS_INLINE explicit MutexLocker(Mutex& l, Mutex::Mode mode = Mutex::Mode::Exclusive, const LockLocation& location = LockLocation::current()) : m_lock(&l) { -#if LOCK_DEBUG m_lock->lock(mode, location); -#else - m_lock->lock(mode); -#endif } ALWAYS_INLINE ~MutexLocker() @@ -143,38 +131,22 @@ public: m_lock->unlock(); } -#if LOCK_DEBUG - ALWAYS_INLINE void attach_and_lock(Mutex& lock, Mutex::Mode mode = Mutex::Mode::Exclusive, const SourceLocation& location = SourceLocation::current()) -#else - ALWAYS_INLINE void attach_and_lock(Mutex& lock, Mutex::Mode mode = Mutex::Mode::Exclusive) -#endif + ALWAYS_INLINE void attach_and_lock(Mutex& lock, Mutex::Mode mode = Mutex::Mode::Exclusive, const LockLocation& location = LockLocation::current()) { VERIFY(!m_locked); m_lock = &lock; m_locked = true; -#if LOCK_DEBUG m_lock->lock(mode, location); -#else - m_lock->lock(mode); -#endif } -#if LOCK_DEBUG - ALWAYS_INLINE void lock(Mutex::Mode mode = Mutex::Mode::Exclusive, const SourceLocation& location = SourceLocation::current()) -#else - ALWAYS_INLINE void lock(Mutex::Mode mode = Mutex::Mode::Exclusive) -#endif + ALWAYS_INLINE void lock(Mutex::Mode mode = Mutex::Mode::Exclusive, const LockLocation& location = LockLocation::current()) { VERIFY(m_lock); VERIFY(!m_locked); m_locked = true; -#if LOCK_DEBUG m_lock->lock(mode, location); -#else - m_lock->lock(mode); -#endif } private: diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index f4f6c88fac..40e984f7aa 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -514,7 +514,7 @@ void Thread::finalize() dbgln("Thread {} leaking {} Locks!", *this, lock_count()); ScopedSpinLock list_lock(m_holding_locks_lock); for (auto& info : m_holding_locks_list) { - const auto& location = info.source_location; + const auto& location = info.lock_location; dbgln(" - Mutex: \"{}\" @ {} locked in function \"{}\" at \"{}:{}\" with a count of: {}", info.lock->name(), info.lock, location.function_name(), location.filename(), location.line_number(), info.count); } VERIFY_NOT_REACHED(); diff --git a/Kernel/Thread.h b/Kernel/Thread.h index 24441292ac..3609ecbc8f 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -12,9 +12,6 @@ #include #include #include -#ifdef LOCK_DEBUG -# include -#endif #include #include #include @@ -27,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -1152,7 +1150,7 @@ public: RecursiveSpinLock& get_lock() const { return m_lock; } #if LOCK_DEBUG - void holding_lock(Mutex& lock, int refs_delta, const SourceLocation& location) + void holding_lock(Mutex& lock, int refs_delta, const LockLocation& location) { VERIFY(refs_delta != 0); m_holding_locks.fetch_add(refs_delta, AK::MemoryOrder::memory_order_relaxed); @@ -1317,7 +1315,7 @@ private: #if LOCK_DEBUG struct HoldingLockInfo { Mutex* lock; - SourceLocation source_location; + LockLocation lock_location; unsigned count; }; Atomic m_holding_locks { 0 };