Bug 149228 – Interpreter::unwind() shouldn't be responsible for filtering out uncatchable exceptions

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

[Saam Barati]

op_catch should do this.

[Saam Barati]

Created attachment 261343 [details]
patch

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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?

[Saam Barati]

Created attachment 261345 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 261349 [details]
patch

fix style

[WebKit Commit Bot]

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.

[Mark Lam]

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?

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/189920