Closed Bug 1081277 Opened 5 years ago Closed 5 years ago

OdinMonkey: loosen detachment limitations

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(3 files, 3 obsolete files)

To land an experimental implementation of ArrayBuffer.transfer (#ifdef NIGHTLY for now), we need to support detaching an ArrayBuffer linked to an asm.js module while that asm.js module is on the stack (since this is the primary use case via malloc).
Attached patch maintain-listSplinter Review
This small patch maintains a list of live AsmJSModules so that they can be traversed in a later patch.
Attachment #8503359 - Flags: review?(benj)
Attached patch handle-detach-in-asmjs (obsolete) — Splinter Review
This patch moves the current "throw if trying to detach while asm.js is running" logic into js/src/asmjs and changes it to use the list-of-modules in the last patch.  What happens in js/src/asmjs is about to change in the next patch.
Attachment #8503371 - Flags: review?(sphink)
Attachment #8503371 - Flags: review?(benj)
Attached patch handle-detach-in-asmjs (obsolete) — Splinter Review
Oops, this is the patch.
Attachment #8503371 - Attachment is obsolete: true
Attachment #8503371 - Flags: review?(sphink)
Attachment #8503371 - Flags: review?(benj)
Attachment #8503374 - Flags: review?(sphink)
Attachment #8503374 - Flags: review?(benj)
Attached patch delay-neuter-error (obsolete) — Splinter Review
This patch delays throwing until returning from the FFI.  This gives the FFI the change to switch heaps (via change-heap) and avoid an error at all (which is what will happen in the case of ArrayBuffer.transfer: transfer will detach the original heap, hand the contenst to a new heap, and change-heap to that).  This still isn't exactly correct (the exception should be delayed until an actual heap access is attempted, but it's strictly better than the current state and a ton less work, so I'd like to punt on doing the fully correct thing a bit longer).
Attachment #8503377 - Flags: review?(benj)
Comment on attachment 8503359 [details] [diff] [review]
maintain-list

Review of attachment 8503359 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Runtime.h
@@ +1083,5 @@
>      /* AsmJSCache callbacks are runtime-wide. */
> +    JS::AsmJSCacheOps   asmJSCacheOps;
> +
> +    /* Head of the linked list of linked asm.js modules. */
> +    js::AsmJSModule    *linkedAsmJSModules;

nit: could you please initialize it in Runtime.cpp, to avoid ASAN's fury and random behavior?
Attachment #8503359 - Flags: review?(benj) → review+
Rebase with some tweaks (JSMSG_OUT_OF_MEMORY) and better comments.
Attachment #8503374 - Attachment is obsolete: true
Attachment #8503374 - Flags: review?(sphink)
Attachment #8503374 - Flags: review?(benj)
Attachment #8504227 - Flags: review?(sphink)
Attachment #8504227 - Flags: review?(benj)
Rebase, better comments, and JSMSG_OUT_OF_MEMORY.
Attachment #8503377 - Attachment is obsolete: true
Attachment #8503377 - Flags: review?(benj)
Attachment #8504229 - Flags: review?(benj)
Comment on attachment 8504227 [details] [diff] [review]
handle-detach-in-asmjs

Review of attachment 8504227 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/asmjs/AsmJSModule.cpp
@@ +868,5 @@
>  
>      restoreHeapToInitialState(maybePrevBuffer);
>  }
>  
> +bool

(this could be void, but i guess the next patch makes it actually fallible...)
Attachment #8504227 - Flags: review?(benj) → review+
Comment on attachment 8504229 [details] [diff] [review]
delay-neuter-error

Review of attachment 8504229 [details] [diff] [review]:
-----------------------------------------------------------------

Simple and clear. r+ if you delete the changes made in AsmJSFrameIterator*, otherwise there seems to be missing something in the detachHeap exit (or there is some magic happening which i am not seeing).

::: js/src/asmjs/AsmJSFrameIterator.h
@@ +71,5 @@
>          Reason_None,
>          Reason_IonFFI,
>          Reason_SlowFFI,
>          Reason_Interrupt,
> +        Reason_OnDetached,

Unless I am missing something, this is unused?

::: js/src/asmjs/AsmJSModule.cpp
@@ +908,5 @@
>  js::OnDetachAsmJSArrayBuffer(JSContext *cx, Handle<ArrayBufferObject*> buffer)
>  {
>      for (AsmJSModule *m = cx->runtime()->linkedAsmJSModules; m; m = m->nextLinked()) {
> +        if (m->maybeHeapBufferObject() && !m->detachHeap(cx))
> +            return false;

This will silently fail. Could we have an error message at this point? The OOM message isn't the right one anymore, though.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +7641,5 @@
> +// call change-heap to another heap (viz., the new heap returned by transfer)
> +// before returning to asm.js code. If the application fails to do this (if the
> +// heap pointer is null), jump to a stub to handle this.
> +static void
> +CheckForHeapDetachment(ModuleCompiler &m, Register scratch)

The Check* prefix in this file often refers to type checking, could this be GenerateCheckForHeapDetachment, or something that makes it clearer that it generates code? That would be symmetric with GenerateOnDetachedLabelExit

@@ +7651,5 @@
> +    AssertStackAlignment(masm, ABIStackAlignment);
> +#if defined(JS_CODEGEN_X86)
> +    CodeOffsetLabel label = masm.movlWithPatch(PatchedAbsoluteAddress(), scratch);
> +    masm.append(AsmJSGlobalAccess(label, AsmJSHeapGlobalDataOffset));
> +    masm.branchTestPtr(Assembler::Equal, scratch, scratch, &m.onDetachedLabel());

here and below, please use Assembler::Zero instead of Assembler::Equal. They're the same on x86 and x64 at least, but testing zero seems to make more sense in this case.

::: js/src/jit-test/tests/asm.js/testNeuter.js
@@ +75,5 @@
> +new Int32Array(buf3)[13] = 1024;
> +new Int32Array(buf4)[13] = 1337;
> +
> +function ffi2() { neuter(buf1, "change-data"); assertEq(changeHeap(buf2), true); }
> +var {changeHeap, get:get2, inner} = asmLink(m, this, {ffi:ffi2}, buf1);

destructured assignments with renaming? w00t

::: js/src/jit-test/tests/asm.js/testProfiling.js
@@ +129,5 @@
> +var buf = new ArrayBuffer(BUF_CHANGE_MIN);
> +var ffi = function() { neuter(buf, 'change-data') }
> +var f = asmLink(asmCompile('g','ffis','buf', USE_ASM + 'var ffi = ffis.ffi; var i32 = new g.Int32Array(buf); function f() { ffi() } return f'), this, {ffi:ffi}, buf);
> +enableSingleStepProfiling();
> +assertThrowsInstanceOf(()=>f(), TypeError);

it seems it could just be assertThrowsInstanceOf(f, TypeError);
Attachment #8504229 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
All great points, thanks.
Comment on attachment 8504227 [details] [diff] [review]
handle-detach-in-asmjs

Review of attachment 8504227 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ArrayBufferObject.cpp
@@ +697,5 @@
>          buffer->setOwnsData(DoesntOwnData);
> +        if (!ArrayBufferObject::neuter(cx, buffer, newContents)) {
> +            js_free(newContents.data());
> +            return BufferContents::createUnowned(nullptr);
> +        }

I think we can finally get rid of the uncommitted backing buffer now, but I guess it should wait for the current round of fixes first.
Attachment #8504227 - Flags: review?(sphink) → review+
You need to log in before you can comment on or make changes to this bug.