builtins: Don't crash for negative return values

Another from the "why are we asserting instead of doing something
sensible" department.

The alternative is to make exit() and return() compute their own exit
code, but tbh I don't want any *other* builtin to hit this either?

Fixes #9659

(cherry picked from commit a16abf22d9)

src/builtin.cpp

@@ -475,6 +475,12 @@ proc_status_t builtin_run(parser_t &parser, const wcstring_list_t &argv, io_stre
             return proc_status_t::empty();
         }
         if (code < 0) {
+            // If the code is below 0, constructing a proc_status_t
+            // would assert() out, which is a terrible failure mode
+            // So instead, what we do is we get a positive code,
+            // and we avoid 0.
+            code = abs((256 + code) % 256);
+            if (code == 0) code = 255;
             FLOGF(warning, "builtin %ls returned invalid exit code %d", cmdname.c_str(), code);
         }
         return proc_status_t::from_exit_code(code);

tests/checks/status-value.fish

@@ -1,4 +1,4 @@
-# RUN: %fish %s
+# RUN: %fish -C 'set -g fish %fish' %s
 
 # Empty commands should be 123
 set empty_var
@@ -24,3 +24,24 @@ echo $status
 # CHECKERR: {{.*}} No matches for wildcard '*gibberishgibberishgibberish*'. {{.*}}
 # CHECKERR: echo *gibberishgibberishgibberish*
 # CHECKERR:      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~^
+
+$fish -c 'exit -5'
+# CHECKERR: warning: builtin exit returned invalid exit code 251
+echo $status
+# CHECK: 251
+
+$fish -c 'exit -1'
+# CHECKERR: warning: builtin exit returned invalid exit code 255
+echo $status
+# CHECK: 255
+
+# (we avoid 0, so this is turned into 255 again)
+$fish -c 'exit -256'
+# CHECKERR: warning: builtin exit returned invalid exit code 255
+echo $status
+# CHECK: 255
+
+$fish -c 'exit -512'
+# CHECKERR: warning: builtin exit returned invalid exit code 255
+echo $status
+# CHECK: 255