Eliminate the termsize handling from common.h

Finish the transition to termsize.h. Remove the scary termsize bits
from common.cpp, which can throw off events at arbitrary calls and are
dangerously reentrant. Migrate everyone to the new termsize.h.

src/common.cpp

@@ -28,12 +28,6 @@
 #ifdef HAVE_EXECINFO_H
 #include <execinfo.h>
 #endif
-#ifdef HAVE_SIGINFO_H
-#include <siginfo.h>
-#endif
-#ifdef HAVE_SYS_IOCTL_H
-#include <sys/ioctl.h>
-#endif
 
 #ifdef __linux__
 // Includes for WSL detection
@@ -101,13 +95,6 @@ int get_debug_stack_frames() { return debug_stack_frames; }
 /// This is set during startup and not modified after.
 static relaxed_atomic_t<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. Use common_get_width()/common_get_height() to read it lazily.
-static owning_lock<struct winsize> s_termsize{
-    (struct winsize){USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX}};
-
-static relaxed_atomic_bool_t s_termsize_valid{false};
-
 static char *wcs2str_internal(const wchar_t *in, char *out);
 static void debug_shared(wchar_t msg_level, const wcstring &msg);
 
@@ -1747,106 +1734,6 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e
     return success;
 }
 
-/// Used to invalidate our idea of having a valid window size. This can occur when either the
-/// COLUMNS or LINES variables are changed. This is also invoked when the shell regains control of
-/// the tty since it is possible the terminal size changed while an external command was running.
-void invalidate_termsize(bool invalidate_vars) {
-    s_termsize_valid = false;
-    if (invalidate_vars) {
-        auto termsize = s_termsize.acquire();
-        termsize->ws_col = termsize->ws_row = USHRT_MAX;
-    }
-}
-
-/// Handle SIGWINCH. This is also invoked when the shell regains control of the tty since it is
-/// possible the terminal size changed while an external command was running.
-void common_handle_winch(int signal) {
-    (void)signal;
-    s_termsize_valid = false;
-}
-
-/// Validate the new terminal size. Fallback to the env vars if necessary.
-static void validate_new_termsize(struct winsize *new_termsize, const environment_t &vars) {
-    if (new_termsize->ws_col == 0 || new_termsize->ws_row == 0) {
-#ifdef HAVE_WINSIZE
-        // Highly hackish. This seems like it should be moved.
-        if (is_main_thread() && parser_t::principal_parser().is_interactive()) {
-            FLOGF(warning, _(L"Current terminal parameters have rows and/or columns set to zero."));
-            FLOGF(warning, _(L"The stty command can be used to correct this "
-                             L"(e.g., stty rows 80 columns 24)."));
-        }
-#endif
-        // Fallback to the environment vars.
-        maybe_t<env_var_t> col_var = vars.get(L"COLUMNS");
-        maybe_t<env_var_t> row_var = vars.get(L"LINES");
-        if (!col_var.missing_or_empty() && !row_var.missing_or_empty()) {
-            // Both vars have to have valid values.
-            int col = fish_wcstoi(col_var->as_string().c_str());
-            bool col_ok = errno == 0 && col > 0 && col <= USHRT_MAX;
-            int row = fish_wcstoi(row_var->as_string().c_str());
-            bool row_ok = errno == 0 && row > 0 && row <= USHRT_MAX;
-            if (col_ok && row_ok) {
-                new_termsize->ws_col = col;
-                new_termsize->ws_row = row;
-            }
-        }
-    }
-}
-
-/// Export the new terminal size as env vars and to the kernel if possible.
-static void export_new_termsize(struct winsize *new_termsize, env_stack_t &vars) {
-    auto cols = vars.get(L"COLUMNS", ENV_EXPORT);
-    vars.set_one(L"COLUMNS", ENV_GLOBAL | (cols.missing_or_empty() ? ENV_DEFAULT : ENV_EXPORT),
-                 std::to_wstring(int(new_termsize->ws_col)));
-
-    auto lines = vars.get(L"LINES", ENV_EXPORT);
-    vars.set_one(L"LINES", ENV_GLOBAL | (lines.missing_or_empty() ? ENV_DEFAULT : ENV_EXPORT),
-                 std::to_wstring(int(new_termsize->ws_row)));
-}
-
-/// Get the current termsize, lazily computing it. Return by reference if it changed.
-static struct winsize get_current_winsize_prim(bool *changed, const environment_t &vars) {
-    auto termsize = s_termsize.acquire();
-    if (s_termsize_valid) return *termsize;
-
-    struct winsize new_termsize = {0, 0, 0, 0};
-#ifdef HAVE_WINSIZE
-    errno = 0;
-    if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &new_termsize) != -1 &&
-        new_termsize.ws_col == termsize->ws_col && new_termsize.ws_row == termsize->ws_row) {
-        s_termsize_valid = true;
-        return *termsize;
-    }
-#endif
-    validate_new_termsize(&new_termsize, vars);
-    termsize->ws_col = new_termsize.ws_col;
-    termsize->ws_row = new_termsize.ws_row;
-    *changed = true;
-    s_termsize_valid = true;
-    return *termsize;
-}
-
-/// Updates termsize as needed, and returns a copy of the winsize.
-struct winsize get_current_winsize() {
-    bool changed = false;
-    auto &vars = env_stack_t::globals();
-    struct winsize termsize = get_current_winsize_prim(&changed, vars);
-    if (changed) {
-        // TODO: this may call us reentrantly through the environment dispatch mechanism. We need to
-        // rationalize this.
-        export_new_termsize(&termsize, vars);
-        // Hack: due to the dispatch the termsize may have just become invalid. Stomp it back to
-        // valid. What a mess.
-        *s_termsize.acquire() = termsize;
-        s_termsize_valid = true;
-    }
-    return termsize;
-}
-
-int common_get_width() { return get_current_winsize().ws_col; }
-
-int common_get_height() { return get_current_winsize().ws_row; }
-
 /// Returns true if seq, represented as a subsequence, is contained within string.
 static bool subsequence_in_string(const wcstring &seq, const wcstring &str) {
     // Impossible if seq is larger than string.

src/common.h

@@ -648,21 +648,6 @@ bool unescape_string(const wchar_t *input, wcstring *output, unescape_flags_t es
 bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t escape_special,
                      escape_string_style_t style = STRING_STYLE_SCRIPT);
 
-/// Returns the width of the terminal window, so that not all functions that use these values
-/// continually have to keep track of it separately.
-///
-/// Only works if common_handle_winch is registered to handle winch signals.
-int common_get_width();
-
-/// Returns the height of the terminal window, so that not all functions that use these values
-/// continually have to keep track of it separatly.
-///
-/// Only works if common_handle_winch is registered to handle winch signals.
-int common_get_height();
-
-/// Handle a window change event by looking up the new window size and saving it in an internal
-/// variable used by common_get_wisth and common_get_height().
-void common_handle_winch(int signal);
 
 /// Write the given paragraph of output, redoing linebreaks to fit \p termsize.
 wcstring reformat_for_screen(const wcstring &msg, const termsize_t &termsize);
@@ -776,9 +761,6 @@ void redirect_tty_output();
 
 std::string get_path_to_tmp_dir();
 
-void invalidate_termsize(bool invalidate_vars = false);
-struct winsize get_current_winsize();
-
 bool valid_var_name_char(wchar_t chr);
 bool valid_var_name(const wcstring &str);
 bool valid_func_name(const wcstring &str);

src/env_dispatch.cpp

@@ -51,6 +51,7 @@
 #include "proc.h"
 #include "reader.h"
 #include "screen.h"
+#include "termsize.h"
 #include "wutil.h"  // IWYU pragma: keep
 
 #define DEFAULT_TERM1 "ansi"
@@ -231,8 +232,7 @@ static void handle_change_ambiguous_width(const env_stack_t &vars) {
 }
 
 static void handle_term_size_change(const env_stack_t &vars) {
-    UNUSED(vars);
-    invalidate_termsize(true);  // force fish to update its idea of the terminal size plus vars
+    termsize_container_t::shared().handle_columns_lines_var_change(vars);
 }
 
 static void handle_fish_history_change(const env_stack_t &vars) {

src/reader.cpp

@@ -74,6 +74,7 @@
 #include "sanity.h"
 #include "screen.h"
 #include "signal.h"
+#include "termsize.h"
 #include "tnode.h"
 #include "tokenizer.h"
 #include "wutil.h"  // IWYU pragma: keep
@@ -621,6 +622,9 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
     void update_command_line_from_history_search();
     void set_buffer_maintaining_pager(const wcstring &b, size_t pos, bool transient = false);
     void delete_char(bool backward = true);
+
+    /// Called to update the termsize, including $COLUMNS and $LINES, as necessary.
+    void update_termsize() { (void)termsize_container_t::shared().updating(parser()); }
 };
 
 /// This variable is set to a signal by the signal handler when ^C is pressed.
@@ -695,7 +699,7 @@ static void term_steal() {
             break;
     }
 
-    invalidate_termsize();
+    termsize_container_t::shared().invalidate_tty();
 }
 
 bool reader_exit_forced() { return s_exit_forced; }
