wasm2js: use scratch memory properly (#2033)

This replaces all uses of __tempMemory__, the old scratch space location, with calls to function imports for scratch memory access. This lets us then implement those in a way that does not use the same heap as main memory. This avoids possible bugs with scratch memory overwriting something, or just in general that it has observable side effects, which can confuse fuzzing etc.

The intrinsics are currently implemented in the glue. We could perhaps emit them inline instead (but that might limit asm.js optimizations, so I wanted to keep our options open for now - easy to change later).

Also fixes some places where we used 0 as the scratch space address.

Author: kripken

scripts/fuzz_opt.py

@@ -26,7 +26,7 @@
 
 # parameters
 
-NANS = False
+NANS = True
 
 FUZZ_OPTS = []  # '--all-features' etc
 

scripts/test/env.js

@@ -8,5 +8,3 @@ export function getTempRet0() {
   return tempRet0;
 }
 
-export const __tempMemory__ = 0;
-

src/abi/js.h

@@ -18,6 +18,7 @@
 #define wasm_abi_abi_h
 
 #include "wasm.h"
+#include "asmjs/shared-constants.h"
 
 namespace wasm {
 
@@ -36,6 +37,57 @@ inline std::string getLegalizationPass(LegalizationLevel level) {
   }
 }
 
+namespace wasm2js {
+
+extern cashew::IString SCRATCH_LOAD_I32,
+                       SCRATCH_STORE_I32,
+                       SCRATCH_LOAD_I64,
+                       SCRATCH_STORE_I64,
+                       SCRATCH_LOAD_F32,
+                       SCRATCH_STORE_F32,
+                       SCRATCH_LOAD_F64,
+                       SCRATCH_STORE_F64;
+
+// The wasm2js scratch memory helpers let us read and write to scratch memory
+// for purposes of implementing things like reinterpret, etc.
+// The optional "specific" parameter is a specific function we want. If not
+// provided, we create them all.
+inline void ensureScratchMemoryHelpers(Module* wasm, cashew::IString specific = cashew::IString()) {
+  auto ensureImport = [&](Name name, const std::vector<Type> params, Type result) {
+    if (wasm->getFunctionOrNull(name)) return;
+    if (specific.is() && name != specific) return;
+    auto func = make_unique<Function>();
+    func->name = name;
+    func->params = params;
+    func->result = result;
+    func->module = ENV;
+    func->base = name;
+    wasm->addFunction(std::move(func));
+  };
+
+  ensureImport(SCRATCH_LOAD_I32, { i32 }, i32);
+  ensureImport(SCRATCH_STORE_I32, { i32, i32 }, none);
+  ensureImport(SCRATCH_LOAD_I64, {}, i64);
+  ensureImport(SCRATCH_STORE_I64, { i64 }, none);
+  ensureImport(SCRATCH_LOAD_F32, {}, f32);
+  ensureImport(SCRATCH_STORE_F32, { f32 }, none);
+  ensureImport(SCRATCH_LOAD_F64, {}, f64);
+  ensureImport(SCRATCH_STORE_F64, { f64 }, none);
+}
+
+inline bool isScratchMemoryHelper(cashew::IString name) {
+  return name == SCRATCH_LOAD_I32 ||
+         name == SCRATCH_STORE_I32 ||
+         name == SCRATCH_LOAD_I64 ||
+         name == SCRATCH_STORE_I64 ||
+         name == SCRATCH_LOAD_F32 ||
+         name == SCRATCH_STORE_F32 ||
+         name == SCRATCH_LOAD_F64 ||
+         name == SCRATCH_STORE_F64;
+}
+
+} // namespace wasm2js
+
 } // namespace ABI
 
 } // namespace wasm

src/asmjs/shared-constants.cpp

@@ -102,4 +102,20 @@ cashew::IString GLOBAL("global"),
                 WASM_I64_UDIV("__wasm_i64_udiv"),
                 WASM_I64_SREM("__wasm_i64_srem"),
                 WASM_I64_UREM("__wasm_i64_urem");
+
+namespace ABI {
+namespace wasm2js {
+
+cashew::IString SCRATCH_LOAD_I32("wasm2js_scratch_load_i32"),
+                SCRATCH_STORE_I32("wasm2js_scratch_store_i32"),
+                SCRATCH_LOAD_I64("wasm2js_scratch_load_i64"),
+                SCRATCH_STORE_I64("wasm2js_scratch_store_i64"),
+                SCRATCH_LOAD_F32("wasm2js_scratch_load_f32"),
+                SCRATCH_STORE_F32("wasm2js_scratch_store_f32"),
+                SCRATCH_LOAD_F64("wasm2js_scratch_load_f64"),
+                SCRATCH_STORE_F64("wasm2js_scratch_store_f64");
+
+} // namespace wasm2js
+} // namespace ABI
+
 }

src/passes/I64ToI32Lowering.cpp

@@ -27,6 +27,7 @@
 #include "emscripten-optimizer/istring.h"
 #include "support/name.h"
 #include "wasm-builder.h"
