Introduce proc_status_t

In fish we play fast and loose with status codes as set directly (e.g. on
failed redirections), vs status codes returned from waitpid(), versus the
value $status. Introduce a new value type proc_status_t to encapsulate
this logic.

src/builtin.cpp

@@ -496,17 +496,17 @@ static const wchar_t *const help_builtins[] = {L"for", L"while",  L"function", L
 static bool cmd_needs_help(const wchar_t *cmd) { return contains(help_builtins, cmd); }
 
 /// Execute a builtin command
-int builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_streams_t &streams) {
+proc_status_t builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_streams_t &streams) {
     UNUSED(parser);
     UNUSED(streams);
-    if (argv == NULL || argv[0] == NULL) return STATUS_INVALID_ARGS;
+    if (argv == NULL || argv[0] == NULL) return proc_status_t::from_exit_code(STATUS_INVALID_ARGS);
 
     // We can be handed a keyword by the parser as if it was a command. This happens when the user
     // follows the keyword by `-h` or `--help`. Since it isn't really a builtin command we need to
     // handle displaying help for it here.
     if (argv[1] && !argv[2] && parse_util_argument_is_help(argv[1]) && cmd_needs_help(argv[0])) {
         builtin_print_help(parser, streams, argv[0], streams.out);
-        return STATUS_CMD_OK;
+        return proc_status_t::from_exit_code(STATUS_CMD_OK);
     }
 
     if (const builtin_data_t *data = builtin_lookup(argv[0])) {
@@ -518,11 +518,11 @@ int builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_streams_t &st
         if (pgroup_to_restore >= 0) {
             tcsetpgrp(STDIN_FILENO, pgroup_to_restore);
         }
-        return ret;
+        return proc_status_t::from_exit_code(ret);
     }
 
     debug(0, UNKNOWN_BUILTIN_ERR_MSG, argv[0]);
-    return STATUS_CMD_ERROR;
+    return proc_status_t::from_exit_code(STATUS_CMD_ERROR);
 }
 
 /// Returns a list of all builtin names.

src/builtin.h

@@ -10,6 +10,7 @@
 
 class completion_t;
 class parser_t;
+class proc_status_t;
 class output_stream_t;
 struct io_streams_t;
 
@@ -78,7 +79,7 @@ enum { COMMAND_NOT_BUILTIN, BUILTIN_REGULAR, BUILTIN_FUNCTION };
 void builtin_init();
 bool builtin_exists(const wcstring &cmd);
 
-int builtin_run(parser_t &parser, int job_pgrp, wchar_t **argv, io_streams_t &streams);
+proc_status_t builtin_run(parser_t &parser, int job_pgrp, wchar_t **argv, io_streams_t &streams);
 
 wcstring_list_t builtin_get_names();
 void builtin_get_names(std::vector<completion_t> *list);

src/exec.cpp

