pw_sync: adds VirtualBasicLockable interface
Adds a virtual lock interface in case the lock selection cannot
be templated.
Change-Id: I79f3f28c28572e970ddf4002477e0ea2c6ee02ab
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/58202
Pigweed-Auto-Submit: Ewout van Bekkum <ewout@google.com>
Commit-Queue: Ewout van Bekkum <ewout@google.com>
Reviewed-by: Keir Mierle <keir@google.com>
diff --git a/pw_sync/BUILD.bazel b/pw_sync/BUILD.bazel
index c8cbe1c..d395863 100644
--- a/pw_sync/BUILD.bazel
+++ b/pw_sync/BUILD.bazel
@@ -115,10 +115,23 @@
],
includes = ["public"],
deps = [
+ ":virtual_basic_lockable",
"//pw_assert",
],
)
+pw_cc_library(
+ name = "virtual_basic_lockable",
+ hdrs = [
+ "public/pw_sync/virtual_basic_lockable.h",
+ ],
+ includes = ["public"],
+ deps = [
+ ":lock_annotations",
+ "//pw_polyfill",
+ ],
+)
+
pw_cc_facade(
name = "mutex_facade",
hdrs = [
@@ -127,6 +140,7 @@
includes = ["public"],
deps = [
":lock_annotations",
+ ":virtual_basic_lockable",
"//pw_preprocessor",
],
)
@@ -163,6 +177,7 @@
deps = [
":lock_annotations",
":mutex_facade",
+ ":virtual_basic_lockable",
"//pw_chrono:system_clock",
"//pw_preprocessor",
],
@@ -176,6 +191,7 @@
deps = [
":mutex",
":timed_mutex_facade",
+ ":virtual_basic_lockable",
"@pigweed_config//:pw_sync_timed_mutex_backend",
],
)
@@ -200,6 +216,7 @@
includes = ["public"],
deps = [
":lock_annotations",
+ ":virtual_basic_lockable",
"//pw_preprocessor",
],
)
@@ -350,6 +367,7 @@
],
deps = [
":borrow",
+ ":virtual_basic_lockable",
"//pw_assert",
"//pw_unit_test",
],
diff --git a/pw_sync/BUILD.gn b/pw_sync/BUILD.gn
index 67d0448..16554c1 100644
--- a/pw_sync/BUILD.gn
+++ b/pw_sync/BUILD.gn
@@ -62,7 +62,19 @@
pw_source_set("borrow") {
public_configs = [ ":public_include_path" ]
public = [ "public/pw_sync/borrow.h" ]
- public_deps = [ dir_pw_assert ]
+ public_deps = [
+ ":virtual_basic_lockable",
+ dir_pw_assert,
+ ]
+}
+
+pw_source_set("virtual_basic_lockable") {
+ public_configs = [ ":public_include_path" ]
+ public = [ "public/pw_sync/virtual_basic_lockable.h" ]
+ public_deps = [
+ ":lock_annotations",
+ dir_pw_polyfill,
+ ]
}
pw_facade("mutex") {
@@ -71,6 +83,7 @@
public = [ "public/pw_sync/mutex.h" ]
public_deps = [
":lock_annotations",
+ ":virtual_basic_lockable",
"$dir_pw_preprocessor",
]
sources = [ "mutex.cc" ]
@@ -82,6 +95,7 @@
public = [ "public/pw_sync/timed_mutex.h" ]
public_deps = [
":mutex",
+ ":virtual_basic_lockable",
"$dir_pw_chrono:system_clock",
"$dir_pw_preprocessor",
]
@@ -94,6 +108,7 @@
public = [ "public/pw_sync/interrupt_spin_lock.h" ]
public_deps = [
":lock_annotations",
+ ":virtual_basic_lockable",
"$dir_pw_preprocessor",
]
sources = [ "interrupt_spin_lock.cc" ]
@@ -174,6 +189,7 @@
sources = [ "borrow_test.cc" ]
deps = [
":borrow",
+ ":virtual_basic_lockable",
dir_pw_assert,
]
}
diff --git a/pw_sync/CMakeLists.txt b/pw_sync/CMakeLists.txt
index 69c553e..d7e3af7 100644
--- a/pw_sync/CMakeLists.txt
+++ b/pw_sync/CMakeLists.txt
@@ -19,5 +19,6 @@
mutex.cc
PUBLIC_DEPS
pw_chrono.system_clock
+ pw_polyfill
pw_preprocessor
)
diff --git a/pw_sync/borrow_test.cc b/pw_sync/borrow_test.cc
index 4782d78..adec2af 100644
--- a/pw_sync/borrow_test.cc
+++ b/pw_sync/borrow_test.cc
@@ -19,6 +19,7 @@
#include "gtest/gtest.h"
#include "pw_assert/check.h"
+#include "pw_sync/virtual_basic_lockable.h"
namespace pw::sync {
namespace {
@@ -43,22 +44,30 @@
Borrowable<Foo&, Lock> borrowable_foo_;
};
-class BasicLockable {
+class BasicLockable : public VirtualBasicLockable {
public:
- void lock() {
- PW_CHECK(!locked_, "Recursive lock detected");
- locked_ = true;
- }
-
- void unlock() {
- PW_CHECK(locked_, "Unlock while unlocked detected");
- locked_ = false;
- }
+ virtual ~BasicLockable() = default;
bool locked() const { return locked_; }
protected:
bool locked_ = false;
+
+ private:
+ void DoLockOperation(Operation operation) override {
+ switch (operation) {
+ case Operation::kLock:
+ PW_CHECK(!locked_, "Recursive lock detected");
+ locked_ = true;
+ return;
+
+ case Operation::kUnlock:
+ default:
+ PW_CHECK(locked_, "Unlock while unlocked detected");
+ locked_ = false;
+ return;
+ }
+ }
};
using BorrowableBasicLockableTest = BorrowableTest<BasicLockable>;
diff --git a/pw_sync/docs.rst b/pw_sync/docs.rst
index 7dcb97b..fd2bef8 100644
--- a/pw_sync/docs.rst
+++ b/pw_sync/docs.rst
@@ -952,6 +952,34 @@
Critical Section Lock Helpers
-----------------------------
+Virtual Lock Interfaces
+=======================
+Virtual lock interfaces can be useful when lock selection cannot be templated.
+
+Why use virtual locks?
+----------------------
+Virtual locks enable depending on locks without templating implementation code
+on the type, while retaining flexibility with respect to the concrete lock type.
+Pigweed tries to avoid pushing policy on to users, and virtual locks are one way
+to accomplish that without templating everything.
+
+A case when virtual locks are useful is when the concrete lock type changes at
+run time. For example, access to flash may be protected at run time by an
+internal mutex, however at crash time we may want to switch to a no-op lock. A
+virtual lock interface could be used here to minimize the code-size cost that
+would occur otherwise if the flash driver were templated.
+
+VirtualBasicLock
+----------------
+The ``VirtualBasicLock`` interface meets the
+`BasicLockable <https://en.cppreference.com/w/cpp/named_req/BasicLockable>`_ C++
+named requirement. Our critical section lock primitives offer optional virtual
+versions, including:
+
+* ``pw::sync::VirtualMutex``
+* ``pw::sync::VirtualTimedMutex``
+* ``pw::sync::VirtualInterruptSpinLock``
+
Borrowable
==========
The Borrowable is a helper construct that enables callers to borrow an object
@@ -966,6 +994,9 @@
`TimedLockable <https://en.cppreference.com/w/cpp/named_req/TimedLockable>`_
C++ named requirements.
+By default the selected lock type is a ``pw::sync::VirtualBasicLockable``. If
+this virtual interface is used, the templated lock parameter can be skipped.
+
External vs Internal locking
----------------------------
Before we explain why Borrowable is useful, it's important to understand the
@@ -1080,7 +1111,7 @@
C++
---
-.. cpp:class:: template <typename GuardedType, typename Lock> pw::sync::BorrowedPointer
+.. cpp:class:: template <typename GuardedType, typename Lock = pw::sync::VirtualBasicLockable> pw::sync::BorrowedPointer
The BorrowedPointer is an RAII handle which wraps a pointer to a borrowed
object along with a held lock which is guarding the object. When destroyed,
@@ -1102,7 +1133,7 @@
**Warning:** Be careful not to leak references to the borrowed object.
-.. cpp:class:: template <typename GuardedReference, typename Lock> pw::sync::Borrowable
+.. cpp:class:: template <typename GuardedReference, typename Lock = pw::sync::VirtualBasicLockable> pw::sync::Borrowable
.. cpp:function:: BorrowedPointer<GuardedType, Lock> acquire()
@@ -1141,14 +1172,13 @@
class ExampleI2c : public pw::i2c::Initiator;
- pw::sync::Mutex i2c_mutex;
+ pw::sync::VirtualMutex i2c_mutex;
ExampleI2c i2c;
- pw::sync::Borrowable<ExampleI2c&, pw::sync::Mutex> borrowable_i2c(
- i2c_mutex, i2c);
+ pw::sync::Borrowable<ExampleI2c&> borrowable_i2c(i2c_mutex, i2c);
pw::Result<ConstByteSpan> ReadI2cData(ByteSpan buffer) {
// Block indefinitely waiting to borrow the i2c bus.
- pw::sync::BorrowedPointer<ExampleI2c, pw::sync::Mutex> borrowed_i2c =
+ pw::sync::BorrowedPointer<ExampleI2c> borrowed_i2c =
borrowable_i2c.acquire();
// Execute a sequence of transactions to get the needed data.
diff --git a/pw_sync/interrupt_spin_lock_facade_test.cc b/pw_sync/interrupt_spin_lock_facade_test.cc
index 0d8a059..880ac81 100644
--- a/pw_sync/interrupt_spin_lock_facade_test.cc
+++ b/pw_sync/interrupt_spin_lock_facade_test.cc
@@ -58,6 +58,22 @@
}
}
+TEST(VirtualInterruptSpinLock, LockUnlock) {
+ pw::sync::VirtualInterruptSpinLock interrupt_spin_lock;
+ interrupt_spin_lock.lock();
+ // TODO(pwbug/291): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(interrupt_spin_lock.try_lock());
+ interrupt_spin_lock.unlock();
+}
+
+VirtualInterruptSpinLock static_virtual_interrupt_spin_lock;
+TEST(VirtualInterruptSpinLock, LockUnlockStatic) {
+ static_virtual_interrupt_spin_lock.lock();
+ // TODO(pwbug/291): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(static_virtual_interrupt_spin_lock.try_lock());
+ static_virtual_interrupt_spin_lock.unlock();
+}
+
TEST(InterruptSpinLock, LockUnlockInC) {
pw::sync::InterruptSpinLock interrupt_spin_lock;
pw_sync_InterruptSpinLock_CallLock(&interrupt_spin_lock);
diff --git a/pw_sync/mutex_facade_test.cc b/pw_sync/mutex_facade_test.cc
index 6788536..771b003 100644
--- a/pw_sync/mutex_facade_test.cc
+++ b/pw_sync/mutex_facade_test.cc
@@ -58,6 +58,22 @@
}
}
+TEST(VirtualMutex, LockUnlock) {
+ pw::sync::VirtualMutex mutex;
+ mutex.lock();
+ // TODO(pwbug/291): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(mutex.try_lock());
+ mutex.unlock();
+}
+
+VirtualMutex static_virtual_mutex;
+TEST(VirtualMutex, LockUnlockStatic) {
+ static_virtual_mutex.lock();
+ // TODO(pwbug/291): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(static_virtual_mutex.try_lock());
+ static_virtual_mutex.unlock();
+}
+
TEST(Mutex, LockUnlockInC) {
pw::sync::Mutex mutex;
pw_sync_Mutex_CallLock(&mutex);
diff --git a/pw_sync/public/pw_sync/borrow.h b/pw_sync/public/pw_sync/borrow.h
index 4ae02ed..038df5f 100644
--- a/pw_sync/public/pw_sync/borrow.h
+++ b/pw_sync/public/pw_sync/borrow.h
@@ -19,13 +19,14 @@
#include <type_traits>
#include "pw_assert/assert.h"
+#include "pw_sync/virtual_basic_lockable.h"
namespace pw::sync {
// The BorrowedPointer is an RAII handle which wraps a pointer to a borrowed
// object along with a held lock which is guarding the object. When destroyed,
// the lock is released.
-template <typename GuardedType, typename Lock>
+template <typename GuardedType, typename Lock = pw::sync::VirtualBasicLockable>
class BorrowedPointer {
public:
// Release the lock on destruction.
@@ -86,7 +87,8 @@
//
// This class is compatible with locks which comply with BasicLockable,
// Lockable, and TimedLockable C++ named requirements.
-template <typename GuardedReference, typename Lock>
+template <typename GuardedReference,
+ typename Lock = pw::sync::VirtualBasicLockable>
class Borrowable {
public:
static_assert(std::is_reference<GuardedReference>::value,
diff --git a/pw_sync/public/pw_sync/interrupt_spin_lock.h b/pw_sync/public/pw_sync/interrupt_spin_lock.h
index a8c7674..d9b75f7 100644
--- a/pw_sync/public/pw_sync/interrupt_spin_lock.h
+++ b/pw_sync/public/pw_sync/interrupt_spin_lock.h
@@ -20,6 +20,7 @@
#ifdef __cplusplus
+#include "pw_sync/virtual_basic_lockable.h"
#include "pw_sync_backend/interrupt_spin_lock_native.h"
namespace pw::sync {
@@ -79,6 +80,34 @@
backend::NativeInterruptSpinLock native_type_;
};
+class PW_LOCKABLE("pw::sync::VirtualInterruptSpinLock")
+ VirtualInterruptSpinLock final : public VirtualBasicLockable {
+ public:
+ VirtualInterruptSpinLock() = default;
+
+ VirtualInterruptSpinLock(const VirtualInterruptSpinLock&) = delete;
+ VirtualInterruptSpinLock(VirtualInterruptSpinLock&&) = delete;
+ VirtualInterruptSpinLock& operator=(const VirtualInterruptSpinLock&) = delete;
+ VirtualInterruptSpinLock& operator=(VirtualInterruptSpinLock&&) = delete;
+
+ InterruptSpinLock& interrupt_spin_lock() { return interrupt_spin_lock_; }
+
+ private:
+ void DoLockOperation(Operation operation) override
+ PW_NO_LOCK_SAFETY_ANALYSIS {
+ switch (operation) {
+ case Operation::kLock:
+ return interrupt_spin_lock_.lock();
+
+ case Operation::kUnlock:
+ default:
+ return interrupt_spin_lock_.unlock();
+ }
+ }
+
+ InterruptSpinLock interrupt_spin_lock_;
+};
+
} // namespace pw::sync
#include "pw_sync_backend/interrupt_spin_lock_inline.h"
diff --git a/pw_sync/public/pw_sync/mutex.h b/pw_sync/public/pw_sync/mutex.h
index a2da0f7..622e40f 100644
--- a/pw_sync/public/pw_sync/mutex.h
+++ b/pw_sync/public/pw_sync/mutex.h
@@ -20,6 +20,7 @@
#ifdef __cplusplus
+#include "pw_sync/virtual_basic_lockable.h"
#include "pw_sync_backend/mutex_native.h"
namespace pw::sync {
@@ -73,6 +74,34 @@
backend::NativeMutex native_type_;
};
+class PW_LOCKABLE("pw::sync::VirtualMutex") VirtualMutex final
+ : public VirtualBasicLockable {
+ public:
+ VirtualMutex() = default;
+
+ VirtualMutex(const VirtualMutex&) = delete;
+ VirtualMutex(VirtualMutex&&) = delete;
+ VirtualMutex& operator=(const VirtualMutex&) = delete;
+ VirtualMutex& operator=(VirtualMutex&&) = delete;
+
+ Mutex& mutex() { return mutex_; }
+
+ private:
+ void DoLockOperation(Operation operation) override
+ PW_NO_LOCK_SAFETY_ANALYSIS {
+ switch (operation) {
+ case Operation::kLock:
+ return mutex_.lock();
+
+ case Operation::kUnlock:
+ default:
+ return mutex_.unlock();
+ }
+ }
+
+ Mutex mutex_;
+};
+
} // namespace pw::sync
#include "pw_sync_backend/mutex_inline.h"
diff --git a/pw_sync/public/pw_sync/timed_mutex.h b/pw_sync/public/pw_sync/timed_mutex.h
index a7fac1a..eb03284 100644
--- a/pw_sync/public/pw_sync/timed_mutex.h
+++ b/pw_sync/public/pw_sync/timed_mutex.h
@@ -22,6 +22,8 @@
#ifdef __cplusplus
+#include "pw_sync/virtual_basic_lockable.h"
+
namespace pw::sync {
// The TimedMutex is a synchronization primitive that can be used to protect
@@ -65,6 +67,34 @@
PW_EXCLUSIVE_TRYLOCK_FUNCTION(true);
};
+class PW_LOCKABLE("pw::sync::VirtualTimedMutex") VirtualTimedMutex final
+ : public VirtualBasicLockable {
+ public:
+ VirtualTimedMutex() = default;
+
+ VirtualTimedMutex(const VirtualTimedMutex&) = delete;
+ VirtualTimedMutex(VirtualTimedMutex&&) = delete;
+ VirtualTimedMutex& operator=(const VirtualTimedMutex&) = delete;
+ VirtualTimedMutex& operator=(VirtualTimedMutex&&) = delete;
+
+ TimedMutex& timed_mutex() { return timed_mutex_; }
+
+ private:
+ void DoLockOperation(Operation operation) override
+ PW_NO_LOCK_SAFETY_ANALYSIS {
+ switch (operation) {
+ case Operation::kLock:
+ return timed_mutex_.lock();
+
+ case Operation::kUnlock:
+ default:
+ return timed_mutex_.unlock();
+ }
+ }
+
+ TimedMutex timed_mutex_;
+};
+
} // namespace pw::sync
#include "pw_sync_backend/timed_mutex_inline.h"
diff --git a/pw_sync/public/pw_sync/virtual_basic_lockable.h b/pw_sync/public/pw_sync/virtual_basic_lockable.h
new file mode 100644
index 0000000..4593e07
--- /dev/null
+++ b/pw_sync/public/pw_sync/virtual_basic_lockable.h
@@ -0,0 +1,73 @@
+// Copyright 2021 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+#pragma once
+
+#include "pw_polyfill/language_feature_macros.h"
+#include "pw_sync/lock_annotations.h"
+
+namespace pw::sync {
+
+// The VirtualBasicLockable is a virtual lock abstraction for locks which meet
+// the C++ named BasicLockable requirements of lock() and unlock().
+//
+// This virtual indirection is useful in case you need configurable lock
+// selection in a portable module where the final type is not defined upstream
+// and ergo module configuration cannot be used or in case the lock type is not
+// fixed at compile time, for example to support run time and crash time use of
+// an object without incurring the code size hit for templating the object.
+class PW_LOCKABLE("pw::sync::VirtualBasicLockable") VirtualBasicLockable {
+ public:
+ void lock() PW_EXCLUSIVE_LOCK_FUNCTION() {
+ DoLockOperation(Operation::kLock);
+ };
+
+ void unlock() PW_UNLOCK_FUNCTION() { DoLockOperation(Operation::kUnlock); };
+
+ protected:
+ ~VirtualBasicLockable() = default;
+
+ enum class Operation {
+ kLock,
+ kUnlock,
+ };
+
+ private:
+ // Uses a single virtual method with an enum to minimize the vtable cost per
+ // implementation of VirtualBasicLockable.
+ virtual void DoLockOperation(Operation operation) = 0;
+};
+
+// The NoOpLock is a type of VirtualBasicLockable that does nothing, i.e. lock
+// operations are no-ops.
+class PW_LOCKABLE("pw::sync::NoOpLock") NoOpLock final
+ : public VirtualBasicLockable {
+ public:
+ constexpr NoOpLock() {}
+ NoOpLock(const NoOpLock&) = delete;
+ NoOpLock(NoOpLock&&) = delete;
+ NoOpLock& operator=(const NoOpLock&) = delete;
+ NoOpLock& operator=(NoOpLock&&) = delete;
+
+ // Gives access to a global NoOpLock instance. It is not necessary to have
+ // multiple NoOpLock instances since they have no state and do nothing.
+ static NoOpLock& Instance() {
+ PW_CONSTINIT static NoOpLock lock;
+ return lock;
+ }
+
+ private:
+ void DoLockOperation(Operation) override {}
+};
+
+} // namespace pw::sync
diff --git a/pw_sync/timed_mutex_facade_test.cc b/pw_sync/timed_mutex_facade_test.cc
index 22ed0d3..cdeb631 100644
--- a/pw_sync/timed_mutex_facade_test.cc
+++ b/pw_sync/timed_mutex_facade_test.cc
@@ -114,6 +114,22 @@
}
}
+TEST(VirtualTimedMutex, LockUnlock) {
+ pw::sync::VirtualTimedMutex mutex;
+ mutex.lock();
+ // TODO(pwbug/291): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(mutex.try_lock());
+ mutex.unlock();
+}
+
+VirtualTimedMutex static_virtual_mutex;
+TEST(VirtualTimedMutex, LockUnlockStatic) {
+ static_virtual_mutex.lock();
+ // TODO(pwbug/291): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(static_virtual_mutex.try_lock());
+ static_virtual_mutex.unlock();
+}
+
TEST(TimedMutex, LockUnlockInC) {
pw::sync::TimedMutex mutex;
pw_sync_TimedMutex_CallLock(&mutex);
