Bug 148076 - jsc-tailcall: Handling exception in caller frame cannot unwind past VMEntry frame
Summary: jsc-tailcall: Handling exception in caller frame cannot unwind past VMEntry f...
Status: RESOLVED DUPLICATE of bug 148666
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 147759
  Show dependency treegraph
 
Reported: 2015-08-17 06:56 PDT by Michael Saboff
Modified: 2015-09-14 10:59 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.02 KB, patch)
2015-08-17 07:11 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2015-08-17 06:56:42 PDT
When we have an exception that we process from the caller's frame, we cannot unwind past the VMEntry frame.

Currently, if a stack overflow check exception happens for a JavaScript frame immediately below a VMENtry frame, we'll unwind past not only the VMENtry frame, but also any C++ frames to find a JavaScript frame with a catch block.

This "works" for current code, but breaks when callee save registers need to be properly restored during the unwind process.  It can also leak and C++ objects allocated by the C++ code.
Comment 1 Michael Saboff 2015-08-17 07:11:46 PDT
Created attachment 259145 [details]
Patch
Comment 2 WebKit Commit Bot 2015-08-17 07:12:59 PDT
Attachment 259145 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITExceptions.h:38:  The parameter name "unwindStart" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Basile Clement 2015-08-17 15:47:46 PDT
Comment on attachment 259145 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:32
> +        (JSC::LLInt::llint_stack_check): Changed to process the exception in the currnet frame.

currnet -> current

> Source/JavaScriptCore/jit/JITExceptions.cpp:67
> +        callFrame = callFrame->callerFrame(vmEntryFrame);

Nit: You can directly use callFrame->callerFrame() here.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:487
> -    CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
> -    pc = returnToThrowForThrownException(exec);
> +    vm.throwException(exec, createStackOverflowError(exec));
> +    pc = returnToThrow(exec);

I think this should be consistent amongst all the tiers, see below.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:923
> -    subp maxFrameExtentForSlowPathCall, sp # Set up temporary stack pointer for call
> +    # Set up temporary stack pointer for call including callee saves
> +    subp maxFrameExtentForSlowPathCall + CalleeSaveSpaceStackAligned, sp

If I understand correctly, the callee save registers are saved below the stack pointer at this point. Could we instead move the saving of callee save after the stack check/setup (which would allow us to continue throwing in the caller)?
Comment 4 Michael Saboff 2015-08-17 16:04:11 PDT
(In reply to comment #3)
> Comment on attachment 259145 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259145&action=review
> 
> > Source/JavaScriptCore/ChangeLog:32
> > +        (JSC::LLInt::llint_stack_check): Changed to process the exception in the currnet frame.
> 
> currnet -> current

Fixed locally.
 
> > Source/JavaScriptCore/jit/JITExceptions.cpp:67
> > +        callFrame = callFrame->callerFrame(vmEntryFrame);
> 
> Nit: You can directly use callFrame->callerFrame() here.

Changed locally.

> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:487
> > -    CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
> > -    pc = returnToThrowForThrownException(exec);
> > +    vm.throwException(exec, createStackOverflowError(exec));
> > +    pc = returnToThrow(exec);
> 
> I think this should be consistent amongst all the tiers, see below.

The reason for the difference is that the LLInt makes a call to lint_stack_check() and the way these slow path calls work is that they pass the PC as the second argument and return the possibly modified PC as the first of two values.  In the LLInt, the PC value is a callee save and therefore it is easiest to preserve the callee saves, set up the PC before making the stack check call.  Given that we need to unwind and restore callee saves for the frame where the stack overflow occurred.  The JITs don't have this issue.

> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:923
> > -    subp maxFrameExtentForSlowPathCall, sp # Set up temporary stack pointer for call
> > +    # Set up temporary stack pointer for call including callee saves
> > +    subp maxFrameExtentForSlowPathCall + CalleeSaveSpaceStackAligned, sp
> 
> If I understand correctly, the callee save registers are saved below the
> stack pointer at this point. Could we instead move the saving of callee save
> after the stack check/setup (which would allow us to continue throwing in
> the caller)?

See above.
Comment 5 Michael Saboff 2015-08-17 16:58:05 PDT
Committed r188555: <http://trac.webkit.org/changeset/188555>
Comment 6 Basile Clement 2015-08-31 18:08:52 PDT

*** This bug has been marked as a duplicate of bug 148666 ***
Comment 7 Csaba Osztrogonác 2015-09-14 10:59:32 PDT
Comment on attachment 259145 [details]
Patch

Cleared review? from attachment 259145 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).