Validate modules while translating

This commit is a change to cranelift-wasm to validate each function body
as it is translated. Additionally top-level module translation functions
will perform module validation. This commit builds on changes in
wasmparser to perform module validation interwtwined with parsing and
translation. This will be necessary for future wasm features such as
module linking where the type behind a function index, for example, can
be far away in another module. Additionally this also brings a nice
benefit where parsing the binary only happens once (instead of having an
up-front serial validation step) and validation can happen in parallel
for each function.

Most of the changes in this commit are plumbing to make sure everything
lines up right. The major functional change here is that module
compilation should be faster by validating in parallel (or skipping
function validation entirely in the case of a cache hit). Otherwise from
a user-facing perspective nothing should be that different.

This commit does mean that cranelift's translation now inherently
validates the input wasm module. This means that the Spidermonkey
integration of cranelift-wasm will also be validating the function as
it's being translated with cranelift. The associated PR for wasmparser
(bytecodealliance/wasmparser#62) provides the necessary tools to create
a `FuncValidator` for Gecko, but this is something I'll want careful
review for before landing!

Cargo.toml

@@ -95,3 +95,7 @@ harness = false
 
 [profile.dev.package.backtrace]
 debug = false # FIXME(#1813)
+
+[patch.crates-io]
+wasmparser = { git = 'https://github.com/alexcrichton/wasm-tools', branch = 'wasmtime-parser-updates' }
+wasmprinter = { git = 'https://github.com/alexcrichton/wasm-tools', branch = 'wasmtime-parser-updates' }

cranelift/entity/src/iter.rs

@@ -1,6 +1,7 @@
 //! A double-ended iterator over entity references and entities.
 
 use crate::EntityRef;
+use alloc::vec;
 use core::iter::Enumerate;
 use core::marker::PhantomData;
 use core::slice;
@@ -84,3 +85,40 @@ impl<'a, K: EntityRef, V> DoubleEndedIterator for IterMut<'a, K, V> {
 }
 
 impl<'a, K: EntityRef, V> ExactSizeIterator for IterMut<'a, K, V> {}
+
+/// Iterate over all keys in order.
+pub struct IntoIter<K: EntityRef, V> {
+    enumerate: Enumerate<vec::IntoIter<V>>,
+    unused: PhantomData<K>,
+}
+
+impl<K: EntityRef, V> IntoIter<K, V> {
+    /// Create an `IntoIter` iterator that visits the `PrimaryMap` keys and values
+    /// of `iter`.
+    pub fn new(iter: vec::IntoIter<V>) -> Self {
+        Self {
+            enumerate: iter.enumerate(),
+            unused: PhantomData,
+        }
+    }
+}
+
+impl<K: EntityRef, V> Iterator for IntoIter<K, V> {
+    type Item = (K, V);
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.enumerate.next().map(|(i, v)| (K::new(i), v))
+    }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        self.enumerate.size_hint()
+    }
+}
+
+impl<K: EntityRef, V> DoubleEndedIterator for IntoIter<K, V> {
+    fn next_back(&mut self) -> Option<Self::Item> {
+        self.enumerate.next_back().map(|(i, v)| (K::new(i), v))
+    }
+}
+
+impl<K: EntityRef, V> ExactSizeIterator for IntoIter<K, V> {}

cranelift/entity/src/primary.rs

@@ -1,6 +1,6 @@
 //! Densely numbered entity references as mapping keys.
 use crate::boxed_slice::BoxedSlice;
-use crate::iter::{Iter, IterMut};
+use crate::iter::{IntoIter, Iter, IterMut};
 use crate::keys::Keys;
 use crate::EntityRef;
 use alloc::boxed::Box;
@@ -150,6 +150,15 @@ where
     }
 }
 
+impl<K, V> Default for PrimaryMap<K, V>
+where
+    K: EntityRef,
+{
+    fn default() -> PrimaryMap<K, V> {
+        PrimaryMap::new()
+    }
+}
+
 /// Immutable indexing into an `PrimaryMap`.
 /// The indexed value must be in the map.
 impl<K, V> Index<K> for PrimaryMap<K, V>
@@ -197,6 +206,18 @@ where
     }
 }
 
+impl<K, V> IntoIterator for PrimaryMap<K, V>
+where
+    K: EntityRef,
+{
+    type Item = (K, V);
+    type IntoIter = IntoIter<K, V>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        IntoIter::new(self.elems.into_iter())
+    }
+}
+
 impl<K, V> FromIterator<V> for PrimaryMap<K, V>
 where
     K: EntityRef,

cranelift/wasm/src/code_translator.rs

@@ -24,7 +24,7 @@
 //! argument.
 use super::{hash_map, HashMap};
 use crate::environ::{FuncEnvironment, GlobalVariable, ReturnMode, WasmResult};
