Bug 1547682: Cranelift: properly handle unknown type errors; r=lth
☠☠ backed out by 00ca72fd66c2 ☠ ☠
authorBenjamin Bouvier <benj@benj.me>
Fri, 26 Apr 2019 18:16:27 +0200
changeset 531106 7bfcf5af011d1bdad6dd065d1b83cfb31ca60f2b
parent 531105 9fd0c4622f00d5173824b7ebef3119edbdfdf84b
child 531107 815455c1634eb1ca7f51f03a6117d4fe037809b5
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth
bugs1547682
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1547682: Cranelift: properly handle unknown type errors; r=lth Differential Revision: https://phabricator.services.mozilla.com/D29183
js/src/wasm/WasmCraneliftCompile.cpp
js/src/wasm/cranelift/src/baldrapi.rs
js/src/wasm/cranelift/src/baldrdash.rs
js/src/wasm/cranelift/src/compile.rs
js/src/wasm/cranelift/src/utils.rs
js/src/wasm/cranelift/src/wasm2clif.rs
--- a/js/src/wasm/WasmCraneliftCompile.cpp
+++ b/js/src/wasm/WasmCraneliftCompile.cpp
@@ -404,34 +404,18 @@ BD_ConstantValue global_constantValue(co
       v.u.f64 = value.f64();
       break;
     default:
       MOZ_CRASH("Bad type");
   }
   return v;
 }
 
-#ifdef DEBUG
-static bool IsCraneliftCompatible(TypeCode type) {
-  switch (type) {
-    case TypeCode::I32:
-    case TypeCode::I64:
-    case TypeCode::F32:
-    case TypeCode::F64:
-      return true;
-    default:
-      return false;
-  }
-}
-#endif
-
 TypeCode global_type(const GlobalDesc* global) {
-  TypeCode type = TypeCode(global->type().code());
-  MOZ_ASSERT(IsCraneliftCompatible(type));
-  return type;
+  return TypeCode(global->type().code());
 }
 
 size_t global_tlsOffset(const GlobalDesc* global) {
   return globalToTlsOffset(global->offset());
 }
 
 // TableDesc
 
