Rework how screen size is tracked

The screen size is fetched after a SIGWINCH is delivered. The current
implementation has two issues:

* It calls ioctl() from the SIGWINCH signal handler, despite ioctl() not
  being a function that is known to be safe to call.
* It's not thread-safe.

Signals can be delivered on arbitrary threads, so we don't know if it's
actually safe to be modifying the cached winsize in response to a
signal. It's also plausible that the winsize may be requested from a
background thread.

To solve the first issue, we twiddle a volatile boolean flag in the
signal handler and defer the ioctl() call until we actually request the
screen size.

To solve the second issue, we introduce a pthread rwlock around the
cached winsize. A rwlock is used because it can be expected that there
are likely to be far more window size reads than window size writes. If
we were using C++11 we could probably get away with atomics, but since
we don't have that (or boost), a rwlock should suffice.

Fixes #1613.

common.cpp

@@ -90,9 +90,12 @@ const wchar_t *program_name;
 int debug_level=1;
 
 /**
-   This struct should be continually updated by signals as the term resizes, and as such always contain the correct current size.
+   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 struct winsize termsize;
+static volatile bool termsize_valid;
+static rwlock_t termsize_rwlock;
 
 static char *wcs2str_internal(const wchar_t *in, char *out);
 
@@ -1686,26 +1689,44 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e
 
 void common_handle_winch(int signal)
 {
-#ifdef HAVE_WINSIZE
+    /* don't run ioctl() here, it's not safe to use in signals */
-    if (ioctl(1,TIOCGWINSZ,&termsize)!=0)
+    termsize_valid = false;
-    {
+}
-        return;
+
-    }
+/* updates termsize as needed, and returns a copy of the winsize. */
-#else
+static struct winsize get_current_winsize()
-    termsize.ws_col = 80;
+{
-    termsize.ws_row = 24;
+#ifndef HAVE_WINSIZE
+    struct winsize retval = {0};
+    retval.ws_col = 80;
+    retval.ws_row = 24;
+    return retval;
 #endif
+    scoped_rwlock guard(termsize_rwlock, true);
+    struct winsize retval = termsize;
+    if (!termsize_valid)
+    {
+        struct winsize size;
+        if (ioctl(1,TIOCGWINSZ,&size) == 0)
+        {
+            retval = size;
+            guard.upgrade();
+            termsize = retval;
+        }
+        termsize_valid = true;
+    }
+    return retval;
 }
 
 int common_get_width()
 {
-    return termsize.ws_col;
+    return get_current_winsize().ws_col;
 }
 
 
 int common_get_height()
 {
-    return termsize.ws_row;
+    return get_current_winsize().ws_row;
 }
 
 void tokenize_variable_array(const wcstring &val, std::vector<wcstring> &out)
@@ -2223,7 +2244,7 @@ void scoped_lock::lock(void)
 {
     assert(! locked);
     assert(! is_forked_child());
-    VOMIT_ON_FAILURE(pthread_mutex_lock(lock_obj));
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_lock(lock_obj));
     locked = true;
 }
 
@@ -2231,7 +2252,7 @@ void scoped_lock::unlock(void)
 {
     assert(locked);
     assert(! is_forked_child());
-    VOMIT_ON_FAILURE(pthread_mutex_unlock(lock_obj));
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_unlock(lock_obj));
     locked = false;
 }
 
@@ -2250,6 +2271,84 @@ scoped_lock::~scoped_lock()
     if (locked) this->unlock();
 }
 
