From c24b9707fd0595165cd003e17123c9a842072aa2 Mon Sep 17 00:00:00 2001 From: Daniel Varga Date: Thu, 2 Apr 2020 12:48:09 +0300 Subject: [PATCH] Backed out 2 changesets (bug 1625927, bug 1625887) on request by dev Backed out changeset 98f78bb87300 (bug 1625927) Backed out changeset 1e25841508c8 (bug 1625887) --- js/moz.configure | 6 +- .../tests/wasm/multi-value/call-js.js | 49 +----- .../tests/wasm/multi-value/call-ref.js | 27 +-- js/src/wasm/WasmCode.h | 2 +- js/src/wasm/WasmInstance.cpp | 162 +++--------------- js/src/wasm/WasmStubs.cpp | 83 +++++---- js/src/wasm/WasmTypes.h | 8 +- .../jsapi/constructor/multi-value.any.js.ini | 21 +-- 8 files changed, 89 insertions(+), 269 deletions(-) diff --git a/js/moz.configure b/js/moz.configure index 22f7e65483c2..8980b6623ca5 100644 --- a/js/moz.configure +++ b/js/moz.configure @@ -596,9 +596,9 @@ set_define('WASM_PRIVATE_REFTYPES', depends_if('--enable-wasm-private-reftypes') # Do not remove until Cranelift supports multi-value. # ===================================================== -@depends(milestone.is_nightly) -def default_wasm_multi_value(is_nightly): - return is_nightly +@depends(milestone.is_nightly, building_js) +def default_wasm_multi_value(is_nightly, building_js): + return is_nightly and building_js js_option('--enable-wasm-multi-value', default=default_wasm_multi_value, diff --git a/js/src/jit-test/tests/wasm/multi-value/call-js.js b/js/src/jit-test/tests/wasm/multi-value/call-js.js index f7bf554b4a76..5701df908555 100644 --- a/js/src/jit-test/tests/wasm/multi-value/call-js.js +++ b/js/src/jit-test/tests/wasm/multi-value/call-js.js @@ -96,47 +96,16 @@ expectMultiValueError(() => (function*() { yield 52; yield 10; yield 0; yield 0; assertEq(calls.join(','), '52,10,0'); } -function expectMultiValueResult(text, expected) { - let instance = wasmEvalText(text); - assertDeepEq(instance.exports.run(), expected); +function expectRunFailure(text, pattern, imports) { + let instance = wasmEvalText(text, imports); + assertErrorMessage(() => instance.exports.run(), + TypeError, + pattern); } -expectMultiValueResult(` +expectRunFailure(` (module - (func (export "run") (result i32 i32 i32) - (i32.const 0) + (func (export "run") (result i32 i32) (i32.const 52) - (i32.const 10)))`, [0, 52, 10]); -expectMultiValueResult(` - (module - (func (export "run") (result f32 f32 f32) - (f32.const 0.5) - (f32.const 52.5) - (f32.const 10.5)))`, [0.5, 52.5, 10.5]); -expectMultiValueResult(` - (module - (func (export "run") (result f64 f64 f64) - (f64.const 0.5) - (f64.const 52.5) - (f64.const 10.5)))`, [0.5, 52.5, 10.5]); - -if (wasmBigIntEnabled()) { - expectMultiValueResult(` - (module - (func (export "run") (result i32 i64 i32) - (i32.const 0) - (i64.const 52) - (i32.const 10)))`, [0, 52n, 10]); - expectMultiValueResult(` - (module - (func (export "run") (result i64 i32 i64) - (i64.const 0) - (i32.const 52) - (i64.const 10)))`, [0n, 52, 10n]); - expectMultiValueResult(` - (module - (func (export "run") (result i64 i64 i64) - (i64.const 0) - (i64.const 52) - (i64.const 10)))`, [0n, 52n, 10n]); -} + (i32.const 10)))`, + /calling WebAssembly functions with multiple results from JavaScript not yet implemented/); diff --git a/js/src/jit-test/tests/wasm/multi-value/call-ref.js b/js/src/jit-test/tests/wasm/multi-value/call-ref.js index c79a3e5633e6..b5818ef9b6bf 100644 --- a/js/src/jit-test/tests/wasm/multi-value/call-ref.js +++ b/js/src/jit-test/tests/wasm/multi-value/call-ref.js @@ -12,18 +12,14 @@ function unboxThreeInts(x, y, z) { return [unboxInt(x), unboxInt(y), unboxInt(z)]; } -function testAddNextThreeIntsInner(addNextThreeInts) { - resetCounter(); - for (let n = 0; n < 100000; n += 3) { - assertEq(addNextThreeInts(), n * 3 + 3); - } -} - function testAddNextThreeInts(text, imports) { let i = new WebAssembly.Instance( new WebAssembly.Module(wasmTextToBinary(text)), { imports }); - testAddNextThreeIntsInner(() => i.exports.addNextThreeInts()); + resetCounter(); + for (let n = 0; n < 100000; n += 3) { + assertEq(i.exports.addNextThreeInts(), n * 3 + 3); + } } testAddNextThreeInts(` @@ -66,18 +62,3 @@ testAddNextThreeInts(` i32.add i32.add))`, {boxNextThreeInts, unboxThreeInts}); - -{ - let i = wasmEvalText(` - (module - (func $boxNextThreeInts (import "imports" "boxNextThreeInts") - (result anyref anyref anyref)) - - (func (export "boxNextThreeInts") (result anyref anyref anyref) - call $boxNextThreeInts))`, - {imports: {boxNextThreeInts}}); - testAddNextThreeIntsInner(() => { - let [a, b, c] = i.exports.boxNextThreeInts(); - return unboxInt(a) + unboxInt(b) + unboxInt(c); - }); -} diff --git a/js/src/wasm/WasmCode.h b/js/src/wasm/WasmCode.h index 35d03b21ebce..8a6828125363 100644 --- a/js/src/wasm/WasmCode.h +++ b/js/src/wasm/WasmCode.h @@ -239,7 +239,7 @@ class FuncExport { bool canHaveJitEntry() const { return !funcType_.temporarilyUnsupportedReftypeForEntry() && - !funcType_.temporarilyUnsupportedResultCountForJitEntry() && + !funcType_.temporarilyUnsupportedResultCountForEntry() && JitOptions.enableWasmJitEntry; } diff --git a/js/src/wasm/WasmInstance.cpp b/js/src/wasm/WasmInstance.cpp index 33c6cd9c9417..a04b9d9858b4 100644 --- a/js/src/wasm/WasmInstance.cpp +++ b/js/src/wasm/WasmInstance.cpp @@ -483,7 +483,7 @@ bool Instance::callImport(JSContext* cx, uint32_t funcImportIndex, } // Functions that return multiple values don't have a jit exit at the moment. - if (fi.funcType().temporarilyUnsupportedResultCountForJitExit()) { + if (fi.funcType().temporarilyUnsupportedResultCountForExit()) { return true; } @@ -1933,125 +1933,6 @@ static bool GetInterpEntry(Instance& instance, uint32_t funcIndex, return true; } -class MOZ_RAII ReturnToJSResultCollector { - class MOZ_RAII StackResultsRooter : public JS::CustomAutoRooter { - ReturnToJSResultCollector& collector_; - - public: - StackResultsRooter(JSContext* cx, ReturnToJSResultCollector& collector) - : JS::CustomAutoRooter(cx), collector_(collector) {} - - void trace(JSTracer* trc) final { - for (ABIResultIter iter(collector_.type_); !iter.done(); iter.next()) { - const ABIResult& result = iter.cur(); - if (result.onStack() && result.type().isReference()) { - char* loc = collector_.stackResultsArea_.get() + result.stackOffset(); - JSObject** refLoc = reinterpret_cast(loc); - TraceNullableRoot(trc, refLoc, "StackResultsRooter::trace"); - } - } - } - }; - friend class StackResultsRooter; - - ResultType type_; - UniquePtr stackResultsArea_; - Maybe rooter_; - - public: - explicit ReturnToJSResultCollector(const ResultType& type) : type_(type){}; - bool init(JSContext* cx) { - bool needRooter = false; - ABIResultIter iter(type_); - for (; !iter.done(); iter.next()) { - const ABIResult& result = iter.cur(); - if (result.onStack() && result.type().isReference()) { - needRooter = true; - } - } - uint32_t areaBytes = iter.stackBytesConsumedSoFar(); - MOZ_ASSERT_IF(needRooter, areaBytes > 0); - if (areaBytes > 0) { - // It is necessary to zero storage for ref results, and it doesn't - // hurt to do so for other POD results. - stackResultsArea_ = cx->make_zeroed_pod_array(areaBytes); - if (!stackResultsArea_) { - return false; - } - if (needRooter) { - rooter_.emplace(cx, *this); - } - } - return true; - } - - void* stackResultsArea() { - MOZ_ASSERT(stackResultsArea_); - return stackResultsArea_.get(); - } - - bool collect(JSContext* cx, void* registerResultLoc, - MutableHandleValue rval) { - if (type_.empty()) { - // No results: set to undefined, and we're done. - rval.setUndefined(); - return true; - } - - // Stack results written to stackResultsArea_; register result - // written to registerResultLoc. - - // First, convert the register return value, and prepare to iterate - // in push order. Note that if the register result is a reference - // type, it is unrooted, so ToJSValue_anyref must not GC in that - // case. - ABIResultIter iter(type_); - DebugOnly usedRegisterResult = false; - for (; !iter.done(); iter.next()) { - if (iter.cur().inRegister()) { - MOZ_ASSERT(!usedRegisterResult); - if (!ToJSValue(cx, registerResultLoc, - iter.cur().type(), rval)) { - return false; - } - usedRegisterResult = true; - } - } - MOZ_ASSERT(usedRegisterResult); - - MOZ_ASSERT((stackResultsArea_ != nullptr) == (iter.count() > 1)); - if (!stackResultsArea_) { - // A single result: we're done. - return true; - } - - // Otherwise, collect results in an array, in push order. - Rooted array(cx, NewDenseEmptyArray(cx)); - if (!array) { - return false; - } - RootedValue tmp(cx); - for (iter.switchToPrev(); !iter.done(); iter.prev()) { - const ABIResult& result = iter.cur(); - if (result.onStack()) { - char* loc = stackResultsArea_.get() + result.stackOffset(); - if (!ToJSValue(cx, loc, result.type(), &tmp)) { - return false; - } - if (!NewbornArrayPush(cx, array, tmp)) { - return false; - } - } else { - if (!NewbornArrayPush(cx, array, rval)) { - return false; - } - } - } - rval.set(ObjectValue(*array)); - return true; - } -}; - bool Instance::callExport(JSContext* cx, uint32_t funcIndex, CallArgs args) { if (memory_) { // If there has been a moving grow, this Instance should have been notified. @@ -2070,10 +1951,9 @@ bool Instance::callExport(JSContext* cx, uint32_t funcIndex, CallArgs args) { return false; } - ArgTypeVector argTypes(*funcType); - ResultType resultType(ResultType::Vector(funcType->results())); - ReturnToJSResultCollector results(resultType); - if (!results.init(cx)) { + if (funcType->temporarilyUnsupportedResultCountForEntry()) { + JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, + JSMSG_WASM_MULTIPLE_RESULT_ENTRY_UNIMPLEMENTED); return false; } @@ -2086,25 +1966,20 @@ bool Instance::callExport(JSContext* cx, uint32_t funcIndex, CallArgs args) { // stored in the first element of the array (which, therefore, must have // length >= 1). Vector exportArgs(cx); - if (!exportArgs.resize(std::max(1, argTypes.length()))) { + if (!exportArgs.resize(std::max(1, funcType->args().length()))) { return false; } ASSERT_ANYREF_IS_JSOBJECT; Rooted> refs(cx); - DebugCodegen(DebugChannel::Function, "wasm-function[%d] arguments [", + DebugCodegen(DebugChannel::Function, "wasm-function[%d]; arguments [", funcIndex); RootedValue v(cx); - for (size_t i = 0; i < argTypes.length(); ++i) { + for (size_t i = 0; i < funcType->args().length(); ++i) { + v = i < args.length() ? args[i] : UndefinedValue(); + ValType type = funcType->arg(i); void* rawArgLoc = &exportArgs[i]; - if (argTypes.isSyntheticStackResultPointerArg(i)) { - *reinterpret_cast(rawArgLoc) = results.stackResultsArea(); - continue; - } - size_t naturalIdx = argTypes.naturalIndex(i); - v = naturalIdx < args.length() ? args[naturalIdx] : UndefinedValue(); - ValType type = funcType->arg(naturalIdx); if (!ToWebAssemblyValue(cx, v, type, rawArgLoc)) { return false; } @@ -2139,12 +2014,8 @@ bool Instance::callExport(JSContext* cx, uint32_t funcIndex, CallArgs args) { if (refs.length() > 0) { DebugCodegen(DebugChannel::Function, "; "); size_t nextRef = 0; - for (size_t i = 0; i < argTypes.length(); ++i) { - if (argTypes.isSyntheticStackResultPointerArg(i)) { - continue; - } - size_t naturalIdx = argTypes.naturalIndex(i); - ValType type = funcType->arg(naturalIdx); + for (size_t i = 0; i < funcType->args().length(); ++i) { + ValType type = funcType->arg(i); if (type.isReference()) { void** rawArgLoc = (void**)&exportArgs[i]; *rawArgLoc = refs[nextRef++]; @@ -2186,8 +2057,15 @@ bool Instance::callExport(JSContext* cx, uint32_t funcIndex, CallArgs args) { void* registerResultLoc = &exportArgs[0]; DebugCodegen(DebugChannel::Function, "wasm-function[%d]; results [", funcIndex); - if (!results.collect(cx, registerResultLoc, args.rval())) { - return false; + const ValTypeVector& results = funcType->results(); + if (results.length() == 0) { + args.rval().set(UndefinedValue()); + } else { + MOZ_ASSERT(results.length() == 1, "multi-value return unimplemented"); + if (!ToJSValue(cx, registerResultLoc, results[0], + args.rval())) { + return false; + } } DebugCodegen(DebugChannel::Function, "]\n"); diff --git a/js/src/wasm/WasmStubs.cpp b/js/src/wasm/WasmStubs.cpp index 1b402783d4e0..d2f2382de1b6 100644 --- a/js/src/wasm/WasmStubs.cpp +++ b/js/src/wasm/WasmStubs.cpp @@ -317,7 +317,7 @@ static void SetupABIArguments(MacroAssembler& masm, const FuncExport& fe, masm.loadPtr(src, iter->gpr()); } else if (type == MIRType::StackResults) { MOZ_ASSERT(args.isSyntheticStackResultPointerArg(iter.index())); - masm.loadPtr(src, iter->gpr()); + MOZ_CRASH("multiple function results not yet implemented"); } else { MOZ_CRASH("unknown GPR type"); } @@ -380,9 +380,7 @@ static void SetupABIArguments(MacroAssembler& masm, const FuncExport& fe, } case MIRType::StackResults: { MOZ_ASSERT(args.isSyntheticStackResultPointerArg(iter.index())); - masm.loadPtr(src, scratch); - masm.storePtr(scratch, Address(masm.getStackPointer(), - iter->offsetFromArgBase())); + MOZ_CRASH("multiple function results not yet implemented"); break; } default: @@ -396,37 +394,33 @@ static void SetupABIArguments(MacroAssembler& masm, const FuncExport& fe, } } -static void StoreRegisterResult(MacroAssembler& masm, const FuncExport& fe, - Register loc) { - ResultType results = ResultType::Vector(fe.funcType().results()); - DebugOnly sawRegisterResult = false; - for (ABIResultIter iter(results); !iter.done(); iter.next()) { - const ABIResult& result = iter.cur(); - if (result.inRegister()) { - MOZ_ASSERT(!sawRegisterResult); - sawRegisterResult = true; - switch (result.type().kind()) { - case ValType::I32: - masm.store32(result.gpr(), Address(loc, 0)); - break; - case ValType::I64: - masm.store64(result.gpr64(), Address(loc, 0)); - break; - case ValType::F32: - masm.canonicalizeFloat(result.fpr()); - masm.storeFloat32(result.fpr(), Address(loc, 0)); - break; - case ValType::F64: - masm.canonicalizeDouble(result.fpr()); - masm.storeDouble(result.fpr(), Address(loc, 0)); - break; - case ValType::Ref: - masm.storePtr(result.gpr(), Address(loc, 0)); - break; - } - } +static void StoreABIReturn(MacroAssembler& masm, const FuncExport& fe, + Register argv) { + // Store the return value in argv[0]. + const ValTypeVector& results = fe.funcType().results(); + if (results.length() == 0) { + return; + } + MOZ_ASSERT(results.length() == 1, "multi-value return unimplemented"); + switch (results[0].kind()) { + case ValType::I32: + masm.store32(ReturnReg, Address(argv, 0)); + break; + case ValType::I64: + masm.store64(ReturnReg64, Address(argv, 0)); + break; + case ValType::F32: + masm.canonicalizeFloat(ReturnFloat32Reg); + masm.storeFloat32(ReturnFloat32Reg, Address(argv, 0)); + break; + case ValType::F64: + masm.canonicalizeDouble(ReturnDoubleReg); + masm.storeDouble(ReturnDoubleReg, Address(argv, 0)); + break; + case ValType::Ref: + masm.storePtr(ReturnReg, Address(argv, 0)); + break; } - MOZ_ASSERT(sawRegisterResult == (results.length() > 0)); } #if defined(JS_CODEGEN_ARM) @@ -647,6 +641,12 @@ static bool GenerateInterpEntry(MacroAssembler& masm, const FuncExport& fe, # endif #endif + if (fe.funcType().temporarilyUnsupportedResultCountForEntry()) { + // Unreachable as the Instance::callExport doesn't let us get here. + masm.breakpoint(); + return FinishOffsets(masm, offsets); + } + // Save all caller non-volatile registers before we clobber them here and in // the wasm callee (which does not preserve non-volatile registers). masm.setFramePushed(0); @@ -745,8 +745,8 @@ static bool GenerateInterpEntry(MacroAssembler& masm, const FuncExport& fe, WasmPop(masm, WasmTlsReg); #endif - // Store the register result, if any, in argv[0]. - StoreRegisterResult(masm, fe, argv); + // Store the return value in argv[0]. + StoreABIReturn(masm, fe, argv); // After the ReturnReg is stored into argv[0] but before fp is clobbered by // the PopRegsInMask(NonVolatileRegs) below, set the return value based on @@ -2709,9 +2709,8 @@ bool wasm::GenerateEntryStubs(MacroAssembler& masm, size_t funcExportIndex, return true; } - // Returning multiple values to JS JIT code not yet implemented (see - // bug 1595031). - if (fe.funcType().temporarilyUnsupportedResultCountForJitEntry()) { + // Returning multiple values to JS not yet implemented (see bug 1595031). + if (fe.funcType().temporarilyUnsupportedResultCountForEntry()) { return true; } @@ -2759,9 +2758,9 @@ bool wasm::GenerateStubs(const ModuleEnvironment& env, continue; } - // Exit to JS JIT code returning multiple values not yet implemented - // (see bug 1595031). - if (fi.funcType().temporarilyUnsupportedResultCountForJitExit()) { + // Exit to JS returning multiple values not yet implemented (see bug + // 1595031). + if (fi.funcType().temporarilyUnsupportedResultCountForExit()) { continue; } diff --git a/js/src/wasm/WasmTypes.h b/js/src/wasm/WasmTypes.h index 05d3820e296a..fd1f73fcafae 100644 --- a/js/src/wasm/WasmTypes.h +++ b/js/src/wasm/WasmTypes.h @@ -1088,14 +1088,14 @@ class FuncType { } return false; } - // Entry from JS to wasm via the JIT is currently unimplemented for - // functions that return multiple values. - bool temporarilyUnsupportedResultCountForJitEntry() const { + // Entry from JS to wasm is currently unimplemented for functions that return + // multiple values. + bool temporarilyUnsupportedResultCountForEntry() const { return results().length() > 1; } // Calls out from wasm to JS that return multiple values is currently // unsupported. - bool temporarilyUnsupportedResultCountForJitExit() const { + bool temporarilyUnsupportedResultCountForExit() const { return results().length() > 1; } // For JS->wasm jit entries, AnyRef parameters and returns are allowed, diff --git a/testing/web-platform/meta/wasm/jsapi/constructor/multi-value.any.js.ini b/testing/web-platform/meta/wasm/jsapi/constructor/multi-value.any.js.ini index 1edeb7c53f94..f964de33f579 100644 --- a/testing/web-platform/meta/wasm/jsapi/constructor/multi-value.any.js.ini +++ b/testing/web-platform/meta/wasm/jsapi/constructor/multi-value.any.js.ini @@ -1,21 +1,17 @@ [multi-value.any.html] [multiple return values from wasm to js] - expected: - if not nightly_build: FAIL + expected: FAIL [multiple return values inside wasm] - expected: - if not nightly_build: FAIL + expected: FAIL [multiple return values from js to wasm] - expected: - if not nightly_build: FAIL + expected: FAIL [multi-value.any.js] [multiple return values from wasm to js] - expected: - if not nightly_build: FAIL + expected: FAIL [multiple return values inside wasm] expected: @@ -28,14 +24,11 @@ [multi-value.any.worker.html] [multiple return values from wasm to js] - expected: - if not nightly_build: FAIL + expected: FAIL [multiple return values inside wasm] - expected: - if not nightly_build: FAIL + expected: FAIL [multiple return values from js to wasm] - expected: - if not nightly_build: FAIL + expected: FAIL