@@ -374,7 +374,7 @@ static bool run_internal_process(process_t *p, std::string outdata, std::string
         maybe_t<dup2_list_t> dup2s{};
         std::shared_ptr<internal_proc_t> internal_proc{};
 
-        int success_status{};
+        proc_status_t success_status{};
 
         bool skip_out() const { return outdata.empty() || src_outfd < 0; }
 
@@ -403,10 +403,10 @@ static bool run_internal_process(process_t *p, std::string outdata, std::string
     f->src_outfd = f->dup2s->fd_for_target_fd(STDOUT_FILENO);
     f->src_errfd = f->dup2s->fd_for_target_fd(STDERR_FILENO);
 
-    // If we have nothing to right we can elide the thread.
+    // If we have nothing to write we can elide the thread.
     // TODO: support eliding output to /dev/null.
     if (f->skip_out() && f->skip_err()) {
-        f->internal_proc->mark_exited(EXIT_SUCCESS);
+        f->internal_proc->mark_exited(proc_status_t::from_exit_code(EXIT_SUCCESS));
         return true;
     }
 
@@ -419,14 +419,16 @@ static bool run_internal_process(process_t *p, std::string outdata, std::string
     f->success_status = p->status;
 
     iothread_perform([f]() {
-        int status = f->success_status;
+        proc_status_t status = f->success_status;
         if (!f->skip_out()) {
             ssize_t ret = write_loop(f->src_outfd, f->outdata.data(), f->outdata.size());
             if (ret < 0) {
                 if (errno != EPIPE) {
                     wperror(L"write");
                 }
-                if (!status) status = 1;
+                if (status.is_success()) {
+                    status = proc_status_t::from_exit_code(1);
+                }
             }
         }
         if (!f->skip_err()) {
@@ -435,7 +437,9 @@ static bool run_internal_process(process_t *p, std::string outdata, std::string
                 if (errno != EPIPE) {
                     wperror(L"write");
                 }
-                if (!status) status = 1;
+                if (status.is_success()) {
+                    status = proc_status_t::from_exit_code(1);
+                }
             }
         }
         f->internal_proc->mark_exited(status);
@@ -592,7 +596,9 @@ static bool handle_builtin_output(const std::shared_ptr<job_t> &j, process_t *p,
     const output_stream_t &stderr_stream = builtin_io_streams.err;
 
     // Mark if we discarded output.
-    if (stdout_stream.buffer().discarded()) p->status = STATUS_READ_TOO_MUCH;
+    if (stdout_stream.buffer().discarded()) {
+        p->status = proc_status_t::from_exit_code(STATUS_READ_TOO_MUCH);
+    }
 
     // We will try to elide constructing an internal process. However if the output is going to a
     // real file, we have to do it. For example in `echo -n > file.txt` we proceed to open file.txt
@@ -657,8 +663,8 @@ static bool handle_builtin_output(const std::shared_ptr<job_t> &j, process_t *p,
             debug(4, L"Set status of job %d (%ls) to %d using short circuit", j->job_id,
                   j->preview().c_str(), p->status);
 
-            int status = p->status;
-            proc_set_last_status(j->get_flag(job_flag_t::NEGATE) ? (!status) : status);
+            int status_value = p->status.status_value();
+            proc_set_last_status(j->get_flag(job_flag_t::NEGATE) ? (!status_value) : status_value);
         }
         return true;
     } else {
@@ -824,7 +830,9 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t>
     }
 
     int status = proc_get_last_status();
-    p->status = status;
+    // FIXME: setting the status this way is dangerous nonsense, we need to decode the status
+    // properly if it was a signal.
+    p->status = proc_status_t::from_exit_code(status);
 
     // If we have a block output buffer, populate it now.
     if (!block_output_bufferfill) {

src/proc.cpp

@@ -147,7 +147,7 @@ void proc_set_last_job_statuses(const job_t &last_job) {
     std::vector<int> ljs;
     ljs.reserve(last_job.processes.size());
     for (const auto &p : last_job.processes) {
-        ljs.push_back(p->pid ? proc_format_status(p->status) : p->status);
+        ljs.push_back(p->status.status_value());
     }
     proc_set_last_job_statuses(std::move(ljs));
 }
@@ -265,10 +265,10 @@ bool job_t::signal(int signal) {
     return true;
 }
 
-void internal_proc_t::mark_exited(int status) {
+void internal_proc_t::mark_exited(proc_status_t status) {
     assert(!exited() && "Process is already exited");
-    exited_.store(true, std::memory_order_relaxed);
-    status_.store(status, std::memory_order_release);
+    status_.store(status, std::memory_order_relaxed);
+    exited_.store(true, std::memory_order_release);
     topic_monitor_t::principal().post(topic_t::internal_exit);
 }
 
@@ -279,14 +279,14 @@ static void mark_job_complete(const job_t *j) {
 }
 
 /// Store the status of the process pid that was returned by waitpid.
-static void mark_process_status(process_t *p, int status) {
+static void mark_process_status(process_t *p, proc_status_t status) {
     // debug( 0, L"Process %ls %ls", p->argv[0], WIFSTOPPED (status)?L"stopped":(WIFEXITED( status
     // )?L"exited":(WIFSIGNALED( status )?L"signaled to exit":L"BLARGH")) );
     p->status = status;
 
-    if (WIFSTOPPED(status)) {
+    if (status.stopped()) {
         p->stopped = 1;
-    } else if (WIFSIGNALED(status) || WIFEXITED(status)) {
+    } else if (status.signal_exited() || status.normal_exited()) {
         p->completed = 1;
     } else {
         // This should never be reached.
@@ -311,7 +311,7 @@ void job_mark_process_as_failed(const std::shared_ptr<job_t> &job, const process
 ///
 /// \param pid the pid of the process whose status changes
 /// \param status the status as returned by wait
-static void handle_child_status(pid_t pid, int status) {
+static void handle_child_status(pid_t pid, proc_status_t status) {
     job_t *j = NULL;
     const process_t *found_proc = NULL;
 
@@ -329,7 +329,8 @@ static void handle_child_status(pid_t pid, int status) {
     }
 
     // If the child process was not killed by a signal or other than SIGINT or SIGQUIT we're done.
-    if (!WIFSIGNALED(status) || (WTERMSIG(status) != SIGINT && WTERMSIG(status) != SIGQUIT)) {
+    if (!status.signal_exited() ||
+        (status.signal_code() != SIGINT && status.signal_code() != SIGQUIT)) {
         return;
     }
 
@@ -345,7 +346,7 @@ static void handle_child_status(pid_t pid, int status) {
         act.sa_handler = SIG_DFL;
         sigaction(SIGINT, &act, 0);
         sigaction(SIGQUIT, &act, 0);
-        kill(getpid(), WTERMSIG(status));
+        kill(getpid(), status.signal_code());
     }
 }
 
@@ -452,7 +453,7 @@ static void process_mark_finished_children(bool block_ok) {
                         if (pid > 0) {
                             assert(pid == proc->pid && "Unexpcted waitpid() return");
                             debug(4, "Reaped PID %d", pid);
-                            handle_child_status(pid, status);
+                            handle_child_status(pid, proc_status_t::from_waitpid(status));
                         }
                     } else {
                         assert(0 && "Don't know how to reap this process");
@@ -542,12 +543,11 @@ static bool process_clean_after_marking(bool allow_interactive) {
         }
 
         for (const process_ptr_t &p : j->processes) {
-            int s;
             if (!p->completed) continue;
 
             if (!p->pid) continue;
 
-            s = p->status;
+            auto s = p->status;
 
             // TODO: The generic process-exit event is useless and unused.
             // Remove this in future.
@@ -558,7 +558,7 @@ static bool process_clean_after_marking(bool allow_interactive) {
 
             // Ignore signal SIGPIPE.We issue it ourselves to the pipe writer when the pipe reader
             // dies.
-            if (!WIFSIGNALED(s) || WTERMSIG(s) == SIGPIPE) {
+            if (!s.signal_exited() || s.signal_code() == SIGPIPE) {
                 continue;
             }
 
@@ -604,7 +604,8 @@ static bool process_clean_after_marking(bool allow_interactive) {
                 fwprintf(stdout, L"\n");
             }
             found = false;
-            p->status = 0;  // clear status so it is not reported more than once
+            p->status = proc_status_t::from_exit_code(
+                0);  // clear status so it is not reported more than once
         }
 
         // If all processes have completed, tell the user the job has completed and delete it from
@@ -932,28 +933,13 @@ void job_t::continue_job(bool send_sigcont) {
         // finished and is not a short-circuited builtin.
         bool negate = get_flag(job_flag_t::NEGATE);
         auto &p = processes.back();
-        if (p->internal_proc_) {
-            // Here the status is synthetic - not associated with a real exited process.
-            // TODO: clean this up, we shouldn't store the process's exit status in an unparsed
-            // state.
-            int status = p->status;
-            proc_set_last_status(negate ? !status : status);
-        } else if ((WIFEXITED(p->status) || WIFSIGNALED(p->status)) && p->pid) {
-            int status = proc_format_status(p->status);
-            proc_set_last_status(negate ? !status : status);
+        if (p->status.normal_exited() || p->status.signal_exited()) {
+            int status_code = p->status.status_value();
+            proc_set_last_status(negate ? !status_code : status_code);
         }
     }
 }
 
-int proc_format_status(int status) {
-    if (WIFSIGNALED(status)) {
-        return 128 + WTERMSIG(status);
-    } else if (WIFEXITED(status)) {
-        return WEXITSTATUS(status);
-    }
-    return status;
-}
-
 void proc_sanity_check() {
     const job_t *fg_job = NULL;
 
@@ -1012,7 +998,7 @@ pid_t proc_wait_any() {
     int pid_status;
     pid_t pid = waitpid(-1, &pid_status, WUNTRACED);
     if (pid == -1) return -1;
-    handle_child_status(pid, pid_status);
+    handle_child_status(pid, proc_status_t::from_waitpid(pid_status));
     process_clean_after_marking(is_interactive);
     return pid;
 }

src/proc.h

@@ -43,6 +43,72 @@ enum {
     JOB_CONTROL_NONE,
 };
 
+/// A proc_status_t is a value type that encapsulates logic around exited vs stopped vs signaled,
+/// etc.
+class proc_status_t {
+    int status_{};
+
+    explicit proc_status_t(int status) : status_(status) {}
+
+    /// Encode a return value \p ret and signal \p sig into a status value like waitpid() does.
+    static constexpr int w_exitcode(int ret, int sig) {
+#ifdef W_EXITCODE
+        return W_EXITCODE(ret, sig);
+#else
+        return ((ret) << 8 | (sig));
+#endif
+    }
+
+   public:
+    proc_status_t() = default;
+
+    /// Construct from a status returned from a waitpid call.
+    static proc_status_t from_waitpid(int status) { return proc_status_t(status); }
+
+    /// Construct directly from an exit code.
+    static proc_status_t from_exit_code(int ret) {
+        // Some paranoia.
+        constexpr int zerocode = w_exitcode(0, 0);
+        static_assert(WIFEXITED(zerocode), "Synthetic exit status not reported as exited");
+        return proc_status_t(w_exitcode(ret, 0 /* sig */));
+    }
+
+    /// \return if we are stopped (as in SIGSTOP).
+    bool stopped() const { return WIFSTOPPED(status_); }
+
+    /// \return if we exited normally (not a signal).
+    bool normal_exited() const { return WIFEXITED(status_); }
+
+    /// \return if we exited because of a signal.
+    bool signal_exited() const { return WIFSIGNALED(status_); }
+
+    /// \return the signal code, given that we signal exited.
+    int signal_code() const {
+        assert(signal_exited() && "Process is not signal exited");
+        return WTERMSIG(status_);
+    }
+
+    /// \return the exit code, given that we normal exited.
+    int exit_code() const {
+        assert(normal_exited() && "Process is not normal exited");
+        return WEXITSTATUS(status_);
+    }
+
+    /// \return if this status represents success.
+    bool is_success() const { return normal_exited() && exit_code() == EXIT_SUCCESS; }
+
+    /// \return the value appropriate to populate $status.
+    int status_value() const {
+        if (signal_exited()) {
+            return 128 + signal_code();
+        } else if (normal_exited()) {
+            return exit_code();
+        } else {
+            DIE("Process is not exited");
+        }
+    }
+};
+
 /// A structure representing a "process" internal to fish. This is backed by a pthread instead of a
 /// separate process.
 class internal_proc_t {
@@ -50,18 +116,18 @@ class internal_proc_t {
     std::atomic<bool> exited_{};
 
     /// If the process has exited, its status code.
-    std::atomic<int> status_{};
+    std::atomic<proc_status_t> status_{};
 
    public:
     /// \return if this process has exited.
-    bool exited() const { return exited_.load(std::memory_order_relaxed); }
+    bool exited() const { return exited_.load(std::memory_order_acquire); }
 
     /// Mark this process as exited, with the given status.
-    void mark_exited(int status);
+    void mark_exited(proc_status_t status);
 
-    int get_status() const {
+    proc_status_t get_status() const {
         assert(exited() && "Process is not exited");
-        return status_.load(std::memory_order_acquire);
+        return status_.load(std::memory_order_relaxed);
     }
 };
 
@@ -160,7 +226,7 @@ class process_t {
     /// True if process has stopped.
     volatile bool stopped{false};
     /// Reported status value.
-    volatile int status{0};
+    proc_status_t status{};
 #ifdef HAVE__PROC_SELF_STAT
     /// Last time of cpu time check.
     struct timeval last_time {};
@@ -461,10 +527,6 @@ void proc_push_interactive(int value);
 /// Set is_interactive flag to the previous value. If needed, update signal handlers.
 void proc_pop_interactive();
 
-/// Format an exit status code as returned by e.g. wait into a fish exit code number as accepted by
-/// proc_set_last_status.
-int proc_format_status(int status);
-
 /// Wait for any process finishing.
 pid_t proc_wait_any();