Bug 125736 – jsCStack: Fix exception handling for the baseline JIT.

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

[Mark Lam]

After the LLINT exception handling fixes, the baseline JIT had 2 remaining issues in :
1. After returning from a native / host function, it did not pop the native / host frame before executing to exception handling code.  This resulted in the VM trying to "unwind" the native / host frame which is not possible.
2. The exception handling thunk (after returning from a native / host function) was wrongly popping a non-existant return address on the stack.  This caused the callerFrame pointer of the current frame to be popped off the stack, and havoc ensues.

Patch coming up soon.

[Mark Lam]

Created attachment 219247 [details]
the patch.

[Michael Saboff]

Comment on attachment 219247 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=219247&action=review

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:-390
> -    // Grab the return address.
> -    jit.preserveReturnAddressAfterCall(JSInterfaceJIT::regT1);
> -    

If you remove this, what is in regT1 that the storePtr below uses for its source?

[Mark Lam]

(In reply to comment #2)
> (From update of attachment 219247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219247&action=review
> 
> > Source/JavaScriptCore/jit/ThunkGenerators.cpp:-390
> > -    // Grab the return address.
> > -    jit.preserveReturnAddressAfterCall(JSInterfaceJIT::regT1);
> > -    
> 
> If you remove this, what is in regT1 that the storePtr below uses for its source?

You are right.  This is not correct.  Will take another look.

[Mark Lam]

> (In reply to comment #2)
> > If you remove this, what is in regT1 that the storePtr below uses for its source?

This turned out to be a non-issue.  It was storing regT1 into VM::m_exceptionLocation, but no one uses VM::m_exceptionLocation anymore.  So, I simply removed the dead code.

That said, while investigating this, I thought to re-test a scenario that I thought was caused by <https://bugs.webkit.org/show_bug.cgi?id=125694> (trying to reduce a test case for it was what led to my discovering https://bugs.webkit.org/show_bug.cgi?id=125694).  It turned out that that scenario is still not resolved.  Here's the scenario in question:

    function b() {
        try {
            throw { }
        } catch(e) {
            throw e;
        }
    }

    try {
        var array = [ 1 ];
        array.forEach(b);
    } catch (e) {
        print("Caught in a(): " + e); // This line is key to reproducing the issue.
    }

This results in an alignment trap in callToNativeFunction.  Some debugging reveals that the stack was already misaligned a lot further back in C code before we got to the callToNativeFunction call that fell on the trap.  The damage was done elsewhere and just happened to be discovered here.

Another issue is scenario:

    function b() {
          throw { }
    }

    // A try catch block around the Array.forEach() is necessary for reproducing the issue.
    try {
        var array = [ 1 ];
        array.forEach(b);
    } catch (e) {
    }

With this, I get a crash in __CFRunLoopFindMode().

The first issue may or may not have to do with exception handling, but the second one obviously is related.  These issue do not reproduce if only run on the LLINT.  They are baseline JIT issues.

Continuing to investigate.

[Mark Lam]

Created attachment 219261 [details]
patch 2

All issues fixed now AFAIK.

[Geoffrey Garen]

Comment on attachment 219261 [details]
patch 2

r=me

[Mark Lam]

Thanks for the review.  Landed in r160656 on the jsCStack branch: <http://trac.webkit.org/r160656>.