Closed Bug 1125561 Opened 7 years ago Closed 7 years ago

Align16: Align asm.js fast FFI calls into Ion

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: gkw, Assigned: luke)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update,testComment=6,origRev=a6f037b538ed])

Attachments

(3 files, 2 obsolete files)

function x() {
    assertValidJitStack()
}
g = (function(stdlib, foreign, heap) {
    "use asm";
    var ff = foreign.ff;
    function f() {
        ff() | 0
    }
    return f
})(this, {
    ff: x
}, new ArrayBuffer(4096));
g()
g()

asserts js debug shell on m-c changeset 7148aa99ad67 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: reinterpret_cast<size_t>(frames.fp()) % JitStackAlignment == 0 (The entry frame should be properly aligned), at jit/JitFrames.cpp

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150119034559" and the hash "350013f91dd2".
The "bad" changeset has the timestamp "20150119053059" and the hash "bfa43f46d24c".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=350013f91dd2&tochange=bfa43f46d24c

Nicolas, is bug 1112159 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
Attached file stack
(lldb) bt
* thread #1: tid = 0x236fd, 0x0000000100388541 js-dbg-opt-64-dm-nsprBuild-darwin-7148aa99ad67`js::jit::AssertValidJitStack(cx=<unavailable>) + 417 at JitFrames.cpp:3038, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100388541 js-dbg-opt-64-dm-nsprBuild-darwin-7148aa99ad67`js::jit::AssertValidJitStack(cx=<unavailable>) + 417 at JitFrames.cpp:3038
    frame #1: 0x00000001000f79fe js-dbg-opt-64-dm-nsprBuild-darwin-7148aa99ad67`TestingFunc_assertValidJitStack(cx=<unavailable>, argc=<unavailable>, vp=0x00007fff5fbfde10) + 14 at TestingFunctions.cpp:1360
    frame #2: 0x0000000101fe3bf9
(lldb)
See GenerateFFIIonExit for the asm.js->Ion call trampoline.  I'm surprised there's a problem, though; this function aligns to AsmJSStackAlignment which is 16, the same as JitStackAlignment.  But perhaps there's a difference between the sp at the Ion call and whatever frames.fp() is returning.
Yes, this is likely a regressor. I guess this might be related to the fact that asm.js is creating a different Entry frame[1] for entering Jitted code, and this cause this assertion to be raised.

Benjamin, do you want to take this issue?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/AsmJSValidate.cpp#8436

(In reply to Luke Wagner [:luke] from comment #2)
> See GenerateFFIIonExit for the asm.js->Ion call trampoline.  I'm surprised
> there's a problem, though; this function aligns to AsmJSStackAlignment which
> is 16, the same as JitStackAlignment.  But perhaps there's a difference
> between the sp at the Ion call and whatever frames.fp() is returning.

The alignment is of the same size, but not the same offset.
The ABI aligns on the frame pointer, but the Jit is aligned on the end of the Jit frame.
Flags: needinfo?(nicolas.b.pierron) → needinfo?(benj)
Yes, will take.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision a6f037b538ed).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
function x() {
    assertJitStackInvariants()
}
g = (function(stdlib, foreign, heap) {
    "use asm";
    var ff = foreign.ff;
    function f() {
        ff() | 0
    }
    return f
})(this, {
    ff: x
}, new ArrayBuffer(4096));
g()
g()

asserts js debug shell on m-c changeset a6f037b538ed with --fuzzing-safe --no-threads --ion-eager at Assertion failure: reinterpret_cast<size_t>(frames.fp()) % JitStackAlignment == 0 (The entry frame should be properly aligned), at jit/JitFrames.cpp

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:update,testComment=6,origRev=a6f037b538ed]
Now Ion, as Odin, wants its entry frames to be aligned on 16 bytes boundaries, because of SIMD.  So when we're calling from Odin to Ion, we need to ensure that SP (+maybe the return address, which may be "naturally" pushed on x86 or x64, and which isn't on ARM and MIPS) is aligned on JitStackAlignment at calls.  As the stack space is used both for calls into Ion or into the coercion paths, we need to adjust the maybe-natural-return-address offset on the coercion paths.

