From 57de4fdae6538fd8481d7d9aef213a59371bd67b Mon Sep 17 00:00:00 2001 From: Luke Wagner Date: Mon, 12 Sep 2016 14:49:39 -0500 Subject: [PATCH] Bug 1284156 - Baldr: simplify representation of null table elements (r=bbouvier) MozReview-Commit-ID: 1hyFX6CCGqR --- js/src/asmjs/WasmCode.cpp | 1 - js/src/asmjs/WasmCode.h | 2 -- js/src/asmjs/WasmGenerator.cpp | 1 - js/src/asmjs/WasmInstance.cpp | 8 +++-- js/src/asmjs/WasmJS.cpp | 26 ++++------------ js/src/asmjs/WasmModule.cpp | 7 ----- js/src/asmjs/WasmModule.h | 1 - js/src/asmjs/WasmTable.cpp | 42 +++++++------------------- js/src/asmjs/WasmTable.h | 14 ++------- js/src/jit-test/tests/wasm/table-gc.js | 12 +++----- js/src/jit/MacroAssembler.cpp | 3 ++ 11 files changed, 33 insertions(+), 84 deletions(-) diff --git a/js/src/asmjs/WasmCode.cpp b/js/src/asmjs/WasmCode.cpp index 44278c09f3e0..a0f5162035a7 100644 --- a/js/src/asmjs/WasmCode.cpp +++ b/js/src/asmjs/WasmCode.cpp @@ -231,7 +231,6 @@ CodeSegment::create(JSContext* cx, cs->interruptCode_ = codeBase + linkData.interruptOffset; cs->outOfBoundsCode_ = codeBase + linkData.outOfBoundsOffset; cs->unalignedAccessCode_ = codeBase + linkData.unalignedAccessOffset; - cs->badIndirectCallCode_ = codeBase + linkData.badIndirectCallOffset; { JitContext jcx(CompileRuntime::get(cx->compartment()->runtimeFromAnyThread())); diff --git a/js/src/asmjs/WasmCode.h b/js/src/asmjs/WasmCode.h index 563c953d3998..3069c36d6c4b 100644 --- a/js/src/asmjs/WasmCode.h +++ b/js/src/asmjs/WasmCode.h @@ -54,7 +54,6 @@ class CodeSegment // These are pointers into code for stubs used for asynchronous // signal-handler control-flow transfer. uint8_t* interruptCode_; - uint8_t* badIndirectCallCode_; uint8_t* outOfBoundsCode_; uint8_t* unalignedAccessCode_; @@ -84,7 +83,6 @@ class CodeSegment uint32_t totalLength() const { return codeLength_ + globalDataLength_; } uint8_t* interruptCode() const { return interruptCode_; } - uint8_t* badIndirectCallCode() const { return badIndirectCallCode_; } uint8_t* outOfBoundsCode() const { return outOfBoundsCode_; } uint8_t* unalignedAccessCode() const { return unalignedAccessCode_; } diff --git a/js/src/asmjs/WasmGenerator.cpp b/js/src/asmjs/WasmGenerator.cpp index ab740f157330..f199f1a76af7 100644 --- a/js/src/asmjs/WasmGenerator.cpp +++ b/js/src/asmjs/WasmGenerator.cpp @@ -508,7 +508,6 @@ ModuleGenerator::finishCodegen() linkData_.interruptOffset = interruptExit.begin; linkData_.outOfBoundsOffset = jumpTargets[JumpTarget::OutOfBounds].begin; linkData_.unalignedAccessOffset = jumpTargets[JumpTarget::UnalignedAccess].begin; - linkData_.badIndirectCallOffset = jumpTargets[JumpTarget::BadIndirectCall].begin; // Only call convertOutOfRangeBranchesToThunks after all other codegen that may // emit new jumps to JumpTargets has finished. diff --git a/js/src/asmjs/WasmInstance.cpp b/js/src/asmjs/WasmInstance.cpp index c16b8e999a66..b56b53f0c0f7 100644 --- a/js/src/asmjs/WasmInstance.cpp +++ b/js/src/asmjs/WasmInstance.cpp @@ -843,7 +843,7 @@ Instance::ensureProfilingState(JSContext* cx, bool newProfilingEnabled) } for (const SharedTable& table : tables_) { - if (!table->isTypedFunction() || !table->initialized()) + if (!table->isTypedFunction()) continue; // This logic will have to be generalized to match the import logic @@ -853,8 +853,10 @@ Instance::ensureProfilingState(JSContext* cx, bool newProfilingEnabled) void** array = table->internalArray(); uint32_t length = table->length(); - for (size_t i = 0; i < length; i++) - UpdateEntry(*code_, newProfilingEnabled, &array[i]); + for (size_t i = 0; i < length; i++) { + if (array[i]) + UpdateEntry(*code_, newProfilingEnabled, &array[i]); + } } return true; diff --git a/js/src/asmjs/WasmJS.cpp b/js/src/asmjs/WasmJS.cpp index 85e944d16677..68c0ba045586 100644 --- a/js/src/asmjs/WasmJS.cpp +++ b/js/src/asmjs/WasmJS.cpp @@ -1200,24 +1200,19 @@ WasmTableObject::getImpl(JSContext* cx, const CallArgs& args) uint32_t index = uint32_t(indexDbl); MOZ_ASSERT(double(index) == indexDbl); - if (!table.initialized()) { - args.rval().setNull(); - return true; - } - ExternalTableElem& elem = table.externalArray()[index]; - Instance& instance = *elem.tls->instance; - const CodeRange* codeRange = instance.code().lookupRange(elem.code); - - // A non-function code range means the bad-indirect-call stub, so a null element. - if (!codeRange || !codeRange->isFunction()) { + if (!elem.code) { args.rval().setNull(); return true; } + Instance& instance = *elem.tls->instance; + const CodeRange& codeRange = *instance.code().lookupRange(elem.code); + MOZ_ASSERT(codeRange.isFunction()); + RootedWasmInstanceObject instanceObj(cx, instance.object()); RootedFunction fun(cx); - if (!instanceObj->getExportedFunction(cx, instanceObj, codeRange->funcDefIndex(), &fun)) + if (!instanceObj->getExportedFunction(cx, instanceObj, codeRange.funcDefIndex(), &fun)) return false; args.rval().setObject(*fun); @@ -1258,15 +1253,6 @@ WasmTableObject::setImpl(JSContext* cx, const CallArgs& args) return false; } - if (!table.initialized()) { - if (!value) { - args.rval().setUndefined(); - return true; - } - - table.init(ExportedFunctionToInstance(value)); - } - if (value) { RootedWasmInstanceObject instanceObj(cx, ExportedFunctionToInstanceObject(value)); uint32_t funcDefIndex = ExportedFunctionToDefinitionIndex(value); diff --git a/js/src/asmjs/WasmModule.cpp b/js/src/asmjs/WasmModule.cpp index 1d63b810b3f4..61c48ba0d596 100644 --- a/js/src/asmjs/WasmModule.cpp +++ b/js/src/asmjs/WasmModule.cpp @@ -462,13 +462,6 @@ Module::initSegments(JSContext* cx, MOZ_ASSERT(dataSegments_.empty()); } - // Ensure all tables are initialized before storing into them. - - for (const SharedTable& table : tables) { - if (!table->initialized()) - table->init(instance); - } - // Now that initialization can't fail partway through, write data/elem // segments into memories/tables. diff --git a/js/src/asmjs/WasmModule.h b/js/src/asmjs/WasmModule.h index 07881ab57abe..a97264c38bcc 100644 --- a/js/src/asmjs/WasmModule.h +++ b/js/src/asmjs/WasmModule.h @@ -51,7 +51,6 @@ struct LinkDataCacheablePod uint32_t interruptOffset; uint32_t outOfBoundsOffset; uint32_t unalignedAccessOffset; - uint32_t badIndirectCallOffset; LinkDataCacheablePod() { mozilla::PodZero(this); } }; diff --git a/js/src/asmjs/WasmTable.cpp b/js/src/asmjs/WasmTable.cpp index 5689bb68e366..b90a06f49378 100644 --- a/js/src/asmjs/WasmTable.cpp +++ b/js/src/asmjs/WasmTable.cpp @@ -48,32 +48,10 @@ Table::create(JSContext* cx, const TableDesc& desc, HandleWasmTableObject maybeO table->array_.reset((uint8_t*)array); table->kind_ = desc.kind; table->length_ = desc.initial; - table->initialized_ = false; table->external_ = desc.external; return table; } -void -Table::init(Instance& instance) -{ - MOZ_ASSERT(!initialized()); - initialized_ = true; - - void* code = instance.codeSegment().badIndirectCallCode(); - if (external_) { - ExternalTableElem* array = externalArray(); - TlsData* tls = &instance.tlsData(); - for (uint32_t i = 0; i < length_; i++) { - array[i].code = code; - array[i].tls = tls; - } - } else { - void** array = internalArray(); - for (uint32_t i = 0; i < length_; i++) - array[i] = code; - } -} - void Table::tracePrivate(JSTracer* trc) { @@ -87,12 +65,15 @@ Table::tracePrivate(JSTracer* trc) TraceEdge(trc, &maybeObject_, "wasm table object"); } - if (!initialized_ || !external_) - return; - - ExternalTableElem* array = externalArray(); - for (uint32_t i = 0; i < length_; i++) - array[i].tls->instance->trace(trc); + if (external_) { + ExternalTableElem* array = externalArray(); + for (uint32_t i = 0; i < length_; i++) { + if (array[i].tls) + array[i].tls->instance->trace(trc); + else + MOZ_ASSERT(!array[i].code); + } + } } void @@ -112,7 +93,6 @@ Table::trace(JSTracer* trc) void** Table::internalArray() const { - MOZ_ASSERT(initialized_); MOZ_ASSERT(!external_); return (void**)array_.get(); } @@ -120,7 +100,6 @@ Table::internalArray() const ExternalTableElem* Table::externalArray() const { - MOZ_ASSERT(initialized_); MOZ_ASSERT(external_); return (ExternalTableElem*)array_.get(); } @@ -142,7 +121,8 @@ Table::setNull(uint32_t index) { // Only external tables can set elements to null after initialization. ExternalTableElem& elem = externalArray()[index]; - elem.code = elem.tls->instance->codeSegment().badIndirectCallCode(); + elem.code = nullptr; + elem.tls = nullptr; } size_t diff --git a/js/src/asmjs/WasmTable.h b/js/src/asmjs/WasmTable.h index 7272c0a62235..848387d37240 100644 --- a/js/src/asmjs/WasmTable.h +++ b/js/src/asmjs/WasmTable.h @@ -36,7 +36,6 @@ class Table : public ShareableBase UniqueByteArray array_; TableKind kind_; uint32_t length_; - bool initialized_; bool external_; void tracePrivate(JSTracer* trc); @@ -47,21 +46,14 @@ class Table : public ShareableBase
HandleWasmTableObject maybeObject); void trace(JSTracer* trc); - // These accessors may be used before initialization. - bool external() const { return external_; } bool isTypedFunction() const { return kind_ == TableKind::TypedFunction; } uint32_t length() const { return length_; } uint8_t* base() const { return array_.get(); } - // A Table must be initialized before any dependent instance can execute. - - bool initialized() const { return initialized_; } - void init(Instance& instance); - - // After initialization, elements must be accessed. All updates must go - // through a set() function with the exception of (profiling) updates to the - // callee pointer that do not change which logical function is being called. + // All updates must go through a set() function with the exception of + // (profiling) updates to the callee pointer that do not change which + // logical function is being called. void** internalArray() const; ExternalTableElem* externalArray() const; diff --git a/js/src/jit-test/tests/wasm/table-gc.js b/js/src/jit-test/tests/wasm/table-gc.js index 12c34cd1ccea..0cd9344913c3 100644 --- a/js/src/jit-test/tests/wasm/table-gc.js +++ b/js/src/jit-test/tests/wasm/table-gc.js @@ -79,8 +79,7 @@ t = null; gc(); assertEq(finalizeCount(), 4); -// The bad-indirect-call stub should (currently, could be changed later) keep -// the instance containing that stub alive. +// Null elements shouldn't keep anything alive. resetFinalizeCount(); var i = evalText(`(module (table (resizable 2)) (export "tbl" table) ${caller})`); var e = i.exports; @@ -96,7 +95,7 @@ gc(); assertEq(finalizeCount(), 1); i = null; gc(); -assertEq(finalizeCount(), 1); +assertEq(finalizeCount(), 2); t = null; gc(); assertEq(finalizeCount(), 3); @@ -144,14 +143,13 @@ assertEq(finalizeCount(), 2); t.set(0, null); assertEq(t.get(0), null); gc(); -assertEq(finalizeCount(), 2); +assertEq(finalizeCount(), 3); t = null; gc(); assertEq(finalizeCount(), 4); -// Once all of an instance's elements in a Table (including the -// bad-indirect-call stub) have been clobbered, the Instance should not be -// rooted. +// Once all of an instance's elements in a Table have been clobbered, the +// Instance should not be reachable. resetFinalizeCount(); var i1 = evalText(`(module (func $f1 (result i32) (i32.const 13)) (export "f1" $f1))`); var i2 = evalText(`(module (func $f2 (result i32) (i32.const 42)) (export "f2" $f2))`); diff --git a/js/src/jit/MacroAssembler.cpp b/js/src/jit/MacroAssembler.cpp index 2b3218fd8ee5..85666e0de8ae 100644 --- a/js/src/jit/MacroAssembler.cpp +++ b/js/src/jit/MacroAssembler.cpp @@ -2818,11 +2818,14 @@ MacroAssembler::wasmCallIndirect(const wasm::CallSiteDesc& desc, const wasm::Cal } loadPtr(Address(scratch, offsetof(wasm::ExternalTableElem, tls)), WasmTlsReg); + branchTest32(Assembler::Zero, WasmTlsReg, WasmTlsReg, wasm::JumpTarget::BadIndirectCall); + loadWasmPinnedRegsFromTls(); loadPtr(Address(scratch, offsetof(wasm::ExternalTableElem, code)), scratch); } else { loadPtr(BaseIndex(scratch, index, ScalePointer), scratch); + branchTest32(Assembler::Zero, scratch, scratch, wasm::JumpTarget::BadIndirectCall); } call(desc, scratch);