Closed Bug 1235631 Opened 5 years ago Closed 5 years ago
Odin: remove change-heap support
Fixup test outside tests/asm.js.
This version also removes ArrayBuffer.transfer: if it's not removed, then there is a significant risk that lingering code compiled with "-s ALLOW_MEMORY_GROWTH=1" will use ArrayBuffer.transfer (only on Nightly) which, with neutering-support removed, would throw. There doesn't appear to be any movement on transfer in TC39 and no intention to implement in other engines, so this seems like the right thing to do at this time. It's easy enough to resurrect later.
Comment on attachment 8702681 [details] [diff] [review] rm-change-heap Review of attachment 8702681 [details] [diff] [review]: ----------------------------------------------------------------- BURN IT WITH FIRE ::: js/src/asmjs/WasmModule.cpp @@ +515,2 @@ > MOZ_ASSERT_IF(heap->is<ArrayBufferObject>(), heap->as<ArrayBufferObject>().isAsmJS()); > + MOZ_ASSERT(!heap_); Wow, the proximity of this assertion and the hasHeap() above seems misleading here. How about keeping the name usesHeap() instead of hasHeap()? or renaming it needsHeap()? ::: js/src/asmjs/WasmModule.h @@ +327,5 @@ > > +// A wasm module can either use no heap, a unshared heap (ArrayBuffer) or shared > +// heap (SharedArrayBuffer). > + > +enum class HeapUse I'd say HeapUsage, but that's probably me not knowing the differences between Use and Usage in english, so trusting your judgement here. ::: js/src/asmjs/WasmStubs.cpp @@ +98,5 @@ > // The signature of the entry point is Module::CodePtr. The exported wasm > // function has an ABI derived from its specific signature, so this function > // must map from the ABI of CodePtr to the export's signature's ABI. > static bool > +GenerateEntry(ModuleGenerator& mg, unsigned exportIndex, HeapUse heapUse) I think you just want a bool hasUse here, not the entire HeapUse? @@ +335,5 @@ > // Generate a stub that is called via the internal ABI derived from the > // signature of the import and calls into an appropriate InvokeImport C++ > // function, having boxed all the ABI arguments into a homogeneous Value array. > static bool > +GenerateInterpExitStub(ModuleGenerator& mg, unsigned importIndex, HeapUse heapUse, heapUse is unused here @@ +440,5 @@ > // Generate a stub that is called via the internal ABI derived from the > // signature of the import and calls into a compatible JIT function, > // having boxed all the ABI arguments into the JIT stack frame layout. > static bool > +GenerateJitExitStub(ModuleGenerator& mg, unsigned importIndex, HeapUse heapUse, You could also just use a boolean hasHeap here. @@ +1072,5 @@ > return mg.defineInlineStub(offsets); > } > > bool > +wasm::GenerateStubs(ModuleGenerator& mg, HeapUse heapUse) and by transitivity, this could use a boolean only as well :)
Attachment #8702681 - Flags: review?(bbouvier+bugzilla) → review+
Agreed on all points, thanks!
You need to log in before you can comment on or make changes to this bug.