-use crate::state::{ControlStackFrame, ElseData, FuncTranslationState, ModuleTranslationState};
+use crate::state::{ControlStackFrame, ElseData, FuncTranslationState};
 use crate::translation_utils::{
     block_with_params, blocktype_params_results, f32_translation, f64_translation,
 };
@@ -43,7 +43,7 @@ use cranelift_frontend::{FunctionBuilder, Variable};
 use std::cmp;
 use std::convert::TryFrom;
 use std::vec::Vec;
-use wasmparser::{MemoryImmediate, Operator};
+use wasmparser::{FuncValidator, MemoryImmediate, Operator};
 
 // Clippy warns about "flags: _" but its important to document that the flags field is ignored
 #[cfg_attr(
@@ -53,14 +53,14 @@ use wasmparser::{MemoryImmediate, Operator};
 /// Translates wasm operators into Cranelift IR instructions. Returns `true` if it inserted
 /// a return.
 pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
-    module_translation_state: &ModuleTranslationState,
+    validator: &mut FuncValidator,
     op: &Operator,
     builder: &mut FunctionBuilder,
     state: &mut FuncTranslationState,
     environ: &mut FE,
 ) -> WasmResult<()> {
     if !state.reachable {
-        translate_unreachable_operator(module_translation_state, &op, builder, state, environ)?;
+        translate_unreachable_operator(validator, &op, builder, state, environ)?;
         return Ok(());
     }
 
@@ -180,12 +180,12 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
          *  possible `Block`'s arguments values.
          ***********************************************************************************/
         Operator::Block { ty } => {
-            let (params, results) = blocktype_params_results(module_translation_state, *ty)?;
+            let (params, results) = blocktype_params_results(validator, *ty)?;
             let next = block_with_params(builder, results, environ)?;
             state.push_block(next, params.len(), results.len());
         }
         Operator::Loop { ty } => {
-            let (params, results) = blocktype_params_results(module_translation_state, *ty)?;
+            let (params, results) = blocktype_params_results(validator, *ty)?;
             let loop_body = block_with_params(builder, params, environ)?;
             let next = block_with_params(builder, results, environ)?;
             builder.ins().jump(loop_body, state.peekn(params.len()));
@@ -204,7 +204,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
         Operator::If { ty } => {
             let val = state.pop1();
 
-            let (params, results) = blocktype_params_results(module_translation_state, *ty)?;
+            let (params, results) = blocktype_params_results(validator, *ty)?;
             let (destination, else_data) = if params == results {
                 // It is possible there is no `else` block, so we will only
                 // allocate a block for it if/when we find the `else`. For now,
@@ -268,7 +268,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
                         let else_block = match *else_data {
                             ElseData::NoElse { branch_inst } => {
                                 let (params, _results) =
-                                    blocktype_params_results(module_translation_state, blocktype)?;
+                                    blocktype_params_results(validator, blocktype)?;
                                 debug_assert_eq!(params.len(), num_return_values);
                                 let else_block = block_with_params(builder, params, environ)?;
                                 builder.ins().jump(destination, state.peekn(params.len()));
@@ -384,9 +384,10 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
         }
         Operator::BrIf { relative_depth } => translate_br_if(*relative_depth, builder, state),
         Operator::BrTable { table } => {
-            let (depths, default) = table.read_table()?;
+            let mut depths = table.targets().collect::<Result<Vec<_>, _>>()?;
+            let default = depths.pop().unwrap().0;
             let mut min_depth = default;
-            for depth in &*depths {
+            for (depth, _) in depths.iter() {
                 if *depth < min_depth {
                     min_depth = *depth;
                 }
@@ -404,7 +405,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             let mut data = JumpTableData::with_capacity(depths.len());
             if jump_args_count == 0 {
                 // No jump arguments
-                for depth in &*depths {
+                for (depth, _) in depths.iter() {
                     let block = {
                         let i = state.control_stack.len() - 1 - (*depth as usize);
                         let frame = &mut state.control_stack[i];
@@ -427,7 +428,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
                 let return_count = jump_args_count;
                 let mut dest_block_sequence = vec![];
                 let mut dest_block_map = HashMap::new();
-                for depth in &*depths {
+                for (depth, _) in depths.iter() {
                     let branch_block = match dest_block_map.entry(*depth as usize) {
                         hash_map::Entry::Occupied(entry) => *entry.get(),
                         hash_map::Entry::Vacant(entry) => {
@@ -1051,9 +1052,9 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
             let index = FuncIndex::from_u32(*function_index);
             state.push1(environ.translate_ref_func(builder.cursor(), index)?);
         }
-        Operator::AtomicNotify { .. }
-        | Operator::I32AtomicWait { .. }
-        | Operator::I64AtomicWait { .. }
+        Operator::MemoryAtomicNotify { .. }
+        | Operator::MemoryAtomicWait32 { .. }
+        | Operator::MemoryAtomicWait64 { .. }
         | Operator::I32AtomicLoad { .. }
         | Operator::I64AtomicLoad { .. }
         | Operator::I32AtomicLoad8U { .. }
@@ -1631,7 +1632,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
 /// are dropped but special ones like `End` or `Else` signal the potential end of the unreachable
 /// portion so the translation state must be updated accordingly.
 fn translate_unreachable_operator<FE: FuncEnvironment + ?Sized>(
-    module_translation_state: &ModuleTranslationState,
+    validator: &FuncValidator,
     op: &Operator,
     builder: &mut FunctionBuilder,
     state: &mut FuncTranslationState,
@@ -1675,7 +1676,7 @@ fn translate_unreachable_operator<FE: FuncEnvironment + ?Sized>(
                         let else_block = match *else_data {
                             ElseData::NoElse { branch_inst } => {
                                 let (params, _results) =
-                                    blocktype_params_results(module_translation_state, blocktype)?;
+                                    blocktype_params_results(validator, blocktype)?;
                                 let else_block = block_with_params(builder, params, environ)?;
 
                                 // We change the target of the branch instruction.

cranelift/wasm/src/environ/dummy.rs

@@ -10,7 +10,6 @@ use crate::environ::{
     WasmFuncType, WasmResult,
 };
 use crate::func_translator::FuncTranslator;
-use crate::state::ModuleTranslationState;
 use crate::translation_utils::{
     DataIndex, DefinedFuncIndex, ElemIndex, FuncIndex, Global, GlobalIndex, Memory, MemoryIndex,
     SignatureIndex, Table, TableIndex,
@@ -26,6 +25,7 @@ use cranelift_frontend::FunctionBuilder;
 use std::boxed::Box;
 use std::string::String;
 use std::vec::Vec;
+use wasmparser::{FuncValidator, FunctionBody};
 
 /// Compute a `ir::ExternalName` for a given wasm function index.
 fn get_func_name(func_index: FuncIndex) -> ir::ExternalName {
@@ -715,10 +715,11 @@ impl<'data> ModuleEnvironment<'data> for DummyEnvironment {
 
     fn define_function_body(
         &mut self,
-        module_translation_state: &ModuleTranslationState,
-        body_bytes: &'data [u8],
-        body_offset: usize,
+        mut validator: FuncValidator,
+        body: FunctionBody<'data>,
     ) -> WasmResult<()> {
+        self.func_bytecode_sizes
+            .push(body.get_binary_reader().bytes_remaining());
         let func = {
             let mut func_environ = DummyFuncEnvironment::new(&self.info, self.return_mode);
             let func_index =
@@ -729,16 +730,10 @@ impl<'data> ModuleEnvironment<'data> for DummyEnvironment {
             if self.debug_info {
                 func.collect_debug_info();
             }
-            self.trans.translate(
-                module_translation_state,
-                body_bytes,
-                body_offset,
-                &mut func,
-                &mut func_environ,
-            )?;
+            self.trans
+                .translate_body(&mut validator, body, &mut func, &mut func_environ)?;
             func
         };
-        self.func_bytecode_sizes.push(body_bytes.len());
         self.info.function_bodies.push(func);
         Ok(())
     }

cranelift/wasm/src/environ/spec.rs

@@ -6,7 +6,7 @@
 //!
 //! [Wasmtime]: https://github.com/bytecodealliance/wasmtime
 
-use crate::state::{FuncTranslationState, ModuleTranslationState};
+use crate::state::FuncTranslationState;
 use crate::translation_utils::{
     DataIndex, ElemIndex, FuncIndex, Global, GlobalIndex, Memory, MemoryIndex, SignatureIndex,
     Table, TableIndex,
@@ -23,8 +23,7 @@ use serde::{Deserialize, Serialize};
 use std::boxed::Box;
 use std::string::ToString;
 use thiserror::Error;
-use wasmparser::BinaryReaderError;
-use wasmparser::Operator;
+use wasmparser::{BinaryReaderError, FuncValidator, FunctionBody, Operator, WasmFeatures};
 
 /// WebAssembly value type -- equivalent of `wasmparser`'s Type.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
@@ -749,9 +748,8 @@ pub trait ModuleEnvironment<'data>: TargetEnvironment {
     /// functions is already provided by `reserve_func_types`.
     fn define_function_body(
         &mut self,
-        module_translation_state: &ModuleTranslationState,
-        body_bytes: &'data [u8],
-        body_offset: usize,
+        validator: FuncValidator,
+        body: FunctionBody<'data>,
     ) -> WasmResult<()>;
 
     /// Provides the number of data initializers up front. By default this does nothing, but
@@ -789,4 +787,9 @@ pub trait ModuleEnvironment<'data>: TargetEnvironment {
     fn custom_section(&mut self, _name: &'data str, _data: &'data [u8]) -> WasmResult<()> {
         Ok(())
     }
+
+    /// Returns the list of enabled wasm features this translation will be using.
+    fn wasm_features(&self) -> WasmFeatures {
+        WasmFeatures::default()
+    }
 }