From 36998eee55fb8bda6aa49af9ac795f9eb481636a Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 28 Apr 2019 18:13:55 -0700 Subject: [PATCH] Make more miscellaneous globals thread safe --- build_tools/find_globals.fish | 6 +++--- src/env.cpp | 4 ++-- src/env_dispatch.cpp | 5 +++-- src/global_safety.h | 18 +++++++++++++++--- src/history.cpp | 9 +++++---- src/history.h | 2 +- src/parse_execution.cpp | 11 +++++------ src/parser.h | 3 +++ src/reader.cpp | 29 ++++++++++++++--------------- src/reader.h | 2 +- src/sanity.cpp | 3 ++- src/wutil.cpp | 8 ++++---- 12 files changed, 58 insertions(+), 42 deletions(-) diff --git a/build_tools/find_globals.fish b/build_tools/find_globals.fish index 5955c5f33..5aaa1f4e9 100755 --- a/build_tools/find_globals.fish +++ b/build_tools/find_globals.fish @@ -74,18 +74,18 @@ end function decl_is_threadsafe set -l vardecl $argv[1] # decls starting with 'const ' or containing ' const ' are assumed safe. - string match -q --regex '(^| )const ' $vardecl + string match -q --regex '(^|\\*| )const ' $vardecl and return 0 # Ordinary types indicating a safe variable. - set safes relaxed_atomic_bool_t std::mutex std::condition_variable + set safes relaxed_atomic_bool_t std::mutex std::condition_variable std::once_flag sig_atomic_t for safe in $safes string match -q "*$safe*" $vardecl and return 0 end # Template types indicate a safe variable. - set safes owning_lock mainthread_t std::atomic relaxed_atomic_t + set safes owning_lock mainthread_t std::atomic relaxed_atomic_t latch_t for safe in $safes string match -q "*$safe<*" $vardecl and return 0 diff --git a/src/env.cpp b/src/env.cpp index 0a2806cf8..1d38966e9 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -167,8 +167,8 @@ env_var_t::env_var_flags_t env_var_t::flags_for(const wchar_t *name) { /// \return a singleton empty list, to avoid unnecessary allocations in env_var_t. std::shared_ptr env_var_t::empty_list() { - static const auto result = std::make_shared(); - return result; + static const auto s_empty_result = std::make_shared(); + return s_empty_result; } environment_t::~environment_t() = default; diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp index b8f85cb47..0778db918 100644 --- a/src/env_dispatch.cpp +++ b/src/env_dispatch.cpp @@ -39,6 +39,7 @@ #include "event.h" #include "fallback.h" // IWYU pragma: keep #include "function.h" +#include "global_safety.h" #include "history.h" #include "input_common.h" #include "maybe.h" @@ -115,7 +116,7 @@ static void init_locale(const environment_t &vars); static void update_fish_color_support(const environment_t &vars); /// True if we think we can set the terminal title. -static bool can_set_term_title = false; +static relaxed_atomic_bool_t can_set_term_title{false}; // Run those dispatch functions which want to be run at startup. static void run_inits(const environment_t &vars); @@ -126,7 +127,7 @@ static std::unique_ptr create_dispatch_table(); // A pointer to the variable dispatch table. This is allocated with new() and deliberately leaked to // avoid shutdown destructors. This is set during startup and should not be modified after. -static const var_dispatch_table_t *s_var_dispatch_table; +static latch_t s_var_dispatch_table; void env_dispatch_init(const environment_t &vars) { run_inits(vars); diff --git a/src/global_safety.h b/src/global_safety.h index b19dec31f..41b76016c 100644 --- a/src/global_safety.h +++ b/src/global_safety.h @@ -63,11 +63,17 @@ class latch_t : detail::fixed_t { T *operator->() { return value_; } const T *operator->() const { return value_; } - template - void emplace(Args &&... args) { + void operator=(T *value) { ASSERT_IS_MAIN_THREAD(); assert(value_ == nullptr && "Latch variable initialized multiple times"); - value_ = new T(std::forward(args)...); + assert(value != nullptr && "Latch variable initialized with null"); + value_ = value; + } + + template + void emplace(Args &&... args) { + // Note: deliberate leak. + *this = new T(std::forward(args)...); } }; @@ -83,6 +89,12 @@ class relaxed_atomic_t { operator T() const { return value_.load(std::memory_order_relaxed); } void operator=(T v) { return value_.store(v, std::memory_order_relaxed); } + + // postincrement + T operator++(int) { return value_.fetch_add(1, std::memory_order_relaxed); } + + // preincrement + T operator++() { return 1 + value_.fetch_add(1, std::memory_order_relaxed); } }; using relaxed_atomic_bool_t = relaxed_atomic_t; diff --git a/src/history.cpp b/src/history.cpp index 12ef8b14f..02ba92aa1 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -31,13 +31,14 @@ #include "common.h" #include "env.h" #include "fallback.h" // IWYU pragma: keep +#include "global_safety.h" #include "history.h" #include "io.h" #include "iothread.h" #include "lru.h" -#include "parser.h" #include "parse_constants.h" #include "parse_util.h" +#include "parser.h" #include "path.h" #include "reader.h" #include "tnode.h" @@ -858,7 +859,7 @@ void history_t::save_internal_unless_disabled() { // the counter. const int kVacuumFrequency = 25; if (countdown_to_vacuum < 0) { - static unsigned int seed = (unsigned int)time(NULL); + unsigned int seed = (unsigned int)time(NULL); // Generate a number in the range [0, kVacuumFrequency). countdown_to_vacuum = rand_r(&seed) / (RAND_MAX / kVacuumFrequency + 1); } @@ -1939,8 +1940,8 @@ void history_t::add_pending_with_file_detection(const wcstring &str, history_identifier_t identifier = 0; if (!potential_paths.empty() && !impending_exit) { // Grab the next identifier. - static history_identifier_t sLastIdentifier = 0; - identifier = ++sLastIdentifier; + static relaxed_atomic_t s_last_identifier{0}; + identifier = ++s_last_identifier; // Prevent saving until we're done, so we have time to get the paths. this->disable_automatic_saving(); diff --git a/src/history.h b/src/history.h index 033766060..d0738f4fe 100644 --- a/src/history.h +++ b/src/history.h @@ -57,7 +57,7 @@ enum history_search_type_t { HISTORY_SEARCH_TYPE_PREFIX_GLOB }; -typedef uint32_t history_identifier_t; +typedef uint64_t history_identifier_t; class history_item_t { friend class history_t; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 273a71afc..bb334f238 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -795,7 +795,6 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( } // Protect against exec with background processes running - static uint32_t last_exec_run_counter = -1; if (process_type == process_type_t::exec && shell_is_interactive()) { bool have_bg = false; for (const auto &bg : jobs()) { @@ -809,13 +808,13 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( } if (have_bg) { - /* debug(1, "Background jobs remain! run_counter: %u, last_exec_run_count: %u", reader_run_count(), last_exec_run_counter); */ - if (isatty(STDIN_FILENO) && reader_run_count() - 1 != last_exec_run_counter) { + uint64_t current_run_count = reader_run_count(); + uint64_t &last_exec_run_count = parser->libdata().last_exec_run_counter; + if (isatty(STDIN_FILENO) && current_run_count - 1 != last_exec_run_count) { reader_bg_job_warning(); - last_exec_run_counter = reader_run_count(); + last_exec_run_count = current_run_count; return parse_execution_errored; - } - else { + } else { hup_background_jobs(); } } diff --git a/src/parser.h b/src/parser.h index ec3fde14f..0796e388a 100644 --- a/src/parser.h +++ b/src/parser.h @@ -152,6 +152,9 @@ struct library_data_t { /// A counter incremented every time a command executes. uint64_t exec_count{0}; + /// Last reader run count. + uint64_t last_exec_run_counter{UINT64_MAX}; + /// Number of recursive calls to builtin_complete(). uint32_t builtin_complete_recursion_level{0}; }; diff --git a/src/reader.cpp b/src/reader.cpp index 31d401329..bec8579e8 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -53,6 +53,7 @@ #include "expand.h" #include "fallback.h" // IWYU pragma: keep #include "function.h" +#include "global_safety.h" #include "highlight.h" #include "history.h" #include "input.h" @@ -500,17 +501,15 @@ static struct termios terminal_mode_on_startup; static struct termios tty_modes_for_external_cmds; /// Tracks a currently pending exit. This may be manipulated from a signal handler. -struct { - /// Whether we should exit the current reader loop. - bool end_current_loop{false}; - /// Whether we should exit all reader loops. This is set in response to a HUP signal and it - /// latches (once set it is never cleared). This should never be reset to false. - volatile bool force{false}; +/// Whether we should exit the current reader loop. +static relaxed_atomic_bool_t s_end_current_loop{false}; - bool should_exit() const { return end_current_loop || force; } +/// Whether we should exit all reader loops. This is set in response to a HUP signal and it +/// latches (once set it is never cleared). This should never be reset to false. +static volatile sig_atomic_t s_exit_forced{false}; -} s_pending_exit; +static bool should_exit() { return s_end_current_loop || s_exit_forced; } /// Give up control of terminal. static void term_donate(outputter_t &outp) { @@ -546,7 +545,7 @@ static void term_steal() { invalidate_termsize(); } -bool reader_exit_forced() { return s_pending_exit.force; } +bool reader_exit_forced() { return s_exit_forced; } /// Given a command line and an autosuggestion, return the string that gets shown to the user. wcstring combine_command_and_autosuggestion(const wcstring &cmdline, @@ -1016,11 +1015,11 @@ void restore_term_mode() { } /// Exit the current reader loop. This may be invoked from a signal handler. -void reader_set_end_loop(bool flag) { s_pending_exit.end_current_loop = flag; } +void reader_set_end_loop(bool flag) { s_end_current_loop = flag; } void reader_force_exit() { // Beware, we may be in a signal handler. - s_pending_exit.force = true; + s_exit_forced = true; } /// Indicates if the given command char ends paging. @@ -2139,7 +2138,7 @@ void reader_pop() { if (new_reader == nullptr) { reader_interactive_destroy(); } else { - s_pending_exit.end_current_loop = false; + s_end_current_loop = false; s_reset(&new_reader->screen, screen_reset_abandon_line); } } @@ -2192,7 +2191,7 @@ void reader_import_history_if_necessary() { } } -bool shell_is_exiting() { return s_pending_exit.should_exit(); } +bool shell_is_exiting() { return should_exit(); } void reader_bg_job_warning() { std::fputws(_(L"There are still jobs active:\n"), stdout); @@ -2254,10 +2253,10 @@ static bool selection_is_at_top() { return true; } -static uint32_t run_count = 0; +static relaxed_atomic_t run_count{0}; /// Returns the current interactive loop count -uint32_t reader_run_count() { return run_count; } +uint64_t reader_run_count() { return run_count; } /// Read interactively. Read input from stdin while providing editing facilities. static int read_i() { diff --git a/src/reader.h b/src/reader.h index 4be1328a0..59cbbbc68 100644 --- a/src/reader.h +++ b/src/reader.h @@ -236,6 +236,6 @@ void reader_bg_job_warning(); /// Return the current interactive reads loop count. Useful for determining how many commands have /// been executed between invocations of code. -uint32_t reader_run_count(); +uint64_t reader_run_count(); #endif diff --git a/src/sanity.cpp b/src/sanity.cpp index 4019cafea..4b173539f 100644 --- a/src/sanity.cpp +++ b/src/sanity.cpp @@ -5,6 +5,7 @@ #include "common.h" #include "fallback.h" // IWYU pragma: keep +#include "global_safety.h" #include "history.h" #include "kill.h" #include "proc.h" @@ -12,7 +13,7 @@ #include "sanity.h" /// Status from earlier sanity checks. -static bool insane = false; +static relaxed_atomic_bool_t insane{false}; void sanity_lose() { debug(0, _(L"Errors detected, shutting down. Break on sanity_lose() to debug.")); diff --git a/src/wutil.cpp b/src/wutil.cpp index 8ccb9362e..1565725a2 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include @@ -22,6 +21,7 @@ #include #include +#include #include #include @@ -534,8 +534,8 @@ static void wgettext_really_init() { /// For wgettext: Internal init function. Automatically called when a translation is first /// requested. static void wgettext_init_if_necessary() { - static pthread_once_t once = PTHREAD_ONCE_INIT; - pthread_once(&once, wgettext_really_init); + static std::once_flag s_wgettext_init{}; + std::call_once(s_wgettext_init, wgettext_really_init); } const wcstring &wgettext(const wchar_t *in) { @@ -614,7 +614,7 @@ int fish_wcswidth(const wchar_t *str) { return fish_wcswidth(str, std::wcslen(st int fish_wcswidth(const wcstring &str) { return fish_wcswidth(str.c_str(), str.size()); } locale_t fish_c_locale() { - static locale_t loc = newlocale(LC_ALL_MASK, "C", NULL); + static const locale_t loc = newlocale(LC_ALL_MASK, "C", NULL); return loc; }