From 3a45cad12ea392bbc5fe38010d1cdf4a90ba7adb Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 8 Feb 2018 21:59:52 -0800 Subject: [PATCH] Introduce fish_mutex_t wrapping std::mutex Add a fish-specific wrapper around std::mutex that records whether it is locked in a bool. This is to make ASSERT_IS_LOCKED() simpler (it can just check the boolean instead of relying on try_lock) which will make Coverity Scan happier. Some details: Coverity Scan was complaining about an apparent double-unlock because it's unaware of the semantics of try_lock(). Specifically fish asserts that a lock is locked by asserting that try_lock fails; if it succeeds fish prints an error and then unlocks the lock (so as not to leave it locked). This unlock is of course correct, but it confused Coverity Scan. --- src/autoload.cpp | 2 +- src/autoload.h | 2 +- src/common.cpp | 11 +++-------- src/common.h | 39 +++++++++++++++++++++++++++++++++----- src/complete.cpp | 4 ++-- src/env.cpp | 2 +- src/env_universal_common.h | 2 +- src/history.h | 2 +- src/iothread.cpp | 6 +++--- 9 files changed, 47 insertions(+), 23 deletions(-) diff --git a/src/autoload.cpp b/src/autoload.cpp index 2e06d4da6..fbb970f3b 100644 --- a/src/autoload.cpp +++ b/src/autoload.cpp @@ -48,7 +48,7 @@ file_access_attempt_t access_file(const wcstring &path, int mode) { autoload_t::autoload_t(const wcstring &env_var_name_var, command_removed_function_t cmd_removed_callback) - : lock(), env_var_name(env_var_name_var), command_removed(cmd_removed_callback) {} + : env_var_name(env_var_name_var), command_removed(cmd_removed_callback) {} void autoload_t::entry_was_evicted(wcstring key, autoload_function_t node) { // This should only ever happen on the main thread. diff --git a/src/autoload.h b/src/autoload.h index fe3372cb2..db755315f 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. - std::mutex lock; + fish_mutex_t lock; /// The environment variable name. const wcstring env_var_name; /// The path from which we most recently autoloaded. diff --git a/src/common.cpp b/src/common.cpp index 98fc57238..6ea7094cb 100644 --- a/src/common.cpp +++ b/src/common.cpp @@ -67,7 +67,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 std::mutex termsize_lock; +static fish_mutex_t termsize_lock; static struct winsize termsize = {USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX}; static volatile bool termsize_valid = false; @@ -1983,16 +1983,11 @@ void assert_is_background_thread(const char *who) { } } -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()) { +void fish_mutex_t::assert_is_locked(const char *who, const char *caller) const { + if (!is_locked_) { 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 04a53ce76..d3116995d 100644 --- a/src/common.h +++ b/src/common.h @@ -397,10 +397,39 @@ 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(void *mutex, const char *who, const char *caller); -#define ASSERT_IS_LOCKED(x) assert_is_locked((void *)(&x), #x, __FUNCTION__) +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__) /// Format the specified size (in bytes, kilobytes, etc.) into the specified stringbuffer. wcstring format_size(long long sz); @@ -519,7 +548,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 @@ -535,7 +564,7 @@ typedef std::lock_guard scoped_rlock; template class acquired_lock { scoped_lock lock; - acquired_lock(std::mutex &lk, DATA *v) : lock(lk), value(*v) {} + acquired_lock(fish_mutex_t &lk, DATA *v) : lock(lk), value(*v) {} template friend class owning_lock; @@ -560,7 +589,7 @@ class owning_lock { owning_lock(owning_lock &&) = default; owning_lock &operator=(owning_lock &&) = default; - std::mutex lock; + fish_mutex_t lock; DATA data; public: diff --git a/src/complete.cpp b/src/complete.cpp index 00f9e6cd1..d19bca649 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -183,7 +183,7 @@ static bool compare_completions_by_order(const completion_entry_t *p1, } /// The lock that guards the list of completion entries. -static std::mutex completion_lock; +static fish_mutex_t completion_lock; void completion_entry_t::add_option(const complete_entry_opt_t &opt) { ASSERT_IS_LOCKED(completion_lock); @@ -1559,7 +1559,7 @@ wcstring complete_print() { } /// Completion "wrapper" support. The map goes from wrapping-command to wrapped-command-list. -static std::mutex wrapper_lock; +static fish_mutex_t wrapper_lock; typedef std::unordered_map wrapper_map_t; static wrapper_map_t &wrap_map() { ASSERT_IS_LOCKED(wrapper_lock); diff --git a/src/env.cpp b/src/env.cpp index b51b8729f..877754b24 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -162,7 +162,7 @@ class variable_entry_t { wcstring value; /**< Value of the variable */ }; -static std::mutex env_lock; +static fish_mutex_t 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 43c8d78ca..842ae01a0 100644 --- a/src/env_universal_common.h +++ b/src/env_universal_common.h @@ -39,7 +39,7 @@ class env_universal_t { // Path that we save to. If empty, use the default. const wcstring explicit_vars_path; - mutable std::mutex lock; + mutable fish_mutex_t lock; bool tried_renaming; 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 da4ae2e92..da5ec154a 100644 --- a/src/history.h +++ b/src/history.h @@ -120,7 +120,7 @@ class history_t { void add(const history_item_t &item, bool pending = false); // Lock for thread safety. - std::mutex lock; + fish_mutex_t lock; // Internal function. void clear_file_state(); diff --git a/src/iothread.cpp b/src/iothread.cpp index e3ca9f72c..53807eca4 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 std::mutex s_main_thread_performer_lock; // protects the main thread requests +static fish_mutex_t 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 std::mutex s_main_thread_request_q_lock; // protects the queue +static fish_mutex_t s_main_thread_request_q_lock; // protects the queue static std::queue s_main_thread_request_queue; // Notifying pipes. @@ -334,7 +334,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); + std::unique_lock perform_lock(s_main_thread_performer_lock.get_mutex()); 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