Bug 125672 – jsCStack: Fix exception handling for the LLINT.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

We need to re-establish the sp for the catch callFrame in op_catch and nativeCallTrampoline.

[Mark Lam]

Created attachment 219143 [details]
the patch.

[Darin Adler]

Comment on attachment 219143 [details]
the patch.

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:283
> +    # So far, we've unwound the stack to the farme just above the sentinel frame.

Typo: "farme"

[Geoffrey Garen]

Comment on attachment 219143 [details]
the patch.

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

r=me

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:283
>> +    # So far, we've unwound the stack to the farme just above the sentinel frame.
> 
> Typo: "farme"

The stack grows downward, so I think you meant "below" here, and not "above".

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1971
> +    # So far, we've unwound the stack until the native frame. We need to pop it,
> +    # and pick up unwinding from its caller now (if needed).

This comment is misleading. We didn't "unwind the stack". Instead, the host function returned to us in the normal way. A better thing to say here is just "Pop the host function stack frame created by op_call".

Even that is not a great comment, since it doesn't explain *why*. Why does _llint_throw_from_native_call need us to pop the host function stack frame? I'm not sure.

[Mark Lam]

(In reply to comment #3)
> (From update of attachment 219143 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219143&action=review
> 
> r=me
> 
> >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:283
> >> +    # So far, we've unwound the stack to the farme just above the sentinel frame.
> > 
> > Typo: "farme"
> 
> The stack grows downward, so I think you meant "below" here, and not "above".

Fixed.

> > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1971
> > +    # So far, we've unwound the stack until the native frame. We need to pop it,
> > +    # and pick up unwinding from its caller now (if needed).
> 
> This comment is misleading. We didn't "unwind the stack". Instead, the host function returned to us in the normal way. A better thing to say here is just "Pop the host function stack frame created by op_call".

We're only here because an exception was thrown and uncaught / unhandled in the native function or any of the JS functions it called.  Hence, there was some stack unwinding action happening prior to getting here, and the frame that we stopped unwinding on is the native frame and it did not handle and clear the exception in the VM.  What this piece of code is suppose to do is to continue the unwinding process from the caller of the native frame.

The reason I added this comment was to provide some context of what happened before, and where we need to pick up the unwinding work.  Is this not adequate as a reason to have this comment?

> Even that is not a great comment, since it doesn't explain *why*. Why does _llint_throw_from_native_call need us to pop the host function stack frame? I'm not sure.

The exception handling mechanism that unwinds the stack will stop unwinding when it sees a native / host function frame.  Just to clarify, are you suggesting that _llint_throw_from_native_call should do the work of popping off the host frame?

Actually looking at the implementation of _llint_throw_from_native_call, I see that it is a useless function that only asserts that VM::m_exception is not null.  But the only way we could possibly reach this code path is if VM::m_exception is not null.  Hence, _llint_throw_from_native_call should be removed altogether.

I'll clean that up, test it, and upload another patch for a follow up review.

[Mark Lam]

There appears to be a regression after I svn’ed up.  This patch is no longer adequate to make exception handling work for the LLINT.  I’m currently investigating the new issues.

[Mark Lam]

(In reply to comment #5)
> There appears to be a regression after I svn’ed up.  This patch is no longer adequate to make exception handling work for the LLINT.  I’m currently investigating the new issues.

Correction: the regression is not in the vicinity of the exception handling code.  Instead the following statement caused the crash I saw:

    print(“My object: “ + { });

I haven’t dug deeper yet, but it may have to do with Object.toString().  The issue is documented in https://bugs.webkit.org/show_bug.cgi?id=125694.

Continuing with exceptions work now.

[Mark Lam]

Created attachment 219181 [details]
patch 2: removed llint_throw_from_native_call.

I changed my mind about the comment in nativeCallTrampoline.  Without the now elided call to llint_throw_from_native_call, the exception handling section is much simpler and I don't think the comment is needed anymore.

[Geoffrey Garen]

Comment on attachment 219181 [details]
patch 2: removed llint_throw_from_native_call.

r=me

[Mark Lam]

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