diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index bc75157a88..6dd7deb1ff 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -200,62 +200,69 @@ pub fn translate_operator( let i = state.control_stack.len() - 1; match state.control_stack[i] { ControlStackFrame::If { - else_data: ElseData::NoElse { branch_inst }, - ref mut reachable_from_top, + ref else_data, + 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 - // as the else body and push a new control frame with a new - // ebb for the code after the if/then/else. At the end of the - // then clause we jump to the destination. + // We finished the consequent, so record its final + // reachability state. + debug_assert!(consequent_ends_reachable.is_none()); + *consequent_ends_reachable = Some(state.reachable); - // The `if` has an `else`, so there's no branch to the end from the top. - *reachable_from_top = false; + if head_is_reachable { + // We have a branch from the head of the `if` to the `else`. + state.reachable = true; - let (params, _results) = - blocktype_params_results(module_translation_state, blocktype)?; - let else_ebb = ebb_with_params(builder, params)?; - builder.ins().jump(destination, state.peekn(params.len())); - state.popn(params.len()); + // Ensure we have an ebb for the `else` block (it may have + // already been pre-allocated, see `ElseData` for details). + let else_ebb = match *else_data { + ElseData::NoElse { branch_inst } => { + 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 - // `else` block here, something like this: - // - // 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. + builder.change_jump_destination(branch_inst, else_ebb); + builder.seal_block(else_ebb); + else_ebb + } + ElseData::WithElse { else_block } => { + builder + .ins() + .jump(destination, state.peekn(num_return_values)); + state.popn(num_return_values); + else_block + } + }; - builder.change_jump_destination(branch_inst, else_ebb); - builder.seal_block(else_ebb); - builder.switch_to_block(else_ebb); + // You might be expecting that we push the parameters for this + // `else` block here, something like this: + // + // 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 - // `ElseData` because nothing else will read it. - } - ControlStackFrame::If { - 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); + builder.switch_to_block(else_ebb); + + // We don't bother updating the control frame's `ElseData` + // to `WithElse` because nothing else will read it. + } } _ => 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 mut reachable_from_top, + ref else_data, + head_is_reachable, + ref mut consequent_ends_reachable, blocktype, .. } => { - if *reachable_from_top { - // We have a branch from the top of the if to the else. + debug_assert!(consequent_ends_reachable.is_none()); + *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) = - blocktype_params_results(module_translation_state, blocktype)?; - let else_ebb = ebb_with_params(builder, params)?; + let else_ebb = match *else_data { + ElseData::NoElse { branch_inst } => { + 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 { .. }, - reachable_from_top, + head_is_reachable, + consequent_ends_reachable: None, .. - } => { - debug_assert!(!reachable_from_top); - true - } + } => head_is_reachable, + // Since we are only in this function when in unreachable code, + // 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 { .. }, - reachable_from_top, + head_is_reachable, + consequent_ends_reachable: Some(consequent_ends_reachable), .. - } => { - // A reachable if without an else has a branch from the top - // directly to the bottom. - reachable_from_top - } + } => head_is_reachable && consequent_ends_reachable, // All other control constructs are already handled. _ => false, }; diff --git a/cranelift/wasm/src/state/func_state.rs b/cranelift/wasm/src/state/func_state.rs index 4b3627e008..88d83ab846 100644 --- a/cranelift/wasm/src/state/func_state.rs +++ b/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, + // 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, }); } diff --git a/cranelift/wasmtests/if-reachability-translation-0.wat b/cranelift/wasmtests/if-reachability-translation-0.wat new file mode 100644 index 0000000000..54145a9d4e --- /dev/null +++ b/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)) diff --git a/cranelift/wasmtests/if-reachability-translation-1.wat b/cranelift/wasmtests/if-reachability-translation-1.wat new file mode 100644 index 0000000000..6e1e6121f4 --- /dev/null +++ b/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)) diff --git a/cranelift/wasmtests/if-reachability-translation-2.wat b/cranelift/wasmtests/if-reachability-translation-2.wat new file mode 100644 index 0000000000..4bbaf99820 --- /dev/null +++ b/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)) diff --git a/cranelift/wasmtests/if-reachability-translation-3.wat b/cranelift/wasmtests/if-reachability-translation-3.wat new file mode 100644 index 0000000000..72251cba16 --- /dev/null +++ b/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)) diff --git a/cranelift/wasmtests/if-reachability-translation-4.wat b/cranelift/wasmtests/if-reachability-translation-4.wat new file mode 100644 index 0000000000..b8a4069430 --- /dev/null +++ b/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)) diff --git a/cranelift/wasmtests/if-reachability-translation-5.wat b/cranelift/wasmtests/if-reachability-translation-5.wat new file mode 100644 index 0000000000..7b1f665e05 --- /dev/null +++ b/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)) diff --git a/cranelift/wasmtests/if-reachability-translation-6.wat b/cranelift/wasmtests/if-reachability-translation-6.wat new file mode 100644 index 0000000000..d9da824f14 --- /dev/null +++ b/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))