From 79255dfe9b6b54748cee41af307efb91870f8eb6 Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Sat, 28 May 2022 13:35:17 -0700
Subject: [PATCH] Make s_observed_signals accurate

s_observed_signals is used to inform the signal handler which signals may
have --on-signal functions attached to them, as an optimization. Prior to
this change it was latched: once we started observing a signal we assume we
will keep observing that signal. Make it properly increment and decrement,
in preparation for making trap work non-interactively.
---
 src/event.cpp       | 44 +++++++++++++++++++++++++++-----------------
 src/global_safety.h |  4 +++-
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/src/event.cpp b/src/event.cpp
index fafd5492e..8682a0066 100644
--- a/src/event.cpp
+++ b/src/event.cpp
@@ -8,6 +8,7 @@
 #include <unistd.h>
 
 #include <algorithm>
+#include <array>
 #include <atomic>
 #include <functional>
 #include <memory>
@@ -87,16 +88,31 @@ static pending_signals_t s_pending_signals;
 /// List of event handlers.
 static owning_lock<event_handler_list_t> s_event_handlers;
 
-/// Variables (one per signal) set when a signal is observed. This is inspected by a signal handler.
-static volatile sig_atomic_t s_observed_signals[NSIG] = {};
+/// Tracks the number of registered event handlers for each signal.
+/// This is inspected by a signal handler. We assume no values in here overflow.
+static std::array<relaxed_atomic_t<uint32_t>, NSIG> s_observed_signals;
 
-static void set_signal_observed(int sig, bool val) {
-    if (sig >= 0 &&
-        static_cast<size_t>(sig) < sizeof s_observed_signals / sizeof *s_observed_signals) {
-        s_observed_signals[sig] = val;
+static inline void inc_signal_observed(int sig) {
+    if (0 <= sig && sig < NSIG) {
+        s_observed_signals[sig]++;
     }
 }
 
+static inline void dec_signal_observed(int sig) {
+    if (0 <= sig && sig < NSIG) {
+        s_observed_signals[sig]--;
+    }
+}
+
+bool event_is_signal_observed(int sig) {
+    // We are in a signal handler!
+    uint32_t count = 0;
+    if (0 <= sig && sig < NSIG) {
+        count = s_observed_signals[sig];
+    }
+    return count > 0;
+}
+
 /// \return true if a handler is "one shot": it fires at most once.
 static bool handler_is_one_shot(const event_handler_t &handler) {
     switch (handler.desc.type) {
@@ -206,13 +222,14 @@ wcstring event_get_desc(const parser_t &parser, const event_t &evt) {
 void event_add_handler(std::shared_ptr<event_handler_t> eh) {
     if (eh->desc.type == event_type_t::signal) {
         signal_handle(eh->desc.param1.signal);
-        set_signal_observed(eh->desc.param1.signal, true);
+        inc_signal_observed(eh->desc.param1.signal);
     }
 
     s_event_handlers.acquire()->push_back(std::move(eh));
 }
 
 // \remove handlers for which \p func returns true.
+// Simultaneously update our signal_observed array.
 template <typename T>
 static void remove_handlers_if(const T &func) {
     auto handlers = s_event_handlers.acquire();
@@ -221,6 +238,9 @@ static void remove_handlers_if(const T &func) {
         event_handler_t *handler = iter->get();
         if (func(*handler)) {
             handler->removed = true;
+            if (handler->desc.type == event_type_t::signal) {
+                dec_signal_observed(handler->desc.param1.signal);
+            }
             iter = handlers->erase(iter);
         } else {
             ++iter;
@@ -244,16 +264,6 @@ event_handler_list_t event_get_function_handlers(const wcstring &name) {
     return result;
 }
 
-bool event_is_signal_observed(int sig) {
-    // We are in a signal handler! Don't allocate memory, etc.
-    bool result = false;
-    if (sig >= 0 && static_cast<unsigned long>(sig) <
-                        sizeof(s_observed_signals) / sizeof(*s_observed_signals)) {
-        result = s_observed_signals[sig];
-    }
-    return result;
-}
-
 /// Perform the specified event. Since almost all event firings will not be matched by even a single
 /// event handler, we make sure to optimize the 'no matches' path. This means that nothing is
 /// allocated/initialized unless needed.
diff --git a/src/global_safety.h b/src/global_safety.h
index cc19f2f59..7230098c1 100644
--- a/src/global_safety.h
+++ b/src/global_safety.h
@@ -93,9 +93,11 @@ class relaxed_atomic_t {
 
     // postincrement
     T operator++(int) { return value_.fetch_add(1, std::memory_order_relaxed); }
+    T operator--(int) { return value_.fetch_sub(1, std::memory_order_relaxed); }
 
     // preincrement
-    T operator++() { return 1 + value_.fetch_add(1, std::memory_order_relaxed); }
+    T operator++() { return value_.fetch_add(1, std::memory_order_relaxed) + 1; }
+    T operator--() { return value_.fetch_sub(1, std::memory_order_relaxed) - 1; }
 };
 
 using relaxed_atomic_bool_t = relaxed_atomic_t<bool>;