1125561 - Align16: Align asm.js fast FFI calls into Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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)

Comment 2

[Luke Wagner [:luke]]

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.

Comment 3

[Nicolas B. Pierron [:nbp]]

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.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Yes, will take.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision a6f037b538ed).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: asm-align-ion.patch

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

Comment 8

[Luke Wagner [:luke]]

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?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(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).

Comment 10

[Nicolas B. Pierron [:nbp]]

(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.

Comment 11

[Luke Wagner [:luke]]

And, just so I understand, did we have word-alignment before which is why we didn't have to worry about this?

Comment 12

[Nicolas B. Pierron [:nbp]]

(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.

Comment 13

[Luke Wagner [:luke]]

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.

Comment 14

[Luke Wagner [:luke]]

Attachment: fix-ion-ffi

Ahh, this is even simpler than it was to begin with :)

Comment 15

[Luke Wagner [:luke]]

Attachment: fix-ion-ffi

Tweaks

Comment 16

[Luke Wagner [:luke]]

Attachment: fix-ion-ffi

Now with tests that hit the real coerce path as well as profiler paths (which were missing).

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 18

[Luke Wagner [:luke]]

(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.

Comment 19

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce28246c736f

Comment 20

[Luke Wagner [:luke]]

Followup to fix warning:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84877a7e0333

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce28246c736f
https://hg.mozilla.org/mozilla-central/rev/84877a7e0333