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

@@ -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.

src/autoload.h

@@ -46,7 +46,7 @@ class env_vars_snapshot_t;
 class autoload_t : public lru_cache_t<autoload_t, autoload_function_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.

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<std::mutex *>(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();
     }
 }
 

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<char_t>.
 void convert_wide_array_to_narrow(const null_terminated_array_t<wchar_t> &arr,
                                   null_terminated_array_t<char> *output);
-typedef std::lock_guard<std::mutex> scoped_lock;
+typedef std::lock_guard<fish_mutex_t> scoped_lock;
 typedef std::lock_guard<std::recursive_mutex> scoped_rlock;
 
 // An object wrapping a scoped lock and a value
@@ -535,7 +564,7 @@ typedef std::lock_guard<std::recursive_mutex> scoped_rlock;
 template <typename DATA>
 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 <typename T>
     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:

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<wcstring, wcstring_list_t> wrapper_map_t;
 static wrapper_map_t &wrap_map() {
     ASSERT_IS_LOCKED(wrapper_lock);

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);
 

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);

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();

src/iothread.cpp

@@ -72,9 +72,9 @@ static owning_lock<thread_data_t> s_spawn_requests;
 static owning_lock<std::queue<spawn_request_t>> 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<main_thread_request_t *> 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<std::mutex> perform_lock(s_main_thread_performer_lock);
+    std::unique_lock<std::mutex> 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