Tested locally on linux x86, linux x64 and the arm simulator, all tests pass.  Need to check out the weirdos *AHEM win32 AHEM* on tbpl though... https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf1644d97d32
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Attachment #8560027 - Flags: review?(luke)
Blocks: 1112158
Summary: Assertion failure: reinterpret_cast<size_t>(frames.fp()) % JitStackAlignment == 0 (The entry frame should be properly aligned), at jit/JitFrames.cpp → Align16: Align asm.js fast FFI calls into Ion
Wait, so when calling Ion, Ion expects sp to be aligned *after* the return address has been pushed?  As in, on entry to an Ion function, %sp % 16 == 0?  What about ARM, which pushes lr as the first insn?
(In reply to Luke Wagner [:luke] from comment #8)
> Wait, so when calling Ion, Ion expects sp to be aligned *after* the return
> address has been pushed? As in, on entry to an Ion function, %sp % 16 == 0?
Yes. See also for instance
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x64/Trampoline-x64.cpp#266-271

> What about ARM, which pushes lr as the first insn?
Not sure what you're referring to: if you're talking about the profiling prologue, that's a distinct issue, as we're taking into account the return address at the call into Ion, not the call into the odin-to-ion trampoline.  For the matter of CodeGenerator-arm which pushes it, this offset isn't taken into account (as MaybeRetAddr is sizeof(void*) on ARM, MaybeNaturalRetAddr is 0 on ARM).
(In reply to Luke Wagner [:luke] from comment #8)
> Wait, so when calling Ion, Ion expects sp to be aligned *after* the return
> address has been pushed?  As in, on entry to an Ion function, %sp % 16 == 0?

Yes, as we do not have a frame pointer, we are going to push things straight after the return address.

> What about ARM, which pushes lr as the first insn?

This is the same principle, we want the stack to be aligned after the push of the link register, even if the link register is pushed by the callee.
And, just so I understand, did we have word-alignment before which is why we didn't have to worry about this?
(In reply to Luke Wagner [:luke] from comment #11)
> And, just so I understand, did we have word-alignment before which is why we
> didn't have to worry about this?

Before that, the Jit did not had any restriction on the alignment, so yes, we had a word alignment.
So looking at the patch, there's a lot of confusing stuff having to do with MaybeRetAddr/MaybeNaturalRetAddr and what's funny is that none of it is no longer necessary since ARM now deals with retaddr in the sane way (callee pushes).  Fixing this, I ended up finding some simplifications for this patch, so I'll post those and see what you think.
Attached patch fix-ion-ffi (obsolete) — Splinter Review
Ahh, this is even simpler than it was to begin with :)
Assignee: benj → luke
Attachment #8560817 - Flags: review?(benj)
Attachment #8560817 - Flags: review?(benj)
Attached patch fix-ion-ffi (obsolete) — Splinter Review
Tweaks
Attachment #8560817 - Attachment is obsolete: true
Attachment #8560819 - Flags: review?(benj)
Attachment #8560027 - Flags: review?(luke)
Attached patch fix-ion-ffiSplinter Review
Now with tests that hit the real coerce path as well as profiler paths (which were missing).
Attachment #8560819 - Attachment is obsolete: true
Attachment #8560819 - Flags: review?(benj)
Attachment #8560822 - Flags: review?(benj)
Comment on attachment 8560822 [details] [diff] [review]
fix-ion-ffi

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

Indeed simpler, thanks!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +8452,5 @@
> +    //   | retaddr | descriptor | callee | argc | this | arg1..N |
> +    // After the Ion frame, the global register (if present) is saved since Ion
> +    // does not preserve non-volatile regs. Also, unlike most ABIs, Ion requires
> +    // that sp be JitStackAlignment-aligned *after* pushing the return address.
> +    static_assert(AsmJSStackAlignment >= JitStackAlignment, "subsumes");

not only it is >=, but also lhs % rhs needs to be 0.  Not mandatory as it is checked at several other locations, but nice to mention.

@@ +8637,4 @@
>  #endif
>  
> +    // As explained above, the frame was aligned for Ion such that
> +    //   sp + sizeof(void*) % JitStackAlignment == 0

nit: my operators precedence table tells me this comment lacks parenthesis

@@ +8638,5 @@
>  
> +    // As explained above, the frame was aligned for Ion such that
> +    //   sp + sizeof(void*) % JitStackAlignment == 0
> +    // But now we possibly want to call one of several different C++ functions,
> +    // so sutract the sizeof(void*) so that sp is properly aligned.

nit: subtract

@@ +8640,5 @@
> +    //   sp + sizeof(void*) % JitStackAlignment == 0
> +    // But now we possibly want to call one of several different C++ functions,
> +    // so sutract the sizeof(void*) so that sp is properly aligned.
> +    static_assert(ABIStackAlignment <= JitStackAlignment, "subsumes");
> +    masm.reserveStack(sizeOfRetAddr);

I guess this happens here to factor out the different paths which would need the adjustment (heap detachment check, oolConvert).

::: js/src/jit-test/tests/asm.js/testBug1125561.js
@@ +15,5 @@
> +    return f
> +`);
> +var f = asmLink(m, null, {ffi:ffi1});
> +f();
> +f();

You seem to be assuming --ion-eager here. If so, setting the ion.warmup.trigger to 10 above is confusing (not sure it is testing anything, at the moment).

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1322,5 @@
>  
>      // Makes a call using the only two methods that it is sane for
>      // independent code to make a call.
>      void callJit(Register callee);
> +    void callJitFromAsmJS(Register callee) { as_blx(callee); }

Can you fix it on MIPS as well, please?
Attachment #8560822 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> not only it is >=, but also lhs % rhs needs to be 0.  Not mandatory as it is
> checked at several other locations, but nice to mention.

Heh, I considered adding this, but then I thought that all natural alignments I've ever heard of are powers of 2 (what else would they be?) in which case this assertion is sufficient.

> I guess this happens here to factor out the different paths which would need
> the adjustment (heap detachment check, oolConvert).

That's right.  Conceptually, this really just breaks the function into two section: the first is Ion-aligned and the second is ABI-aligned.  (If Ion would be ABI-aligned this wouldn't be necessary...)

> You seem to be assuming --ion-eager here. If so, setting the
> ion.warmup.trigger to 10 above is confusing (not sure it is testing
> anything, at the moment).

Oops, good catch; I copypasta'd that and I should've set it to 0.
https://hg.mozilla.org/mozilla-central/rev/ce28246c736f
https://hg.mozilla.org/mozilla-central/rev/84877a7e0333
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.