cranelift-wasm: Fix reachability tracking for if .. else .. end

We weren't previously keeping track of quite the right information for whether
an `if .. else .. end`'s following block was reachable or not. It should be
reachable if the head is reachable and either the consequent or alternative end
reachable (and therefore fall through to the following block) or do an early
`br_if` to it.

This commit rejiggers `ControlStackFrame::If` to keep track of reachability at
the end of the consequent (we don't need to keep track of it at the end of the
alternative, since that is simply `state.reachable`) and adds Wasm tests for
every reachability situation we can encounter with `if .. else .. end`.

Fixes #1132

cranelift/wasm/src/code_translator.rs

@@ -200,62 +200,69 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             let i = state.control_stack.len() - 1;
             match state.control_stack[i] {
                 ControlStackFrame::If {
-                    else_data: ElseData::NoElse { branch_inst },
+                    ref else_data,
-                    ref mut reachable_from_top,
+                    head_is_reachable,
+                    ref mut consequent_ends_reachable,
+                    num_return_values,
                     blocktype,
                     destination,
                     ..
                 } => {
-                    // We take the control frame pushed by the if, use its ebb
+                    // We finished the consequent, so record its final
-                    // as the else body and push a new control frame with a new
+                    // reachability state.
-                    // ebb for the code after the if/then/else. At the end of the
+                    debug_assert!(consequent_ends_reachable.is_none());
-                    // then clause we jump to the destination.
+                    *consequent_ends_reachable = Some(state.reachable);
 
-                    // The `if` has an `else`, so there's no branch to the end from the top.
+                    if head_is_reachable {
-                    *reachable_from_top = false;
+                        // We have a branch from the head of the `if` to the `else`.
+                        state.reachable = true;
 
-                    let (params, _results) =
+                        // Ensure we have an ebb for the `else` block (it may have
-                        blocktype_params_results(module_translation_state, blocktype)?;
+                        // already been pre-allocated, see `ElseData` for details).
-                    let else_ebb = ebb_with_params(builder, params)?;
+                        let else_ebb = match *else_data {
-                    builder.ins().jump(destination, state.peekn(params.len()));
+                            ElseData::NoElse { branch_inst } => {
-                    state.popn(params.len());
+                                let (params, _results) =
+                                    blocktype_params_results(module_translation_state, blocktype)?;
+                                debug_assert_eq!(params.len(), num_return_values);
+                                let else_ebb = ebb_with_params(builder, params)?;
+                                builder.ins().jump(destination, state.peekn(params.len()));
+                                state.popn(params.len());
 
-                    // You might be expecting that we push the parameters for this
+                                builder.change_jump_destination(branch_inst, else_ebb);
-                    // `else` block here, something like this:
+                                builder.seal_block(else_ebb);
-                    //
+                                else_ebb
-                    //     state.pushn(&control_stack_frame.params);
+                            }
-                    //
+                            ElseData::WithElse { else_block } => {
-                    // We don't do that because they are already on the top of the stack
+                                builder
-                    // for us: we pushed the parameters twice when we saw the initial
+                                    .ins()
-                    // `if` so that we wouldn't have to save the parameters in the
+                                    .jump(destination, state.peekn(num_return_values));
-                    // `ControlStackFrame` as another `Vec` allocation.
+                                state.popn(num_return_values);
+                                else_block
+                            }
+                        };
 
-                    builder.change_jump_destination(branch_inst, else_ebb);
+                        // You might be expecting that we push the parameters for this
-                    builder.seal_block(else_ebb);
+                        // `else` block here, something like this:
-                    builder.switch_to_block(else_ebb);
+                        //
+                        //     state.pushn(&control_stack_frame.params);
+                        //
+                        // We don't do that because they are already on the top of the stack
+                        // for us: we pushed the parameters twice when we saw the initial
+                        // `if` so that we wouldn't have to save the parameters in the
+                        // `ControlStackFrame` as another `Vec` allocation.
 
-                    // NB: we don't bother updating the control frame's
+                        builder.switch_to_block(else_ebb);
-                    // `ElseData` because nothing else will read it.
+
-                }
+                        // We don't bother updating the control frame's `ElseData`
-                ControlStackFrame::If {
+                        // to `WithElse` because nothing else will read it.
-                    destination,
+                    }
-                    num_return_values,
-                    else_data: ElseData::WithElse { else_block, .. },
-                    reachable_from_top,
-                    ..
-                } => {
-                    debug_assert!(!reachable_from_top);
-                    builder
-                        .ins()
-                        .jump(destination, state.peekn(num_return_values));
-                    state.popn(num_return_values);
-                    builder.switch_to_block(else_block);
                 }
                 _ => unreachable!(),
             }
         }
         Operator::End => {
             let frame = state.control_stack.pop().unwrap();
+
             if !builder.is_unreachable() || !builder.is_pristine() {
                 let return_count = frame.num_return_values();
                 builder
@@ -1212,6 +1219,7 @@ fn translate_unreachable_operator(
     builder: &mut FunctionBuilder,
     state: &mut FuncTranslationState,
 ) -> WasmResult<()> {
+    debug_assert!(!state.reachable);
     match *op {
         Operator::If { ty } => {
             // Push a placeholder control stack entry. The if isn't reachable,
@@ -1233,25 +1241,33 @@ fn translate_unreachable_operator(
             let i = state.control_stack.len() - 1;
             match state.control_stack[i] {
                 ControlStackFrame::If {
-                    else_data: ElseData::NoElse { branch_inst },
+                    ref else_data,
-                    ref mut reachable_from_top,
+                    head_is_reachable,
+                    ref mut consequent_ends_reachable,
                     blocktype,
                     ..
                 } => {
-                    if *reachable_from_top {
+                    debug_assert!(consequent_ends_reachable.is_none());
-                        // We have a branch from the top of the if to the else.
+                    *consequent_ends_reachable = Some(state.reachable);
+
+                    if head_is_reachable {
+                        // We have a branch from the head of the `if` to the `else`.
                         state.reachable = true;
-                        // And because there's an else, there can no longer be a
-                        // branch from the top directly to the end.
-                        *reachable_from_top = false;
 
-                        let (params, _results) =
+                        let else_ebb = match *else_data {
-                            blocktype_params_results(module_translation_state, blocktype)?;
+                            ElseData::NoElse { branch_inst } => {
-                        let else_ebb = ebb_with_params(builder, params)?;
+                                let (params, _results) =
+                                    blocktype_params_results(module_translation_state, blocktype)?;
+                                let else_ebb = ebb_with_params(builder, params)?;
+
+                                // We change the target of the branch instruction.
+                                builder.change_jump_destination(branch_inst, else_ebb);
+                                builder.seal_block(else_ebb);
+                                else_ebb
+                            }
+                            ElseData::WithElse { else_block } => else_block,
+                        };
 
-                        // We change the target of the branch instruction.
-                        builder.change_jump_destination(branch_inst, else_ebb);
-                        builder.seal_block(else_ebb);
                         builder.switch_to_block(else_ebb);
 
                         // Again, no need to push the parameters for the `else`,
@@ -1260,18 +1276,6 @@ fn translate_unreachable_operator(
                         // `translate_operator` for details.
                     }
                 }
-                ControlStackFrame::If {
-                    else_data: ElseData::WithElse { else_block, .. },
-                    reachable_from_top,
-                    ..
-                } => {
-                    debug_assert!(
-                        !reachable_from_top,
-                        "should not be reachable from top if we have an else block"
-                    );
-                    builder.switch_to_block(else_block);
-                    state.reachable = true;
-                }
                 _ => unreachable!(),
             }
         }
@@ -1291,23 +1295,24 @@ fn translate_unreachable_operator(
                     // And loops can't have branches to the end.
                     false
                 }
+                // If we never set `consequent_ends_reachable` then that means
+                // we are finishing the consequent now, and there was no
+                // `else`. Whether the following block is reachable depends only
+                // on if the head was reachable.
                 ControlStackFrame::If {
-                    else_data: ElseData::WithElse { .. },
+                    head_is_reachable,
-                    reachable_from_top,
+                    consequent_ends_reachable: None,
                     ..
-                } => {
+                } => head_is_reachable,
-                    debug_assert!(!reachable_from_top);
+                // Since we are only in this function when in unreachable code,
-                    true
+                // we know that the alternative just ended unreachable. Whether
-                }
+                // the following block is reachable depends on if the consequent
+                // ended reachable or not.
                 ControlStackFrame::If {
-                    else_data: ElseData::NoElse { .. },
+                    head_is_reachable,
-                    reachable_from_top,
+                    consequent_ends_reachable: Some(consequent_ends_reachable),
                     ..
-                } => {
+                } => head_is_reachable && consequent_ends_reachable,
-                    // A reachable if without an else has a branch from the top
-                    // directly to the bottom.
-                    reachable_from_top
-                }
                 // All other control constructs are already handled.
                 _ => false,
             };

cranelift/wasm/src/state/func_state.rs

@@ -17,13 +17,22 @@ use std::vec::Vec;
 #[derive(Debug)]
 pub enum ElseData {
     /// The `if` does not already have an `else` block.
+    ///
+    /// This doesn't mean that it will never have an `else`, just that we
+    /// haven't seen it yet.
     NoElse {
         /// If we discover that we need an `else` block, this is the jump
         /// instruction that needs to be fixed up to point to the new `else`
         /// block rather than the destination block after the `if...end`.
         branch_inst: Inst,
     },
+
     /// We have already allocated an `else` block.
+    ///
+    /// Usually we don't know whether we will hit an `if .. end` or an `if
+    /// .. else .. end`, but sometimes we can tell based on the block's type
+    /// signature that the signature is not valid if there isn't an `else`. In
+    /// these cases, we pre-allocate the `else` block.
     WithElse {
         /// This is the `else` block.
         else_block: Ebb,
@@ -49,8 +58,18 @@ pub enum ControlStackFrame {
         num_return_values: usize,
         original_stack_size: usize,
         exit_is_branched_to: bool,
-        reachable_from_top: bool,
         blocktype: wasmparser::TypeOrFuncType,
+        /// Was the head of the `if` reachable?
+        head_is_reachable: bool,
+        /// What was the reachability at the end of the consequent?
+        ///
+        /// This is `None` until we're finished translating the consequent, and
+        /// is set to `Some` either by hitting an `else` when we will begin
+        /// translating the alternative, or by hitting an `end` in which case
+        /// there is no alternative.
+        consequent_ends_reachable: Option<bool>,
+        // Note: no need for `alternative_ends_reachable` because that is just
+        // `state.reachable` when we hit the `end` in the `if .. else .. end`.
     },
     Block {
         destination: Ebb,
@@ -370,11 +389,6 @@ impl FuncTranslationState {
             self.stack.push(val);
         }
 
-        let has_else = match else_data {
-            ElseData::NoElse { .. } => false,
-            ElseData::WithElse { .. } => true,
-        };
-
         self.control_stack.push(ControlStackFrame::If {
             destination,
             else_data,
@@ -382,7 +396,8 @@ impl FuncTranslationState {
             num_param_values: num_param_types,
             num_return_values: num_result_types,
             exit_is_branched_to: false,
-            reachable_from_top: self.reachable && !has_else,
+            head_is_reachable: self.reachable,
+            consequent_ends_reachable: None,
             blocktype,
         });
     }

cranelift/wasmtests/if-reachability-translation-0.wat

@@ -0,0 +1,12 @@
+;; An unreachable `if` means that the consequent, alternative, and following
+;; block are also unreachable.
+
+(module
+  (func (param i32) (result i32)
+    unreachable
+    if  ;; label = @2
+      nop
+    else
+      nop
+    end
+    i32.const 0))

cranelift/wasmtests/if-reachability-translation-1.wat

@@ -0,0 +1,12 @@
+;; Reachable `if` head and reachable consequent and alternative means that the
+;; following block is also reachable.
+
+(module
+  (func (param i32) (result i32)
+    local.get 0
+    if  ;; label = @2
+      nop
+    else
+      nop
+    end
+    i32.const 0))

cranelift/wasmtests/if-reachability-translation-2.wat

@@ -0,0 +1,12 @@
+;; Reachable `if` head and unreachable consequent and reachable alternative
+;; means that the following block is also reachable.
+
+(module
+  (func (param i32) (result i32)
+    local.get 0
+    if
+      unreachable
+    else
+      nop
+    end
+    i32.const 0))

cranelift/wasmtests/if-reachability-translation-3.wat

@@ -0,0 +1,12 @@
+;; Reachable `if` head and consequent and unreachable alternative means that the
+;; following block is also reachable.
+
+(module
+  (func (param i32) (result i32)
+    local.get 0
+    if
+      nop
+    else
+      unreachable
+    end
+    i32.const 0))

cranelift/wasmtests/if-reachability-translation-4.wat

@@ -0,0 +1,12 @@
+;; Reachable `if` head and unreachable consequent and alternative means that the
+;; following block is unreachable.
+
+(module
+  (func (param i32) (result i32)
+    local.get 0
+    if
+      unreachable
+    else
+      unreachable
+    end
+    i32.const 0))

cranelift/wasmtests/if-reachability-translation-5.wat

@@ -0,0 +1,14 @@
+;; Reachable `if` head and unreachable consequent and alternative, but with a
+;; branch out of the consequent, means that the following block is reachable.
+
+(module
+  (func (param i32 i32) (result i32)
+    local.get 0
+    if
+      local.get 1
+      br_if 0
+      unreachable
+    else
+      unreachable
+    end
+    i32.const 0))

cranelift/wasmtests/if-reachability-translation-6.wat

@@ -0,0 +1,14 @@
+;; Reachable `if` head and unreachable consequent and alternative, but with a
+;; branch out of the alternative, means that the following block is reachable.
+
+(module
+  (func (param i32 i32) (result i32)
+    local.get 0
+    if
+      unreachable
+    else
+      local.get 1
+      br_if 0
+      unreachable
+    end
+    i32.const 0))