Bug 125736 - jsCStack: Fix exception handling for the baseline JIT.
Summary: jsCStack: Fix exception handling for the baseline JIT.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-14 03:46 PST by Mark Lam
Modified: 2013-12-16 12:44 PST (History)
5 users (show)

See Also:


Attachments
the patch. (4.91 KB, patch)
2013-12-14 04:05 PST, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
patch 2 (10.40 KB, patch)
2013-12-14 16:49 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-12-14 03:46:55 PST
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.
Comment 1 Mark Lam 2013-12-14 04:05:23 PST
Created attachment 219247 [details]
the patch.
Comment 2 Michael Saboff 2013-12-14 09:37:17 PST
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?
Comment 3 Mark Lam 2013-12-14 09:54:19 PST
(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.
Comment 4 Mark Lam 2013-12-14 12:14:45 PST
> (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.
Comment 5 Mark Lam 2013-12-14 16:49:56 PST
Created attachment 219261 [details]
patch 2

All issues fixed now AFAIK.
Comment 6 Geoffrey Garen 2013-12-16 11:42:30 PST
Comment on attachment 219261 [details]
patch 2

r=me
Comment 7 Mark Lam 2013-12-16 12:44:27 PST
Thanks for the review.  Landed in r160656 on the jsCStack branch: <http://trac.webkit.org/r160656>.