+void scoped_rwlock::lock(void)
+{
+    assert(! (locked || locked_shared));
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_rdlock(rwlock_obj));
+    locked = true;
+}
+
+void scoped_rwlock::unlock(void)
+{
+    assert(locked);
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj));
+    locked = false;
+}
+
+void scoped_rwlock::lock_shared(void)
+{
+    assert(! (locked || locked_shared));
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj));
+    locked_shared = true;
+}
+
+void scoped_rwlock::unlock_shared(void)
+{
+    assert(locked_shared);
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj));
+    locked_shared = false;
+}
+
+void scoped_rwlock::upgrade(void)
+{
+    assert(locked_shared);
+    assert(! is_forked_child());
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_unlock(rwlock_obj));
+    locked = false;
+    VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_wrlock(rwlock_obj));
+    locked_shared = true;
+}
+
+scoped_rwlock::scoped_rwlock(pthread_rwlock_t &rwlock, bool shared) : rwlock_obj(&rwlock), locked(false), locked_shared(false)
+{
+    if (shared)
+    {
+        this->lock_shared();
+    }
+    else
+    {
+        this->lock();
+    }
+}
+
+scoped_rwlock::scoped_rwlock(rwlock_t &rwlock, bool shared) : rwlock_obj(&rwlock.rwlock), locked(false), locked_shared(false)
+{
+    if (shared)
+    {
+        this->lock_shared();
+    }
+    else
+    {
+        this->lock();
+    }
+}
+
+scoped_rwlock::~scoped_rwlock()
+{
+    if (locked)
+    {
+        this->unlock();
+    }
+    else if (locked_shared)
+    {
+        this->unlock_shared();
+    }
+}
+
 wcstokenizer::wcstokenizer(const wcstring &s, const wcstring &separator) :
     buffer(),
     str(),

common.h

@@ -121,7 +121,9 @@ inline bool selection_direction_is_cardinal(selection_direction_t dir)
 /**
  Helper macro for errors
  */
-#define VOMIT_ON_FAILURE(a) do { if (0 != (a)) { int err = errno; fprintf(stderr, "%s failed on line %d in file %s: %d (%s)\n", #a, __LINE__, __FILE__, err, strerror(err)); abort(); }} while (0)
+#define VOMIT_ON_FAILURE(a) do { if (0 != (a)) { VOMIT_ABORT(errno, #a); } } while (0)
+#define VOMIT_ON_FAILURE_NO_ERRNO(a) do { int err = (a); if (0 != err) { VOMIT_ABORT(err, #a); } } while (0)
+#define VOMIT_ABORT(err, str) do { int code = (err); fprintf(stderr, "%s failed on line %d in file %s: %d (%s)\n", str, __LINE__, __FILE__, code, strerror(code)); abort(); } while(0)
 
 /** Exits without invoking destructors (via _exit), useful for code after fork. */
 void exit_without_destructors(int code) __attribute__((noreturn));
@@ -544,12 +546,12 @@ class mutex_lock_t
     pthread_mutex_t mutex;
     mutex_lock_t()
     {
-        pthread_mutex_init(&mutex, NULL);
+        VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_init(&mutex, NULL));
     }
     
     ~mutex_lock_t()
     {
-        pthread_mutex_destroy(&mutex);
+        VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_destroy(&mutex));
     }
 };
 
@@ -571,6 +573,48 @@ public:
     ~scoped_lock();
 };
 
+class rwlock_t
+{
+public:
+    pthread_rwlock_t rwlock;
+    rwlock_t()
+    {
+        VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_init(&rwlock, NULL));
+    }
+
+    ~rwlock_t()
+    {
+        VOMIT_ON_FAILURE_NO_ERRNO(pthread_rwlock_destroy(&rwlock));
+    }
+};
+
+/*
+   Scoped lock class for rwlocks
+ */
+class scoped_rwlock
+{
+    pthread_rwlock_t *rwlock_obj;
+    bool locked;
+    bool locked_shared;
+
+    /* No copying */
+    scoped_rwlock &operator=(const scoped_lock &);
+    scoped_rwlock(const scoped_lock &);
+
+public:
+    void lock(void);
+    void unlock(void);
+    void lock_shared(void);
+    void unlock_shared(void);
+    /*
+       upgrade shared lock to exclusive.
+       equivalent to `lock.unlock_shared(); lock.lock();`
+     */
+    void upgrade(void);
+    scoped_rwlock(pthread_rwlock_t &rwlock, bool shared = false);
+    scoped_rwlock(rwlock_t &rwlock, bool shared = false);
+    ~scoped_rwlock();
+};
 
 /**
    A scoped manager to save the current value of some variable, and optionally

function.cpp

@@ -67,9 +67,6 @@ void function_autoload_t::command_removed(const wcstring &cmd)
     function_remove_ignore_autoload(cmd);
 }
 
-/* Helper macro for vomiting */
-#define VOMIT_ON_FAILURE(a) do { if (0 != (a)) { int err = errno; fprintf(stderr, "%s failed on line %d in file %s: %d (%s)\n", #a, __LINE__, __FILE__, err, strerror(err)); abort(); }} while (0)
-
 /**
    Kludgy flag set by the load function in order to tell function_add
    that the function being defined is autoloaded. There should be a