@@ -444,21 +428,16 @@ size_t table_tlsOffset(const TableDesc* 
 
 // Sig
 
 size_t funcType_numArgs(const FuncTypeWithId* funcType) {
   return funcType->args().length();
 }
 
 const BD_ValType* funcType_args(const FuncTypeWithId* funcType) {
-#ifdef DEBUG
-  for (ValType valType : funcType->args()) {
-    MOZ_ASSERT(IsCraneliftCompatible(TypeCode(valType.code())));
-  }
-#endif
   static_assert(sizeof(BD_ValType) == sizeof(ValType), "update BD_ValType");
   return (const BD_ValType*)&funcType->args()[0];
 }
 
 TypeCode funcType_retType(const FuncTypeWithId* funcType) {
   return TypeCode(funcType->ret().code());
 }
 
--- a/js/src/wasm/cranelift/src/baldrapi.rs
+++ b/js/src/wasm/cranelift/src/baldrapi.rs
@@ -15,16 +15,19 @@
 
 //! This module exports the bindings generated by bindgen form the baldrapi.h file.
 //!
 //! The Baldr API consists of a set of C functions and some associated types.
 
 #![allow(non_upper_case_globals)]
 #![allow(non_camel_case_types)]
 #![allow(non_snake_case)]
+// We need to allow dead code because the Rustc compiler complains about variants never being
+// constructed in TypeCode, which is true because these values come from C++.
+#![allow(dead_code)]
 
 use cranelift_codegen::binemit::CodeOffset;
 use cranelift_codegen::entity::EntityRef;
 use cranelift_codegen::ir::SourceLoc;
 use cranelift_wasm::FuncIndex;
 
 use compile::CompiledFunc;
 
--- a/js/src/wasm/cranelift/src/baldrdash.rs
+++ b/js/src/wasm/cranelift/src/baldrdash.rs
@@ -11,106 +11,102 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 // Safe wrappers to the low-level ABI.  This re-exports all types in
 // baldrapi but none of the functions.
 
-use baldrapi::CraneliftModuleEnvironment;
 use cranelift_codegen::cursor::FuncCursor;
 use cranelift_codegen::entity::EntityRef;
 use cranelift_codegen::ir::immediates::{Ieee32, Ieee64};
 use cranelift_codegen::ir::{self, InstBuilder};
-use cranelift_wasm::{FuncIndex, GlobalIndex, SignatureIndex, TableIndex};
+use cranelift_wasm::{FuncIndex, GlobalIndex, SignatureIndex, TableIndex, WasmResult};
+
 use std::mem;
 use std::slice;
 
 use baldrapi;
+use baldrapi::BD_ValType as ValType;
+use baldrapi::CraneliftModuleEnvironment;
+use baldrapi::TypeCode;
+
+use utils::BasicError;
 
 pub use baldrapi::BD_SymbolicAddress as SymbolicAddress;
-pub use baldrapi::BD_ValType as ValType;
 pub use baldrapi::CraneliftCompiledFunc as CompiledFunc;
 pub use baldrapi::CraneliftFuncCompileInput as FuncCompileInput;
 pub use baldrapi::CraneliftMetadataEntry as MetadataEntry;
 pub use baldrapi::CraneliftStaticEnvironment as StaticEnvironment;
 pub use baldrapi::FuncTypeIdDescKind;
 pub use baldrapi::Trap;
-pub use baldrapi::TypeCode;
 
-/// Convert a `TypeCode` into the equivalent Cranelift type.
-///
-/// We expect Cranelift's `VOID` type to go away in the future, so use `None` to represent a
-/// function without a return value.
-impl Into<Option<ir::Type>> for TypeCode {
-    fn into(self) -> Option<ir::Type> {
-        match self {
-            TypeCode::I32 => Some(ir::types::I32),
-            TypeCode::I64 => Some(ir::types::I64),
-            TypeCode::F32 => Some(ir::types::F32),
-            TypeCode::F64 => Some(ir::types::F64),
-            TypeCode::BlockVoid => None,
-            _ => panic!("unexpected type"),
-        }
+/// Converts a `TypeCode` into the equivalent Cranelift type, if it's a known type, or an error
+/// otherwise.
+#[inline]
+fn typecode_to_type(type_code: TypeCode) -> WasmResult<Option<ir::Type>> {
+    match type_code {
+        TypeCode::I32 => Ok(Some(ir::types::I32)),
+        TypeCode::I64 => Ok(Some(ir::types::I64)),
+        TypeCode::F32 => Ok(Some(ir::types::F32)),
+        TypeCode::F64 => Ok(Some(ir::types::F64)),
+        TypeCode::BlockVoid => Ok(None),
+        _ => Err(BasicError::new(format!("unknown type code: {:?}", type_code)).into()),
     }
 }
 
 /// Convert a non-void `TypeCode` into the equivalent Cranelift type.
-impl Into<ir::Type> for TypeCode {
-    fn into(self) -> ir::Type {
-        match self.into() {
-            Some(t) => t,
-            None => panic!("unexpected void type"),
-        }
-    }
+#[inline]
+fn typecode_to_nonvoid_type(type_code: TypeCode) -> WasmResult<ir::Type> {
+    Ok(typecode_to_type(type_code)?.expect("unexpected void type"))
 }
 
 /// Convert a `TypeCode` into the equivalent Cranelift type.
-impl Into<ir::Type> for ValType {
-    fn into(self) -> ir::Type {
-        unsafe { baldrapi::env_unpack(self) }.into()
-    }
+#[inline]
+fn valtype_to_type(val_type: ValType) -> WasmResult<ir::Type> {
+    let type_code = unsafe { baldrapi::env_unpack(val_type) };
+    typecode_to_nonvoid_type(type_code)
 }
 
 /// Convert a u32 into a `BD_SymbolicAddress`.
 impl From<u32> for SymbolicAddress {
     fn from(x: u32) -> SymbolicAddress {
         assert!(x < SymbolicAddress::Limit as u32);
         unsafe { mem::transmute(x) }
     }
 }
 
 #[derive(Clone, Copy)]
 pub struct GlobalDesc(*const baldrapi::GlobalDesc);
 
 impl GlobalDesc {
-    pub fn value_type(self) -> TypeCode {
-        unsafe { baldrapi::global_type(self.0) }
+    pub fn value_type(self) -> WasmResult<ir::Type> {
+        let type_code = unsafe { baldrapi::global_type(self.0) };
+        typecode_to_nonvoid_type(type_code)
     }
 
     pub fn is_constant(self) -> bool {
         unsafe { baldrapi::global_isConstant(self.0) }
     }
 
     pub fn is_indirect(self) -> bool {
         unsafe { baldrapi::global_isIndirect(self.0) }
     }
 
     /// Insert an instruction at `pos` that materialized the constant value.
-    pub fn emit_constant(self, pos: &mut FuncCursor) -> ir::Value {
+    pub fn emit_constant(self, pos: &mut FuncCursor) -> WasmResult<ir::Value> {
         unsafe {
             let v = baldrapi::global_constantValue(self.0);
-            // Note that the floating point constants below
             match v.t {
-                TypeCode::I32 => pos.ins().iconst(ir::types::I32, i64::from(v.u.i32)),
-                TypeCode::I64 => pos.ins().iconst(ir::types::I64, v.u.i64),
-                TypeCode::F32 => pos.ins().f32const(Ieee32::with_bits(v.u.i32 as u32)),
-                TypeCode::F64 => pos.ins().f64const(Ieee64::with_bits(v.u.i64 as u64)),
-                _ => panic!("unexpected type"),
+                TypeCode::I32 => Ok(pos.ins().iconst(ir::types::I32, i64::from(v.u.i32))),
+                TypeCode::I64 => Ok(pos.ins().iconst(ir::types::I64, v.u.i64)),
+                TypeCode::F32 => Ok(pos.ins().f32const(Ieee32::with_bits(v.u.i32 as u32))),
+                TypeCode::F64 => Ok(pos.ins().f64const(Ieee64::with_bits(v.u.i64 as u64))),
+                _ => Err(BasicError::new(format!("unexpected type: {}", v.t as u64)).into()),
             }
         }
     }
 
     /// Get the offset from the `WasmTlsReg` to the memory representing this global variable.
     pub fn tls_offset(self) -> usize {
         unsafe { baldrapi::global_tlsOffset(self.0) }
     }
@@ -125,33 +121,37 @@ impl TableDesc {
         unsafe { baldrapi::table_tlsOffset(self.0) }
     }
 }
 
 #[derive(Clone, Copy)]
 pub struct FuncTypeWithId(*const baldrapi::FuncTypeWithId);
 
 impl FuncTypeWithId {
-    pub fn args<'a>(self) -> &'a [ValType] {
-        unsafe {
-            let num_args = baldrapi::funcType_numArgs(self.0);
-            // The `funcType_args` callback crashes when there are no arguments. Also note that
-            // `slice::from_raw_parts()` requires a non-null pointer for empty slices.
-            // TODO: We should get all the parts of a signature in a single callback that returns a
-            // struct.
-            if num_args == 0 {
-                &[]
-            } else {
-                slice::from_raw_parts(baldrapi::funcType_args(self.0), num_args)
+    pub fn args<'a>(self) -> WasmResult<Vec<ir::Type>> {
+        let num_args = unsafe { baldrapi::funcType_numArgs(self.0) };
+        // The `funcType_args` callback crashes when there are no arguments. Also note that
+        // `slice::from_raw_parts()` requires a non-null pointer for empty slices.
+        // TODO: We should get all the parts of a signature in a single callback that returns a
+        // struct.
+        if num_args == 0 {
+            Ok(Vec::new())
+        } else {
+            let args = unsafe { slice::from_raw_parts(baldrapi::funcType_args(self.0), num_args) };
+            let mut ret = Vec::new();
+            for &arg in args {
+                ret.push(valtype_to_type(arg)?);
             }
+            Ok(ret)
         }
     }
 
-    pub fn ret_type(self) -> TypeCode {
-        unsafe { baldrapi::funcType_retType(self.0) }
+    pub fn ret_type(self) -> WasmResult<Option<ir::Type>> {
+        let type_code = unsafe { baldrapi::funcType_retType(self.0) };
+        typecode_to_type(type_code)
     }
 
     pub fn id_kind(self) -> FuncTypeIdDescKind {
         unsafe { baldrapi::funcType_idKind(self.0) }
     }
 
     pub fn id_immediate(self) -> usize {
         unsafe { baldrapi::funcType_idImmediate(self.0) }
--- a/js/src/wasm/cranelift/src/compile.rs
+++ b/js/src/wasm/cranelift/src/compile.rs
@@ -104,17 +104,17 @@ impl<'a, 'b> BatchCompiler<'a, 'b> {
         &mut self,
         func: &bd::FuncCompileInput,
     ) -> WasmResult<bd::FuncTypeWithId> {
         self.context.clear();
 
         // Set up the signature before translating the WebAssembly byte code.
         // The translator refers to it.
         let index = FuncIndex::new(func.index as usize);
-        let wsig = init_sig(&mut self.context.func.signature, &self.environ, index);
+        let wsig = init_sig(&mut self.context.func.signature, &self.environ, index)?;
         self.context.func.name = wasm_function_name(index);
 
         let tenv = &mut TransEnv::new(&*self.isa, &self.environ, self.static_environ);
         self.trans.translate(
             func.bytecode(),
             func.offset_in_module as usize,
             &mut self.context.func,
             tenv,
--- a/js/src/wasm/cranelift/src/utils.rs
+++ b/js/src/wasm/cranelift/src/utils.rs
@@ -12,16 +12,18 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 /// Helpers common to other source files here.
 use std::error;
 use std::fmt;
 
+use cranelift_wasm::WasmError;
+
 type DashError = Box<error::Error>;
 pub type DashResult<T> = Result<T, DashError>;
 
 /// A simple error type that contains a string message, used to wrap raw Cranelift error types
 /// which don't implement std::error::Error.
 
 #[derive(Debug)]
 pub struct BasicError {
@@ -40,8 +42,14 @@ impl fmt::Display for BasicError {
     }
 }
 
 impl error::Error for BasicError {
     fn description(&self) -> &str {
         &self.msg
     }
 }
+
+impl Into<WasmError> for BasicError {
+    fn into(self) -> WasmError {
+        WasmError::User(self.into())
+    }
+}
--- a/js/src/wasm/cranelift/src/wasm2clif.rs
+++ b/js/src/wasm/cranelift/src/wasm2clif.rs
@@ -63,49 +63,51 @@ fn imm64(offset: usize) -> ir::immediate
 }
 
 /// Convert a usize offset into a `Uimm64`.
 fn uimm64(offset: usize) -> ir::immediates::Uimm64 {
     (offset as u64).into()
 }
 
 /// Initialize a `Signature` from a wasm signature.
-fn init_sig_from_wsig(sig: &mut ir::Signature, wsig: bd::FuncTypeWithId) {
+fn init_sig_from_wsig(sig: &mut ir::Signature, wsig: bd::FuncTypeWithId) -> WasmResult<()> {
     sig.clear(CallConv::Baldrdash);
-    for &arg in wsig.args() {
-        sig.params.push(ir::AbiParam::new(arg.into()));
+    for arg in wsig.args()? {
+        sig.params.push(ir::AbiParam::new(arg));
     }
 
-    if let Some(ret_type) = wsig.ret_type().into() {
+    if let Some(ret_type) = wsig.ret_type()? {
         let ret = match ret_type {
             // Spidermonkey requires i32 returns to have their high 32 bits
             // zero so that it can directly box them.
             ir::types::I32 => ir::AbiParam::new(ret_type).uext(),
             _ => ir::AbiParam::new(ret_type),
         };
         sig.returns.push(ret);
     }
 
     // Add a VM context pointer argument.
     // This corresponds to SpiderMonkey's `WasmTlsReg` hidden argument.
     sig.params.push(ir::AbiParam::special(
         native_pointer_type(),
         ir::ArgumentPurpose::VMContext,
     ));
+
+    Ok(())
 }
 
 /// Initialize the signature `sig` to match the function with `index` in `env`.
 pub fn init_sig(
     sig: &mut ir::Signature,
     env: &bd::ModuleEnvironment,
     func_index: FuncIndex,
-) -> bd::FuncTypeWithId {
+) -> WasmResult<bd::FuncTypeWithId> {
     let wsig = env.function_signature(func_index);
-    init_sig_from_wsig(sig, wsig);
-    wsig
+    init_sig_from_wsig(sig, wsig)?;
+    Ok(wsig)
 }
 
 /// A `TargetIsa` and `ModuleEnvironment` joined so we can implement `FuncEnvironment`.
 pub struct TransEnv<'a, 'b, 'c> {
     isa: &'a TargetIsa,
     env: &'b bd::ModuleEnvironment<'b>,
     static_env: &'c bd::StaticEnvironment,
 
@@ -383,25 +385,29 @@ impl<'a, 'b, 'c> FuncEnvironment for Tra
     fn target_config(&self) -> TargetFrontendConfig {
         self.isa.frontend_config()
     }
 
     fn pointer_type(&self) -> ir::Type {
         native_pointer_type()
     }
 
-    fn make_global(&mut self, func: &mut ir::Function, index: GlobalIndex) -> GlobalVariable {
+    fn make_global(
+        &mut self,
+        func: &mut ir::Function,
+        index: GlobalIndex,
+    ) -> WasmResult<GlobalVariable> {
         let global = self.env.global(index);
         if global.is_constant() {
             // Constant globals have a known value at compile time. We insert an instruction to
             // materialize the constant at the front of the entry block.
             let mut pos = FuncCursor::new(func);
             pos.next_ebb().expect("empty function");
             pos.next_inst();
-            return GlobalVariable::Const(global.emit_constant(&mut pos));
+            return Ok(GlobalVariable::Const(global.emit_constant(&mut pos)?));
         }
 
         // This is a global variable. Here we don't care if it is mutable or not.
         let vmctx_gv = self.get_vmctx_gv(func);
         let offset = global.tls_offset();
 
         // Some globals are represented as a pointer to the actual data, in which case we
         // must do an extra dereference to get to them.
@@ -412,24 +418,26 @@ impl<'a, 'b, 'c> FuncEnvironment for Tra
                 global_type: native_pointer_type(),
                 readonly: false,
             });
             (gv, 0.into())
         } else {
             (vmctx_gv, offset32(offset))
         };
 
-        GlobalVariable::Memory {
+        let mem_ty = global.value_type()?;
+
+        Ok(GlobalVariable::Memory {
             gv: base_gv,
-            ty: global.value_type().into(),
+            ty: mem_ty,
             offset,
-        }
+        })
     }
 
-    fn make_heap(&mut self, func: &mut ir::Function, index: MemoryIndex) -> ir::Heap {
+    fn make_heap(&mut self, func: &mut ir::Function, index: MemoryIndex) -> WasmResult<ir::Heap> {
         assert_eq!(index.index(), 0, "Only one WebAssembly memory supported");
         // Get the address of the `TlsData::memoryBase` field.
         let base_addr = self.get_vmctx_gv(func);
         // Get the `TlsData::memoryBase` field. We assume this is never modified during execution
         // of the function.
         let base = func.create_global_value(ir::GlobalValueData::Load {
             base: base_addr,
             offset: offset32(0),
@@ -450,42 +458,46 @@ impl<'a, 'b, 'c> FuncEnvironment for Tra
                 base: base_addr,
                 offset: native_pointer_size().into(),
                 global_type: ir::types::I32,
                 readonly: false,
             });
             ir::HeapStyle::Dynamic { bound_gv }
         };
 
-        func.create_heap(ir::HeapData {
+        Ok(func.create_heap(ir::HeapData {
             base,
             min_size,
             offset_guard_size: guard_size,
             style,
             index_type: ir::types::I32,
-        })
+        }))
     }
 
-    fn make_indirect_sig(&mut self, func: &mut ir::Function, index: SignatureIndex) -> ir::SigRef {
+    fn make_indirect_sig(
+        &mut self,
+        func: &mut ir::Function,
+        index: SignatureIndex,
+    ) -> WasmResult<ir::SigRef> {
         let mut sigdata = ir::Signature::new(CallConv::Baldrdash);
         let wsig = self.env.signature(index);
-        init_sig_from_wsig(&mut sigdata, wsig);
+        init_sig_from_wsig(&mut sigdata, wsig)?;
 
         if wsig.id_kind() != bd::FuncTypeIdDescKind::None {
             // A signature to be used for an indirect call also takes a signature id.
             sigdata.params.push(ir::AbiParam::special(
                 native_pointer_type(),
                 ir::ArgumentPurpose::SignatureId,
             ));
         }
 
-        func.import_signature(sigdata)
+        Ok(func.import_signature(sigdata))
     }
 
-    fn make_table(&mut self, func: &mut ir::Function, index: TableIndex) -> ir::Table {
+    fn make_table(&mut self, func: &mut ir::Function, index: TableIndex) -> WasmResult<ir::Table> {
         let table_desc = self.get_table(func, index);
 
         // TODO we'd need a better way to synchronize the shape of GlobalDataDesc and these
         // offsets.
         let bound_gv = func.create_global_value(ir::GlobalValueData::Load {
             base: table_desc.global,
             offset: 0.into(),
             global_type: ir::types::I32,
@@ -494,36 +506,40 @@ impl<'a, 'b, 'c> FuncEnvironment for Tra
 
         let base_gv = func.create_global_value(ir::GlobalValueData::Load {
             base: table_desc.global,
             offset: offset32(native_pointer_size() as usize),
             global_type: native_pointer_type(),
             readonly: false,
         });
 
-        func.create_table(ir::TableData {
+        Ok(func.create_table(ir::TableData {
             base_gv,
             min_size: ir::immediates::Uimm64::new(0),
             bound_gv,
             element_size: ir::immediates::Uimm64::new(u64::from(self.pointer_bytes()) * 2),
             index_type: ir::types::I32,
-        })
+        }))
     }
 
-    fn make_direct_func(&mut self, func: &mut ir::Function, index: FuncIndex) -> ir::FuncRef {
+    fn make_direct_func(
+        &mut self,
+        func: &mut ir::Function,
+        index: FuncIndex,
+    ) -> WasmResult<ir::FuncRef> {
         // Create a signature.
         let mut sigdata = ir::Signature::new(CallConv::Baldrdash);
-        init_sig(&mut sigdata, &self.env, index);
+        init_sig(&mut sigdata, &self.env, index)?;
         let signature = func.import_signature(sigdata);
 
-        func.import_function(ir::ExtFuncData {
+        Ok(func.import_function(ir::ExtFuncData {
             name: wasm_function_name(index),
             signature,
             colocated: true,
-        })
+        }))
     }
 
     fn translate_call_indirect(
         &mut self,
         mut pos: FuncCursor,
         table_index: TableIndex,
         table: ir::Table,
         sig_index: SignatureIndex,
@@ -747,19 +763,20 @@ impl<'a, 'b, 'c> FuncEnvironment for Tra
             .special_param(ir::ArgumentPurpose::VMContext)
             .expect("Missing vmctx arg");
         let addr = pos.ins().func_addr(native_pointer_type(), fnref);
         let call = pos.ins().call_indirect(sigref, addr, &[instance, vmctx]);
         self.switch_to_wasm_tls_realm(&mut pos);
         Ok(pos.func.dfg.first_result(call))
     }
 
-    fn translate_loop_header(&mut self, mut pos: FuncCursor) {
+    fn translate_loop_header(&mut self, mut pos: FuncCursor) -> WasmResult<()> {
         let interrupt = self.load_interrupt_flag(&mut pos);
         pos.ins().trapnz(interrupt, ir::TrapCode::Interrupt);
+        Ok(())
     }
 
     fn return_mode(&self) -> ReturnMode {
         // Since we're using SM's epilogue insertion code, we can only handle a single return
         // instruction at the end of the function.
         ReturnMode::FallthroughReturn
     }
 }