Merge pull request #2197 from cfallin/fix-if-params

Account for duplicated if-block params on `end` op in unreachable case.

cranelift/wasm/src/code_translator.rs

@@ -332,24 +332,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
                 builder.seal_block(header)
             }
 
-            // The "If" frame pushes its parameters twice, so they're available to the else block
+            frame.truncate_value_stack_to_original_size(&mut state.stack);
-            // (see also `FuncTranslationState::push_if`).
-            // Yet, the original_stack_size member accounts for them only once, so that the else
-            // block can see the same number of parameters as the consequent block. As a matter of
-            // fact, we need to substract an extra number of parameter values for if blocks.
-            let num_duplicated_params = if let ControlStackFrame::If {
-                num_param_values, ..
-            } = frame
-            {
-                debug_assert!(num_param_values <= frame.original_stack_size());
-                num_param_values
-            } else {
-                0
-            };
-
-            state
-                .stack
-                .truncate(frame.original_stack_size() - num_duplicated_params);
             state
                 .stack
                 .extend_from_slice(builder.block_params(next_block));
@@ -1908,9 +1891,8 @@ fn translate_unreachable_operator<FE: FuncEnvironment + ?Sized>(
                                 let (params, _results) =
                                     blocktype_params_results(module_translation_state, blocktype)?;
                                 let else_block = block_with_params(builder, params, environ)?;
-                                state.stack.truncate(
+                                let frame = state.control_stack.last().unwrap();
-                                    state.control_stack.last().unwrap().original_stack_size(),
+                                frame.truncate_value_stack_to_else_params(&mut state.stack);
-                                );
 
                                 // We change the target of the branch instruction.
                                 builder.change_jump_destination(branch_inst, else_block);
@@ -1918,9 +1900,8 @@ fn translate_unreachable_operator<FE: FuncEnvironment + ?Sized>(
                                 else_block
                             }
                             ElseData::WithElse { else_block } => {
-                                state.stack.truncate(
+                                let frame = state.control_stack.last().unwrap();
-                                    state.control_stack.last().unwrap().original_stack_size(),
+                                frame.truncate_value_stack_to_else_params(&mut state.stack);
-                                );
                                 else_block
                             }
                         };
@@ -1941,9 +1922,8 @@ fn translate_unreachable_operator<FE: FuncEnvironment + ?Sized>(
             let control_stack = &mut state.control_stack;
             let frame = control_stack.pop().unwrap();
 
-            // Now we have to split off the stack the values not used
+            // Pop unused parameters from stack.
-            // by unreachable code that hasn't been translated
+            frame.truncate_value_stack_to_original_size(stack);
-            stack.truncate(frame.original_stack_size());
 
             let reachable_anyway = match frame {
                 // If it is a loop we also have to seal the body loop block

cranelift/wasm/src/state/func_state.rs

@@ -128,7 +128,9 @@ impl ControlStackFrame {
             Self::Loop { header, .. } => header,
         }
     }
-    pub fn original_stack_size(&self) -> usize {
+    /// Private helper. Use `truncate_value_stack_to_else_params()` or
+    /// `truncate_value_stack_to_original_size()` to restore value-stack state.
+    fn original_stack_size(&self) -> usize {
         match *self {
             Self::If {
                 original_stack_size,
@@ -178,6 +180,33 @@ impl ControlStackFrame {
             Self::Loop { .. } => {}
         }
     }
+
+    /// Pop values from the value stack so that it is left at the
+    /// input-parameters to an else-block.
+    pub fn truncate_value_stack_to_else_params(&self, stack: &mut Vec<Value>) {
+        debug_assert!(matches!(self, &ControlStackFrame::If { .. }));
+        stack.truncate(self.original_stack_size());
+    }
+
+    /// Pop values from the value stack so that it is left at the state it was
+    /// before this control-flow frame.
+    pub fn truncate_value_stack_to_original_size(&self, stack: &mut Vec<Value>) {
+        // The "If" frame pushes its parameters twice, so they're available to the else block
+        // (see also `FuncTranslationState::push_if`).
+        // Yet, the original_stack_size member accounts for them only once, so that the else
+        // block can see the same number of parameters as the consequent block. As a matter of
+        // fact, we need to substract an extra number of parameter values for if blocks.
+        let num_duplicated_params = match self {
+            &ControlStackFrame::If {
+                num_param_values, ..
+            } => {
+                debug_assert!(num_param_values <= self.original_stack_size());
+                num_param_values
+            }
+            _ => 0,
+        };
+        stack.truncate(self.original_stack_size() - num_duplicated_params);
+    }
 }
 
 /// Contains information passed along during a function's translation and that records:

cranelift/wasmtests/if-unreachable-else-params-2.wat

@@ -0,0 +1,18 @@
+(module
+  (type (;0;) (func (param i32 i32) (result f64)))
+  (func $main (type 0) (param i32 i32) (result f64)
+    f64.const 1.0
+    local.get 0
+    local.get 1
+    if (param i32)  ;; label = @2
+      i64.load16_s align=1
+      drop
+    else
+      unreachable
+    end)
+  (table (;0;) 63 255 funcref)
+  (memory (;0;) 13 16)
+  (export "t1" (table 0))
+  (export "m1" (memory 0))
+  (export "main" (func $main))
+  (export "memory" (memory 0)))