Early steps towards rationalizing SIGINT handling

Previously we would try to walk all the blocks (from within the
signal handler!) and mark them as skipped. Stop doing that, it's
wildly unsafe.

Also rationalize how the skip flag is set per block. Remove places
that shouldn't set it (e.g. break and continue shouldn't set skip
on the loop block).

src/builtin.cpp

@@ -3033,15 +3033,14 @@ static int builtin_break_continue(parser_t &parser, io_streams_t &streams, wchar
         return STATUS_BUILTIN_ERROR;
     }
 
-    // Skip blocks interior to the loop.
+    // Skip blocks interior to the loop (but not the loop itself)
     size_t block_idx = loop_idx;
     while (block_idx--) {
         parser.block_at_index(block_idx)->skip = true;
     }
 
-    /* Skip the loop itself */
+    // Mark the loop's status
     block_t *loop_block = parser.block_at_index(loop_idx);
-    loop_block->skip = true;
     loop_block->loop_status = is_break ? LOOP_BREAK : LOOP_CONTINUE;
     return STATUS_BUILTIN_OK;
 }
@@ -3098,12 +3097,11 @@ static int builtin_return(parser_t &parser, io_streams_t &streams, wchar_t **arg
         return STATUS_BUILTIN_ERROR;
     }
 
-    // Skip everything up to (and then including) the function block.
-    for (size_t i = 0; i < function_block_idx; i++) {
+    // Skip everything up to and including the function block.
+    for (size_t i = 0; i <= function_block_idx; i++) {
         block_t *b = parser.block_at_index(i);
         b->skip = true;
     }
-    parser.block_at_index(function_block_idx)->skip = true;
     return status;
 }
 

src/fish_tests.cpp

@@ -769,6 +769,7 @@ static void test_cancellation() {
     // Nasty infinite loop that doesn't actually execute anything.
     test_1_cancellation(L"echo (while true ; end) (while true ; end) (while true ; end)");
     test_1_cancellation(L"while true ; end");
+    test_1_cancellation(L"while true ; echo nothing > /dev/null; end");
     test_1_cancellation(L"for i in (while true ; end) ; end");
 
     // Restore signal handling.

src/parse_execution.cpp

@@ -220,13 +220,12 @@ parse_execution_context_t::cancellation_reason(const block_t *block) const {
     if (parser && parser->cancellation_requested) {
         return execution_cancellation_skip;
     }
-    if (block && block->loop_status != LOOP_NORMAL) {
-        // Nasty hack - break and continue set the 'skip' flag as well as the loop status flag.
-        return execution_cancellation_loop_control;
-    }
     if (block && block->skip) {
         return execution_cancellation_skip;
     }
+    if (block && block->loop_status != LOOP_NORMAL) {
+        return execution_cancellation_loop_control;
+    }
     return execution_cancellation_none;
 }
 
@@ -485,7 +484,6 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(
         const wcstring &val = argument_sequence.at(i);
         env_set(for_var_name, val.c_str(), ENV_LOCAL);
         fb->loop_status = LOOP_NORMAL;
-        fb->skip = 0;
 
         this->run_job_list(block_contents, fb);
 
@@ -494,7 +492,6 @@ parse_execution_result_t parse_execution_context_t::run_for_statement(
             if (fb->loop_status == LOOP_CONTINUE) {
                 // Reset the loop state.
                 fb->loop_status = LOOP_NORMAL;
-                fb->skip = false;
                 continue;
             } else if (fb->loop_status == LOOP_BREAK) {
                 break;
@@ -656,7 +653,6 @@ parse_execution_result_t parse_execution_context_t::run_while_statement(
             if (wb->loop_status == LOOP_CONTINUE) {
                 // Reset the loop state.
                 wb->loop_status = LOOP_NORMAL;
-                wb->skip = false;
                 continue;
             } else if (wb->loop_status == LOOP_BREAK) {
                 break;

src/parser.cpp

@@ -101,17 +101,13 @@ static wcstring user_presentable_path(const wcstring &path) {
 
 parser_t::parser_t() : cancellation_requested(false), is_within_fish_initialization(false) {}
 
-/// A pointer to the principal parser (which is a static local).
-static parser_t *s_principal_parser = NULL;
+
+static parser_t s_principal_parser;
 
 parser_t &parser_t::principal_parser(void) {
     ASSERT_IS_NOT_FORKED_CHILD();
     ASSERT_IS_MAIN_THREAD();
-    static parser_t parser;
-    if (!s_principal_parser) {
-        s_principal_parser = &parser;
-    }
-    return parser;
+    return s_principal_parser;
 }
 
 void parser_t::set_is_within_fish_initialization(bool flag) {
@@ -120,14 +116,8 @@ void parser_t::set_is_within_fish_initialization(bool flag) {
 
 void parser_t::skip_all_blocks(void) {
     // Tell all blocks to skip.
-    if (s_principal_parser) {
-        s_principal_parser->cancellation_requested = true;
-
-        // write(2, "Cancelling blocks\n", strlen("Cancelling blocks\n"));
-        for (size_t i = 0; i < s_principal_parser->block_count(); i++) {
-            s_principal_parser->block_at_index(i)->skip = true;
-        }
-    }
+    // This may be called from a signal handler!
+    s_principal_parser.cancellation_requested = true;
 }
 
 void parser_t::push_block(block_t *new_current) {
@@ -139,22 +129,17 @@ void parser_t::push_block(block_t *new_current) {
         new_current->src_filename = intern(filename);
     }
 
+    // New blocks should be skipped if the outer block is skipped, except TOP and SUBST block, which
+    // open up new environments.
     const block_t *old_current = this->current_block();
-    if (old_current && old_current->skip) {
-        new_current->skip = true;
-    }
-
-    // New blocks should be skipped if the outer block is skipped, except TOP ans SUBST block, which
-    // open up new environments. Fake blocks should always be skipped. Rather complicated... :-(
-    new_current->skip = old_current ? old_current->skip : 0;
+    new_current->skip = old_current && old_current->skip;
 
     // Type TOP and SUBST are never skipped.
     if (type == TOP || type == SUBST) {
-        new_current->skip = 0;
+        new_current->skip = false;
     }
 
-
-    new_current->job = 0;
+    new_current->job = nullptr;
     new_current->loop_status = LOOP_NORMAL;
 
     this->block_stack.push_back(new_current);
@@ -782,7 +767,7 @@ void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &erro
 
 block_t::block_t(block_type_t t)
     : block_type(t),
-      skip(),
+      skip(false),
       tok_pos(),
       node_offset(NODE_OFFSET_INVALID),
       loop_status(LOOP_NORMAL),

src/parser.h

@@ -178,7 +178,7 @@ class parser_t {
 
    private:
     /// Indication that we should skip all blocks.
-    bool cancellation_requested;
+    volatile sig_atomic_t cancellation_requested;
     /// Indicates that we are within the process of initializing fish.
     bool is_within_fish_initialization;
     /// Stack of execution contexts. We own these pointers and must delete them.