Odin: remove change-heap support

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Comment 1

3 years ago
Created attachment 8702672 [details] [diff] [review]
rm-change-heap
Attachment #8702672 - Flags: review?(benj)
(Assignee)

Comment 2

3 years ago
Created attachment 8702675 [details] [diff] [review]
rm-change-heap

Fixup test outside tests/asm.js.
Attachment #8702672 - Attachment is obsolete: true
Attachment #8702672 - Flags: review?(benj)
Attachment #8702675 - Flags: review?(benj)
(Assignee)

Comment 3

3 years ago
Created attachment 8702681 [details] [diff] [review]
rm-change-heap

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.
Attachment #8702675 - Attachment is obsolete: true
Attachment #8702675 - Flags: review?(benj)
Attachment #8702681 - Flags: review?(benj)
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+
(Assignee)

Comment 5

3 years ago
Agreed on all points, thanks!

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7a9bcc4b922
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.