+#include "abi/js.h"
 #include "ir/flat.h"
 #include "ir/iteration.h"
 #include "ir/memory-utils.h"
@@ -337,25 +338,30 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
   template<typename T>
   using BuilderFunc = std::function<T*(std::vector<Expression*>&, Type)>;
 
+  // Fixes up a call. If we performed fixups, returns the call; otherwise returns nullptr;
   template<typename T>
-  void visitGenericCall(T* curr, BuilderFunc<T> callBuilder) {
+  T* visitGenericCall(T* curr, BuilderFunc<T> callBuilder) {
+    bool fixed = false;
     std::vector<Expression*> args;
     for (auto* e : curr->operands) {
       args.push_back(e);
       if (hasOutParam(e)) {
         TempVar argHighBits = fetchOutParam(e);
         args.push_back(builder->makeGetLocal(argHighBits, i32));
+        fixed =  true;
       }
     }
     if (curr->type != i64) {
-      replaceCurrent(callBuilder(args, curr->type));
-      return;
+      auto* ret = callBuilder(args, curr->type);
+      replaceCurrent(ret);
+      return fixed ? ret : nullptr;
     }
     TempVar lowBits = getTemp();
     TempVar highBits = getTemp();
+    auto* call = callBuilder(args, i32);
     SetLocal* doCall = builder->makeSetLocal(
       lowBits,
-      callBuilder(args, i32)
+      call
     );
     SetLocal* setHigh = builder->makeSetLocal(
       highBits,
@@ -365,14 +371,21 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
     Block* result = builder->blockify(doCall, setHigh, getLow);
     setOutParam(result, std::move(highBits));
     replaceCurrent(result);
+    return call;
   }
   void visitCall(Call* curr) {
-    visitGenericCall<Call>(
+    auto* fixedCall = visitGenericCall<Call>(
       curr,
       [&](std::vector<Expression*>& args, Type ty) {
         return builder->makeCall(curr->target, args, ty);
       }
     );
+    // If this was to an import, we need to call the legal version. This assumes
+    // that legalize-js-interface has been run before.
+    if (fixedCall && getModule()->getFunction(fixedCall->target)->imported()) {
+      fixedCall->target = std::string("legalfunc$") + fixedCall->target.str;
+      return;
+    }
   }
 
   void visitCallIndirect(CallIndirect* curr) {
@@ -635,51 +648,31 @@ struct I64ToI32Lowering : public WalkerPass<PostWalker<I64ToI32Lowering>> {
     // our f64 through memory at address 0
     TempVar highBits = getTemp();
     Block *result = builder->blockify(
-      builder->makeStore(8, 0, 8, makeGetTempMemory(), curr->value, f64),
+      builder->makeCall(ABI::wasm2js::SCRATCH_STORE_F64, { curr->value }, none),
       builder->makeSetLocal(
         highBits,
-        builder->makeLoad(4, true, 4, 4, makeGetTempMemory(), i32)
+        builder->makeCall(ABI::wasm2js::SCRATCH_LOAD_I32, { builder->makeConst(Literal(int32_t(1))) }, i32)
       ),
-      builder->makeLoad(4, true, 0, 4, makeGetTempMemory(), i32)
+      builder->makeCall(ABI::wasm2js::SCRATCH_LOAD_I32, { builder->makeConst(Literal(int32_t(0))) }, i32)
     );
     setOutParam(result, std::move(highBits));
     replaceCurrent(result);
     MemoryUtils::ensureExists(getModule()->memory);
-    ensureTempMemoryGlobal();
+    ABI::wasm2js::ensureScratchMemoryHelpers(getModule());
   }
 
   void lowerReinterpretInt64(Unary* curr) {
     // Assume that the wasm file assumes the address 0 is invalid and roundtrip
     // our i64 through memory at address 0
     TempVar highBits = fetchOutParam(curr->value);
     Block *result = builder->blockify(
-      builder->makeStore(4, 0, 4, makeGetTempMemory(), curr->value, i32),
-      builder->makeStore(4, 4, 4, makeGetTempMemory(), builder->makeGetLocal(highBits, i32), i32),
-      builder->makeLoad(8, true, 0, 8, makeGetTempMemory(), f64)
+      builder->makeCall(ABI::wasm2js::SCRATCH_STORE_I32, { builder->makeConst(Literal(int32_t(0))), curr->value }, none),
+      builder->makeCall(ABI::wasm2js::SCRATCH_STORE_I32, { builder->makeConst(Literal(int32_t(1))), builder->makeGetLocal(highBits, i32) }, none),
+      builder->makeCall(ABI::wasm2js::SCRATCH_LOAD_F64, {}, f64)
     );
     replaceCurrent(result);
     MemoryUtils::ensureExists(getModule()->memory);
-    ensureTempMemoryGlobal();
-  }
-
-  Name tempMemory = "__tempMemory__";
-
-  void ensureTempMemoryGlobal() {
-    // Ensure the existence of an imported global, __tempMemory__, which points to 8
-    // bytes of scratch memory we can use for roundtrip purposes.
-    if (!getModule()->getGlobalOrNull(tempMemory)) {
-      auto global = make_unique<Global>();
-      global->name = tempMemory;
-      global->type = i32;
-      global->mutable_ = false;
-      global->module = ENV;
-      global->base = tempMemory;
-      getModule()->addGlobal(global.release());
-    }
-  }
-
-  Expression* makeGetTempMemory() {
-    return builder->makeGetGlobal(tempMemory, i32);
+    ABI::wasm2js::ensureScratchMemoryHelpers(getModule());
   }
 
   void lowerTruncFloatToInt(Unary *curr) {

src/passes/RemoveNonJSOps.cpp

@@ -33,6 +33,7 @@
 #include "asmjs/shared-constants.h"
 #include "wasm-builder.h"
 #include "wasm-s-parser.h"
+#include "abi/js.h"
 #include "ir/memory-utils.h"
 #include "ir/module-utils.h"
 #include "ir/find_all.h"
@@ -50,6 +51,9 @@ struct RemoveNonJSOpsPass : public WalkerPass<PostWalker<RemoveNonJSOpsPass>> {
   Pass* create() override { return new RemoveNonJSOpsPass; }
 
   void doWalkModule(Module* module) {
+    // Intrinsics may use scratch memory, ensure it.
+    ABI::wasm2js::ensureScratchMemoryHelpers(module);
+
     // Discover all of the intrinsics that we need to inject, lowering all
     // operations to intrinsic calls while we're at it.
     if (!builder) builder = make_unique<Builder>(*module);

src/passes/wasm-intrinsics.wast

@@ -7,8 +7,8 @@
 ;;
 ;; LOCAL MODS done by hand afterwards:
 ;;  * Remove hardcoded address 1024 (apparently a free memory location rustc
-;;    thinks is ok to use?); add a global __tempMemory__ which is used for that
-;;    purpose.
+;;    thinks is ok to use?); add intrinsic functions, which load/store to
+;;    special scratch space, wasm2js_scratch_load_i32 etc.
 ;;  * Fix function type of __wasm_ctz_i64, which was wrong somehow,
 ;;    i32, i32 => i32 instead of i64 => i64
 ;;
@@ -22,7 +22,8 @@
  (type $4 (func (param i32 i32) (result i32)))
  (type $5 (func (param i64) (result i64)))
  (import "env" "memory" (memory $0 17))
- (import "env" "__tempMemory__" (global $__tempMemory__ i32))
+ (import "env" "wasm2js_scratch_load_i64" (func $wasm2js_scratch_load_i64 (result i64)))
+ (import "env" "wasm2js_scratch_store_i64" (func $wasm2js_scratch_store_i64 (param i64)))
  (export "__wasm_i64_sdiv" (func $__wasm_i64_sdiv))
  (export "__wasm_i64_udiv" (func $__wasm_i64_udiv))
  (export "__wasm_i64_srem" (func $__wasm_i64_srem))
@@ -136,9 +137,7 @@
     (local.get $var$1)
    )
   )
-  (i64.load
-   (global.get $__tempMemory__)
-  )
+  (call $wasm2js_scratch_load_i64)
  )
  ;; lowering of the i64.mul instruction, return $var0 * $var$1
  (func $__wasm_i64_mul (; 4 ;) (type $0) (param $var$0 i64) (param $var$1 i64) (result i64)
@@ -579,8 +578,7 @@
                (i64.const 4294967296)
               )
              )
-             (i64.store
-              (global.get $__tempMemory__)
+             (call $wasm2js_scratch_store_i64
               (i64.extend_i32_u
                (i32.sub
                 (local.tee $var$2
@@ -641,8 +639,7 @@
               (local.get $var$3)
              )
             )
-            (i64.store
-             (global.get $__tempMemory__)
+            (call $wasm2js_scratch_store_i64
              (i64.or
               (i64.shl
                (i64.extend_i32_u
@@ -722,8 +719,7 @@
          )
          (br $label$3)
         )
-        (i64.store
-         (global.get $__tempMemory__)
+        (call $wasm2js_scratch_store_i64
          (i64.shl
           (i64.extend_i32_u
            (i32.sub
@@ -765,8 +761,7 @@
        )
        (br $label$2)
       )
-      (i64.store
-       (global.get $__tempMemory__)
+      (call $wasm2js_scratch_store_i64
        (i64.extend_i32_u
         (i32.and
          (local.get $var$4)
@@ -897,8 +892,7 @@
       )
      )
     )
-    (i64.store
-     (global.get $__tempMemory__)
+    (call $wasm2js_scratch_store_i64
      (local.get $var$5)
     )
     (return
@@ -911,8 +905,7 @@
      )
     )
    )
-   (i64.store
-    (global.get $__tempMemory__)
+   (call $wasm2js_scratch_store_i64
     (local.get $var$0)
    )
    (local.set $var$0