Bug 149228

Summary: Interpreter::unwind() shouldn't be responsible for filtering out uncatchable exceptions
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, oliver, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 147374    
Attachments:
Description Flags
patch
none
patch
none
patch mark.lam: review+

Description Saam Barati 2015-09-16 13:07:10 PDT
op_catch should do this.
Comment 1 Saam Barati 2015-09-16 17:58:34 PDT
Created attachment 261343 [details]
patch
Comment 2 WebKit Commit Bot 2015-09-16 18:00:36 PDT
Attachment 261343 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1462:  The parameter name "profiler" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/Interpreter.cpp:657:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2145:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Geoffrey Garen 2015-09-16 18:29:14 PDT
Comment on attachment 261343 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:15
> +        the exception further down the call stack. This is necessary
> +        in a later patch that will implement exception handling
> +        in the DFG, and part of that patch includes exception
> +        handling that doesn't go through genericUnwind. This patch
> +        enables that by destroying the notion that all exception 

Can you explain a little more about this? Why does the DFG sometimes want to call into genericUnwind and sometimes want not to?
Comment 4 Saam Barati 2015-09-16 18:29:25 PDT
Created attachment 261345 [details]
patch
Comment 5 WebKit Commit Bot 2015-09-16 18:30:55 PDT
Attachment 261345 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1462:  The parameter name "profiler" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/Interpreter.cpp:657:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2145:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Saam Barati 2015-09-16 18:50:00 PDT
Created attachment 261349 [details]
patch

fix style
Comment 7 WebKit Commit Bot 2015-09-16 18:53:21 PDT
Attachment 261349 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1462:  The parameter name "profiler" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:2147:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Mark Lam 2015-09-17 09:38:29 PDT
Comment on attachment 261349 [details]
patch

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

r=me.  Since you're changing the LLINT, please do a test run of the JSC tests on the cloop build as well to make sure nothing broke.  Thanks.

> Source/JavaScriptCore/ChangeLog:14
> +        the exception further down the call stack. This is necessary
> +        in a later patch that will implement exception handling
> +        in the DFG, and part of that patch includes exception
> +        handling that doesn't go through genericUnwind. This patch

I think you can provide a little more detail to motivate this.  How about something like:

... This is necessary in a later patch that will implement exception handling in the DFG.  The DFG may determine that an exception thrown in a function (or an inlined callee) is caught by a catch block in the sane function, and optimize away the call to genericUnwind altogether. ...

> Source/JavaScriptCore/interpreter/Interpreter.cpp:658
> +        if (!m_isTermination) {
> +            if (m_codeBlock && !isWebAssemblyExecutable(m_codeBlock->ownerExecutable()))
> +                m_handler = m_codeBlock->handlerForBytecodeOffset(bytecodeOffset);
> +            else
> +                m_handler = nullptr;
> +        }
> +
> +        if (!m_isTermination && m_handler)
> +            return StackVisitor::Done;

nit, but I suggest reducing this further to:

        m_handler = nullptr;
        if (!m_isTermination) {
            if (m_codeBlock && !isWebAssemblyExecutable(m_codeBlock->ownerExecutable()))
                m_handler = m_codeBlock->handlerForBytecodeOffset(bytecodeOffset);
        }

        if (m_handler)
            return StackVisitor::Done;

> Source/JavaScriptCore/interpreter/Interpreter.cpp:741
>  
>      ASSERT(vm.exception() && vm.exception()->stack().size());

Above these lines, there's a check for:
    if (exceptionValue.isObject())
        isTermination = isTerminatedExecutionException(exception);

You can now remove the "if (exceptionValue.isObject())" check.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1468
> +        genericUnwind(&vm, exec);
> +        pc = returnToThrow(exec);
> +        LLINT_RETURN_TWO(pc, bitwise_cast<void*>(static_cast<uintptr_t>(1)));

Why not just do a "LLINT_RETURN_TWO(pc, 1);" here instead and let llint_throw_from_slow_path_trampoline take care of calling genericUnwind()?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1846
> +    dispatch(0) # go to exception handler because this is a terminating exception.

Why not just do a  "jmp _llint_throw_from_slow_path_trampoline" here?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1736
> +    dispatch(0) # go to exception handler because this is a terminating exception.

Why not just do a  "jmp _llint_throw_from_slow_path_trampoline" here?
Comment 9 Saam Barati 2015-09-17 11:31:58 PDT
landed in:
http://trac.webkit.org/changeset/189920