Bug 1603772: Cranelift: only return the ret value when it's not used for internal purposes; r=rhunt
authorBenjamin Bouvier <benj@benj.me>
Fri, 13 Dec 2019 19:35:58 +0000
changeset 507049 b92d5b9b880a082ca7c9a61ad9addc88c35f5ab8
parent 507048 c18a1b66855ca56a3933b5b63f5c5fa9d8eb8998
child 507050 fbeac1746d504ac63ad910d9c103ca2d65377d73
push id36922
push userncsoregi@mozilla.com
push dateMon, 16 Dec 2019 17:21:47 +0000
treeherdermozilla-central@27d0d6cc2131 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrhunt
bugs1603772
milestone73.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 1603772: Cranelift: only return the ret value when it's not used for internal purposes; r=rhunt The return value of a wasm builtin call may just be used to check if the runtime caused an internal error (e.g. oom). There are assertions in code that the return value of wasm builtins not supposed to return a wasm value actually do this, so we shouldn't return values that are only internally used. This could have been done a simpler way by only having "FailureMode::NotZero" imply "do not return", but this is more future-proof like this: shared memory / atomics builtins both check the internal value *and* return it to the wasm value stack. Differential Revision: https://phabricator.services.mozilla.com/D57125
js/src/wasm/cranelift/src/wasm2clif.rs
--- a/js/src/wasm/cranelift/src/wasm2clif.rs
+++ b/js/src/wasm/cranelift/src/wasm2clif.rs
@@ -110,17 +110,21 @@ pub fn init_sig(
     init_sig_from_wsig(call_conv, wsig)
 }
 
 /// An instance call may return a special value to indicate that the operation
 /// failed and we need to trap. This indicates what kind of value to check for,
 /// if any.
 enum FailureMode {
     Infallible,
-    NotZero,
+    /// The value returned by the function must be checked. internal_ret set to true indicates that
+    /// the returned value is only used internally, and should not be passed back to wasm.
+    NotZero {
+        internal_ret: bool,
+    },
 }
 
 /// A description of builtin call to the `wasm::Instance`.
 struct InstanceCall {
     address: SymbolicAddress,
     arguments: &'static [ir::Type],
     ret: Option<ir::Type>,
     failure_mode: FailureMode,
@@ -139,52 +143,52 @@ const FN_MEMORY_SIZE: InstanceCall = Ins
     arguments: &[],
     ret: Some(ir::types::I32),
     failure_mode: FailureMode::Infallible,
 };
 const FN_MEMORY_COPY: InstanceCall = InstanceCall {
     address: SymbolicAddress::MemoryCopy,
     arguments: &[ir::types::I32, ir::types::I32, ir::types::I32, POINTER_TYPE],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_MEMORY_COPY_SHARED: InstanceCall = InstanceCall {
     address: SymbolicAddress::MemoryCopyShared,
     arguments: &[ir::types::I32, ir::types::I32, ir::types::I32, POINTER_TYPE],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_MEMORY_FILL: InstanceCall = InstanceCall {
     address: SymbolicAddress::MemoryFill,
     arguments: &[ir::types::I32, ir::types::I32, ir::types::I32, POINTER_TYPE],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_MEMORY_FILL_SHARED: InstanceCall = InstanceCall {
     address: SymbolicAddress::MemoryFillShared,
     arguments: &[ir::types::I32, ir::types::I32, ir::types::I32, POINTER_TYPE],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_MEMORY_INIT: InstanceCall = InstanceCall {
     address: SymbolicAddress::MemoryInit,
     arguments: &[
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
     ],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_DATA_DROP: InstanceCall = InstanceCall {
     address: SymbolicAddress::DataDrop,
     arguments: &[ir::types::I32],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_TABLE_SIZE: InstanceCall = InstanceCall {
     address: SymbolicAddress::TableSize,
     arguments: &[ir::types::I32],
     ret: Some(ir::types::I32),
     failure_mode: FailureMode::Infallible,
 };
 const FN_TABLE_COPY: InstanceCall = InstanceCall {
@@ -192,35 +196,35 @@ const FN_TABLE_COPY: InstanceCall = Inst
     arguments: &[
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
     ],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_TABLE_INIT: InstanceCall = InstanceCall {
     address: SymbolicAddress::TableInit,
     arguments: &[
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
         ir::types::I32,
     ],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 const FN_ELEM_DROP: InstanceCall = InstanceCall {
     address: SymbolicAddress::ElemDrop,
     arguments: &[ir::types::I32],
     ret: Some(ir::types::I32),
-    failure_mode: FailureMode::NotZero,
+    failure_mode: FailureMode::NotZero { internal_ret: true },
 };
 
 // Custom trap codes specific to this embedding
 
 pub const TRAP_THROW_REPORTED: u16 = 1;
 
 /// A `TargetIsa` and `ModuleEnvironment` joined so we can implement `FuncEnvironment`.
 pub struct TransEnv<'a, 'b, 'c> {
@@ -553,23 +557,27 @@ impl<'a, 'b, 'c> TransEnv<'a, 'b, 'c> {
         self.reload_tls_and_pinned_regs(pos);
 
         if call.ret.is_none() {
             return None;
         }
 
         let ret = pos.func.dfg.first_result(call_ins);
         match call.failure_mode {
-            FailureMode::Infallible => {}
-            FailureMode::NotZero => {
+            FailureMode::Infallible => Some(ret),
+            FailureMode::NotZero { internal_ret } => {
                 pos.ins()
                     .trapnz(ret, ir::TrapCode::User(TRAP_THROW_REPORTED));
+                if internal_ret {
+                    None
+                } else {
+                    Some(ret)
+                }
             }
         }
-        Some(ret)
     }
 }
 
 impl<'a, 'b, 'c> TargetEnvironment for TransEnv<'a, 'b, 'c> {
     fn target_config(&self) -> TargetFrontendConfig {
         self.isa.frontend_config()
     }
 
@@ -1028,21 +1036,17 @@ impl<'a, 'b, 'c> FuncEnvironment for Tra
         len: ir::Value,
     ) -> WasmResult<()> {
         assert!(dst_table_index.index() == 0);
         assert!(src_table_index.index() == 0);
         // Slightly lower the amount of SSA nodes as `dst == src`
         let index = pos
             .ins()
             .iconst(ir::types::I32, dst_table_index.index() as i64);
-        let ret = self.instance_call(
-            &mut pos,
-            &FN_TABLE_COPY,
-            &[dst, src, len, index, index],
-        );
+        let ret = self.instance_call(&mut pos, &FN_TABLE_COPY, &[dst, src, len, index, index]);
         debug_assert!(ret.is_none());
         Ok(())
     }
 
     fn translate_table_init(
         &mut self,
         mut pos: FuncCursor,
         seg_index: u32,