From bfe08a471dbcee62d21c95fcfd11dfdf5c909c1a Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sun, 30 Dec 2018 20:15:49 -0600 Subject: [PATCH] Remove fish_mutex_t wrapper around std::mutex @ridiculousfish had introduced this in 3a45cad12ea392bbc5fe38010d1cdf4a90ba7adb to work around an issue with Coverity Scan where it couldn't tell the mutex was correctly locked, but even with the `fish_mutex_t` hack, it still emits the same warnings, so there's no pointing in keeping it. --- src/autoload.h | 2 +- src/common.cpp | 11 +++++++--- src/common.h | 41 ++++++-------------------------------- src/complete.cpp | 4 +--- src/env.cpp | 2 +- src/env_universal_common.h | 2 +- src/history.h | 2 +- src/iothread.cpp | 6 +++--- 8 files changed, 22 insertions(+), 48 deletions(-) diff --git a/src/autoload.h b/src/autoload.h index 83614b761..ceb703c53 100644 --- a/src/autoload.h +++ b/src/autoload.h @@ -46,7 +46,7 @@ class env_vars_snapshot_t; class autoload_t : public lru_cache_t { private: /// Lock for thread safety. - fish_mutex_t lock; + std::mutex lock; /// The environment variable name. const wcstring env_var_name; /// The paths from which to autoload, or missing if none. diff --git a/src/common.cpp b/src/common.cpp index 94367bdac..e13f68738 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -78,7 +78,7 @@ static pid_t initial_fg_process_group = -1; /// This struct maintains the current state of the terminal size. It is updated on demand after /// receiving a SIGWINCH. Do not touch this struct directly, it's managed with a rwlock. Use /// common_get_width()/common_get_height(). -static fish_mutex_t termsize_lock; +static std::mutex termsize_lock; static struct winsize termsize = {USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX}; static volatile bool termsize_valid = false; @@ -2234,11 +2234,16 @@ void assert_is_background_thread(const char *who) { } } -void fish_mutex_t::assert_is_locked(const char *who, const char *caller) const { - if (!is_locked_) { +void assert_is_locked(void *vmutex, const char *who, const char *caller) { + std::mutex *mutex = static_cast(vmutex); + + // Note that std::mutex.try_lock() is allowed to return false when the mutex isn't + // actually locked; fortunately we are checking the opposite so we're safe. + if (mutex->try_lock()) { debug(0, "%s is not locked when it should be in '%s'", who, caller); debug(0, "Break on debug_thread_error to debug."); debug_thread_error(); + mutex->unlock(); } } diff --git a/src/common.h b/src/common.h index 49a381f0f..889036841 100644 --- a/src/common.h +++ b/src/common.h @@ -543,39 +543,10 @@ void assert_is_background_thread(const char *who); #define ASSERT_IS_BACKGROUND_THREAD_TRAMPOLINE(x) assert_is_background_thread(x) #define ASSERT_IS_BACKGROUND_THREAD() ASSERT_IS_BACKGROUND_THREAD_TRAMPOLINE(__FUNCTION__) -// fish_mutex is a wrapper around std::mutex that tracks whether it is locked, allowing for checking -// if the mutex is locked. It owns a boolean guarded by the lock that records whether the lock is -// currently locked; this is only used by assertions for correctness. -class fish_mutex_t { - std::mutex lock_{}; - bool is_locked_{false}; - - public: - constexpr fish_mutex_t() = default; - ~fish_mutex_t() = default; - - void lock() { - lock_.lock(); - is_locked_ = true; - } - - void unlock() { - is_locked_ = false; - lock_.unlock(); - } - - // assert that this lock (identified as 'who') is locked in the function 'caller'. - void assert_is_locked(const char *who, const char *caller) const; - - // return the underlying std::mutex. Note the fish_mutex_t cannot track locks to the underlying - // mutex; do not use assert_is_locked() with this. - std::mutex &get_mutex() { return lock_; } -}; - /// Useful macro for asserting that a lock is locked. This doesn't check whether this thread locked /// it, which it would be nice if it did, but here it is anyways. -void assert_is_locked(const fish_mutex_t &m, const char *who, const char *caller); -#define ASSERT_IS_LOCKED(x) (x).assert_is_locked(#x, __FUNCTION__) +void assert_is_locked(void *mutex, const char *who, const char *caller); +#define ASSERT_IS_LOCKED(x) assert_is_locked((void *)(&x), #x, __FUNCTION__) /// Format the specified size (in bytes, kilobytes, etc.) into the specified stringbuffer. wcstring format_size(long long sz); @@ -706,7 +677,7 @@ class null_terminated_array_t { // null_terminated_array_t. void convert_wide_array_to_narrow(const null_terminated_array_t &arr, null_terminated_array_t *output); -typedef std::lock_guard scoped_lock; +typedef std::lock_guard scoped_lock; typedef std::lock_guard scoped_rlock; // An object wrapping a scoped lock and a value @@ -721,8 +692,8 @@ typedef std::lock_guard scoped_rlock; // template class acquired_lock { - std::unique_lock lock; - acquired_lock(fish_mutex_t &lk, DATA *v) : lock(lk), value(v) {} + std::unique_lock lock; + acquired_lock(std::mutex &lk, DATA *v) : lock(lk), value(v) {} template friend class owning_lock; @@ -752,7 +723,7 @@ class owning_lock { owning_lock(owning_lock &&) = default; owning_lock &operator=(owning_lock &&) = default; - fish_mutex_t lock; + std::mutex lock; DATA data; public: diff --git a/src/complete.cpp b/src/complete.cpp index c6f2bf432..440d5f3c9 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -1253,9 +1253,7 @@ bool completer_t::try_complete_user(const wcstring &str) { bool result = false; size_t name_len = str.length() - 1; - // We don't bother with the thread-safe `getpwent_r()` variant because this is the sole place - // where we call getpwent(). - static fish_mutex_t lock; + static std::mutex lock; scoped_lock locker(lock); setpwent(); // cppcheck-suppress getpwentCalled diff --git a/src/env.cpp b/src/env.cpp index 22e8c1097..4085b8576 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -139,7 +139,7 @@ class env_node_t { bool contains_any_of(const wcstring_list_t &vars) const; }; -static fish_mutex_t env_lock; +static std::mutex env_lock; static bool local_scope_exports(const env_node_t *n); diff --git a/src/env_universal_common.h b/src/env_universal_common.h index 6e055db68..3a8daf6c9 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -52,7 +52,7 @@ class env_universal_t { // fish wrote the uvars contents. bool ok_to_save{true}; - mutable fish_mutex_t lock; + mutable std::mutex lock; bool load_from_path(const wcstring &path, callback_data_list_t &callbacks); void load_from_fd(int fd, callback_data_list_t &callbacks); diff --git a/src/history.h b/src/history.h index 08e6f36c4..6419cdcc9 100644 --- a/src/history.h +++ b/src/history.h @@ -122,7 +122,7 @@ class history_t { void add(const history_item_t &item, bool pending = false); // Lock for thread safety. - fish_mutex_t lock; + std::mutex lock; // Internal function. void clear_file_state(); diff --git a/src/iothread.cpp b/src/iothread.cpp index c3fc55736..d834da8c1 100644 --- a/src/iothread.cpp +++ b/src/iothread.cpp @@ -72,9 +72,9 @@ static owning_lock s_spawn_requests; static owning_lock> s_result_queue; // "Do on main thread" support. -static fish_mutex_t s_main_thread_performer_lock; // protects the main thread requests +static std::mutex s_main_thread_performer_lock; // protects the main thread requests static std::condition_variable s_main_thread_performer_cond; // protects the main thread requests -static fish_mutex_t s_main_thread_request_q_lock; // protects the queue +static std::mutex s_main_thread_request_q_lock; // protects the queue static std::queue s_main_thread_request_queue; // Notifying pipes. @@ -332,7 +332,7 @@ void iothread_perform_on_main(void_function_t &&func) { assert_with_errno(write_loop(s_write_pipe, &wakeup_byte, sizeof wakeup_byte) != -1); // Wait on the condition, until we're done. - std::unique_lock perform_lock(s_main_thread_performer_lock.get_mutex()); + std::unique_lock perform_lock(s_main_thread_performer_lock); while (!req.done) { // It would be nice to support checking for cancellation here, but the clients need a // deterministic way to clean up to avoid leaks