Bug 129045 – [Win][LLINT] Incorrect stack alignment.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

LLINT expects the stack to be aligned on 16 byte boundaries, but MSVC only aligns the stack on 4 byte boundaries, it seems.

Created attachment 224637 [details]
Patch

[Brent Fulgham]

Comment on attachment 224637 [details]
Patch

Looks good to me! I think Mark should do the full review.

[Michael Saboff]

Comment on attachment 224637 [details]
Patch

Looks great.  r=me.

(In reply to comment #3)
> (From update of attachment 224637 [details])
> Looks great.  r=me.

Thanks for looking into this, guys :)

[Geoffrey Garen]

Windows is not the only platform on which the incoming stack is unaligned. For example, the same is true on ARM32. Can we share some of this code? It seems like aligning the stack is a generic action in the LLInt. We need to avoid adding too much Windows-specific cruft to the LLInt.

[Mark Lam]

Comment on attachment 224637 [details]
Patch

I don’t see the code to turn this code on in the build.  It looks like it will still be using the C loop LLINT.  Will you create another patch to turn this on?  Thanks.

(In reply to comment #6)
> (From update of attachment 224637 [details])
> I don’t see the code to turn this code on in the build.  It looks like it will still be using the C loop LLINT.  Will you create another patch to turn this on?  Thanks.

Yes you're right, it's still not turned on.

Things seem to be running fine now with "normal" browsing, I'm not getting the stack alignment asserts, and the crashes I had was related to the double floating point type operations, which is fixed in this patch.

However, there are alot of failures on the JSC stress tests.
Should we look into those first, or should I post a patch to enable it, and look into the stress tests afterwards?

[Mark Lam]

(In reply to comment #7)
> However, there are alot of failures on the JSC stress tests.
> Should we look into those first, or should I post a patch to enable it, and look into the stress tests afterwards?

Do those tests fail with the C Loop LLINT?  If turning on the ASM LLINT and JIT does not introduce regressions, then yes, let’s turn it on.

[WebKit Commit Bot]

Comment on attachment 224637 [details]
Patch

Clearing flags on attachment: 224637

Committed r164374: <http://trac.webkit.org/changeset/164374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #8)
> (In reply to comment #7)
> > However, there are alot of failures on the JSC stress tests.
> > Should we look into those first, or should I post a patch to enable it, and look into the stress tests afterwards?
> 
> Do those tests fail with the C Loop LLINT?  If turning on the ASM LLINT and JIT does not introduce regressions, then yes, let’s turn it on.

Ok, sounds good, I will test that, and create a new patch if there are no regressions.

(In reply to comment #5)
> Windows is not the only platform on which the incoming stack is unaligned. For example, the same is true on ARM32. Can we share some of this code? It seems like aligning the stack is a generic action in the LLInt. We need to avoid adding too much Windows-specific cruft to the LLInt.

Is this something that could be done with a macro?

[Mark Lam]

(In reply to comment #11)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > However, there are alot of failures on the JSC stress tests.
> > > Should we look into those first, or should I post a patch to enable it, and look into the stress tests afterwards?
> > 
> > Do those tests fail with the C Loop LLINT?  If turning on the ASM LLINT and JIT does not introduce regressions, then yes, let’s turn it on.
> 
> Ok, sounds good, I will test that, and create a new patch if there are no regressions.

Please also take note of what the failures are.  If I remember correctly, most of them should be only timeouts.  Turning on the baselineJIT and DFGJIT should help with that.  I think there are also some 32-bit x86 failures still, but am not certain.  Let’s make a record of what the failures are and whether they are regressions, and then, we can make an informed decision based on that.

[Geoffrey Garen]

> > Windows is not the only platform on which the incoming stack is unaligned. For example, the same is true on ARM32. Can we share some of this code? It seems like aligning the stack is a generic action in the LLInt. We need to avoid adding too much Windows-specific cruft to the LLInt.
> 
> Is this something that could be done with a macro?

Possibly.

What we want, ideally, is a cross-platform idiom like this:

if (!incomingStackIsAligned())
    alignStack();

'incomingStackIsAligned()' would return false on Windows and ARM32, and true on some other platforms. alignStack() might have a shared implementation on all platforms based on our abstract fp and sp registers -- or it might need a specific implementation on some platforms.