@@ -800,8 +804,7 @@ void reader_data_t::repaint() {
     size_t cursor_position = focused_on_pager ? pager.cursor_position() : cmd_line->position();
 
     // Prepend the mode prompt to the left prompt.
-    int screen_width = common_get_width();
-    s_write(&screen, screen_width, mode_prompt_buff + left_prompt_buff, right_prompt_buff,
+    s_write(&screen, termsize_last().width, mode_prompt_buff + left_prompt_buff, right_prompt_buff,
             full_line, cmd_line->size(), colors, indents, cursor_position, current_page_rendering,
             focused_on_pager);
 
@@ -1053,9 +1056,9 @@ void reader_data_t::exec_prompt() {
     // Do not allow the exit status of the prompts to leak through.
     const bool apply_exit_status = false;
 
-    // HACK: Query winsize again because it might have changed.
+    // Update the termsize now.
     // This allows prompts to react to $COLUMNS.
-    (void)get_current_winsize();
+    update_termsize();
 
     // If we have any prompts, they must be run non-interactively.
     if (!left_prompt.empty() || !right_prompt.empty()) {
@@ -1091,7 +1094,8 @@ void reader_data_t::exec_prompt() {
 }
 
 void reader_init() {
-    auto &vars = parser_t::principal_parser().vars();
+    parser_t &parser = parser_t::principal_parser();
+    auto &vars = parser.vars();
 
     // Ensure this var is present even before an interactive command is run so that if it is used
     // in a function like `fish_prompt` or `fish_right_prompt` it is defined at the time the first
@@ -1121,7 +1125,7 @@ void reader_init() {
 
     // We do this not because we actually need the window size but for its side-effect of correctly
     // setting the COLUMNS and LINES env vars.
-    get_current_winsize();
+    termsize_container_t::shared().updating(parser);
 }
 
 /// Restore the term mode if we own the terminal. It's important we do this before
@@ -1972,7 +1976,7 @@ static void reader_interactive_init(parser_t &parser) {
         }
     }
 
-    invalidate_termsize();
+    termsize_container_t::shared().invalidate_tty();
 
     // For compatibility with fish 2.0's $_, now replaced with `status current-command`
     parser.vars().set_one(L"_", ENV_GLOBAL, L"fish");
@@ -2329,7 +2333,7 @@ void reader_pop() {
         reader_interactive_destroy();
     } else {
         s_end_current_loop = false;
-        s_reset_abandoning_line(&new_reader->screen, common_get_width());
+        s_reset_abandoning_line(&new_reader->screen, termsize_last().width);
     }
 }
 
@@ -2964,7 +2968,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
                 // already be printed, all we need to do is repaint.
                 wcstring_list_t argv(1, el->text());
                 event_fire_generic(parser(), L"fish_posterror", &argv);
-                s_reset_abandoning_line(&screen, common_get_width());
+                s_reset_abandoning_line(&screen, termsize_last().width);
                 mark_repaint_needed();
             }
 
@@ -3485,7 +3489,7 @@ maybe_t<wcstring> reader_data_t::readline(int nchars_or_0) {
 
     history_search.reset();
 
-    s_reset_abandoning_line(&screen, common_get_width());
+    s_reset_abandoning_line(&screen, termsize_last().width);
     event_fire_generic(parser(), L"fish_prompt");
     exec_prompt();
 
@@ -3510,6 +3514,9 @@ maybe_t<wcstring> reader_data_t::readline(int nchars_or_0) {
     }
 
     while (!rls.finished && !shell_is_exiting()) {
+        // Perhaps update the termsize. This is cheap if it has not changed.
+        update_termsize();
+
         if (rls.nchars <= command_line.size()) {
             // We've already hit the specified character limit.
             rls.finished = true;

src/signal.cpp

@@ -220,9 +220,8 @@ static void fish_signal_handler(int sig, siginfo_t *info, void *context) {
     switch (sig) {
 #ifdef SIGWINCH
         case SIGWINCH:
-            /// Respond to a winch signal by invalidating the terminal size.
+            /// Respond to a winch signal by telling the termsize container.
             termsize_container_t::handle_winch();
-            common_handle_winch(sig);
             break;
 #endif
 

src/termsize.cpp

@@ -6,9 +6,8 @@
 #include "parser.h"
 #include "wutil.h"
 
-// A counter which is incremented every SIGWINCH.
-// This is only updated from termsize_handle_winch().
-static volatile uint32_t s_sigwinch_gen_count{0};
+// A counter which is incremented every SIGWINCH, or when the tty is otherwise invalidated.
+static volatile uint32_t s_tty_termsize_gen_count{0};
 
 /// \return a termsize from ioctl, or none on error or if not supported.
 static maybe_t<termsize_t> read_termsize_from_tty() {
@@ -41,7 +40,7 @@ void termsize_container_t::data_t::mark_override_from_env(termsize_t ts) {
     // Here we pretend to have an up-to-date tty value so that we will prefer the environment value.
     this->last_from_env = ts;
     this->last_from_tty.reset();
-    this->last_winch_gen_count = s_sigwinch_gen_count;
+    this->last_tty_gen_count = s_tty_termsize_gen_count;
 }
 
 termsize_t termsize_container_t::last() const { return this->data_.acquire()->current(); }
@@ -58,11 +57,11 @@ termsize_t termsize_container_t::updating(parser_t &parser) {
 
         // Critical read of signal-owned variable.
         // This must happen before the TIOCGWINSZ ioctl.
-        const uint32_t sigwinch_gen_count = s_sigwinch_gen_count;
-        if (data->last_winch_gen_count != sigwinch_gen_count) {
-            // We have received a sigwinch (or we have not yet computed the value).
+        const uint32_t tty_gen_count = s_tty_termsize_gen_count;
+        if (data->last_tty_gen_count != tty_gen_count) {
+            // Our idea of the size of the terminal may be stale.
             // Apply any updates.
-            data->last_winch_gen_count = sigwinch_gen_count;
+            data->last_tty_gen_count = tty_gen_count;
             data->last_from_tty = this->tty_size_reader_();
         }
         new_size = data->current();
@@ -103,7 +102,7 @@ termsize_t termsize_container_t::initialize(const environment_t &vars) {
     if (new_termsize.width > 0 && new_termsize.height > 0) {
         data->mark_override_from_env(new_termsize);
     } else {
-        data->last_winch_gen_count = s_sigwinch_gen_count;
+        data->last_tty_gen_count = s_tty_termsize_gen_count;
         data->last_from_tty = this->tty_size_reader_();
     }
     return data->current();
@@ -124,4 +123,7 @@ void termsize_container_t::handle_columns_lines_var_change(const environment_t &
 }
 
 // static
-void termsize_container_t::handle_winch() { s_sigwinch_gen_count += 1; }
+void termsize_container_t::handle_winch() { s_tty_termsize_gen_count += 1; }
+
+// static
+void termsize_container_t::invalidate_tty() { s_tty_termsize_gen_count += 1; }

src/termsize.h

@@ -62,6 +62,9 @@ struct termsize_container_t {
     /// Naturally this may be called from within a signal handler.
     static void handle_winch();
 
+    /// Invalidate the tty in the sense that we need to re-fetch its termsize.
+    static void invalidate_tty();
+
     /// Note that COLUMNS and/or LINES global variables changed.
     void handle_columns_lines_var_change(const environment_t &vars);
 
@@ -79,9 +82,9 @@ struct termsize_container_t {
         // The last termsize seen from the environment (COLUMNS/LINES), or none if none.
         maybe_t<termsize_t> last_from_env{};
 
-        // The last-seen winch generation count.
+        // The last-seen tty-invalidation generation count.
         // Set to a huge value so it's initially stale.
-        uint32_t last_winch_gen_count{UINT32_MAX};
+        uint32_t last_tty_gen_count{UINT32_MAX};
 
         /// \return the current termsize from this data.
         termsize_t current() const;