Bug 147988 – DFG callOperations should not implicitly emit an exception check. At callOperation call sites, we should explicitly emit exception checks

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 259073 [details]
patch

I still am failing 3d-cube under exception fuzzing.
I need to see why this is. I can't reproduce this under
the same exception fire target as the exception-fuzzing perl
script is using. Once I figure this out, this patch is ready
to go. It passes the other tests.

Also, this patch includes comments for where we aren't
emitting exception checks. This will not necessarily be
in the landed patch (though we could add some variant of it
if it's helpful). Those comments exist to let the reviewer of
this patch know which callOperations aren't emitting exception
checks.

[Saam Barati]

Created attachment 259074 [details]
patch

rebased, same patch as before .

[WebKit Commit Bot]

Attachment 259074 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:100:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3685:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3748:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3807:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3840:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3852:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5978:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5993:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6430:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1693:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1694:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1697:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:575:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:612:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:624:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2263:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1852:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2181:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2383:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2396:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4882:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4915:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:205:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:207:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 24 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 259086 [details]
patch

fixed exception fuzzing issues.
I'm just making operationExceptionFuzz an almost
normal operation that takes an ExecState.

[WebKit Commit Bot]

Attachment 259086 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:100:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3685:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3748:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3807:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3840:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3852:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5978:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5993:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6430:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1693:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1694:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1697:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:575:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:612:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:624:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2263:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1852:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2181:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2383:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2396:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4882:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4915:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 22 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 259107 [details]
patch

free buffer used for exception fuzzing on VM destruction.

[WebKit Commit Bot]

Attachment 259107 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:100:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3685:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3748:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3807:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3840:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3852:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5978:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5993:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6430:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1693:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1694:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1697:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:575:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:612:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:624:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2263:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1852:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2181:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2383:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2396:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4882:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4915:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 22 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 259107 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1887
> +    void checkException()

I want to switch this name to checkExceptionAfterCall().

[Mark Lam]

Comment on attachment 259107 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:96
> +        SpillRegistersMode spillMode, bool needsExceptionCheck, ResultType result)

Instead of using a boolean, can you use an enum type like:

enum class ExceptionCheckRequirement {
    CheckNotNeeded,
    CheckNeeded
};

Ditto for all other places where you pass the needsExceptionCheck param.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:565
>          callOperation(operationCompareStrictEqCell, resultPayloadGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR);

For cases like these where you expect the operation to not throw an exception, you could assert in the operation function that no exception was thrown.  Alternatively, Fil's idea to generate an assertion at each node boundary that you told me about sounds good too.  Even if the assertion check is not needed right now, I think it would be good to have some form of an assertion in case someone modifies the operation function later in such a way that can throw an exception.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:566
> +        // checkException(); // Maybe not needed?

operationCompareStrictEqCell() can throw OutOfMemoryError due to JSRopeString::resolveRope() called from JSString::value(), called from JSValue::strictEqualSlowCaseInline().

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:574
> -        callOperation(operationCompareStrictEq, resultPayloadGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR);
> +        callOperation(operationCompareStrictEq, resultPayloadGPR, arg1TagGPR, arg1PayloadGPR, arg2TagGPR, arg2PayloadGPR); // Maybe not needed?

I don't think you intended to add the "// Maybe not needed?" comment at the end of the callOperation line.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:575
> +        //checkException(); // Maybe not needed?

Ditto.  operationCompareStrictEq() can throw OutOfMemoryError.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:612
> +        //checkException(); // Maybe not needed?

Ditto.  operationCompareStrictEqCell can throw OutOfMemoryError.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:624
> +        //checkException(); // Maybe not needed?

Ditto.  operationCompareStrictEq() can throw OutOfMemoryError.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1693
> +        // checkException(); // Not needed.

Yes, exception check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2250
> +        // checkException(); // Not needed.

Yes, check not needed.  sin() has no knowledge of the VM and cannot throw.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2263
> +        //checkException(); // Not needed.

Yes, check not needed.  cos() has no knowledge of the VM and cannot throw.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:516
> +        checkException(); // Maybe not needed?

Exception check needed.  operationCompareStrictEqCell can throw OutOfMemoryError.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:543
> +        checkException(); // Maybe not needed?

Exception check needed.  operationCompareStrictEq can throw OutOfMemoryError.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:579
> +        checkException(); // Maybe not needed?

Exception check needed.  operationCompareStrictEqCell can throw OutOfMemoryError.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1698
> +            slowPathCall(slowCase, this, operationConvertJSValueToBoolean, resultGPR, arg1GPR, NeedToSpill, false)); // Does not need exception check.

Yes, operationConvertJSValueToBoolean does not need an exception check.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1852
> +            //checkException(); // Maybe not needed?

Yes, operationConvertJSValueToBoolean does not need an exception check.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2181
> +            //checkException(); // Not needed.

Yes, operationConvertDoubleToInt52 does not need an exception check.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2383
> +        //checkException(); // Not needed. 

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2396
> +        //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4882
> +    //checkException(); // Not needed.

Yes, operationConvertBoxedDoubleToInt52 does not need an exception check.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4915
>      callOperation(operationConvertDoubleToInt52, resultGPR, valueFPR);
> +    //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3685
>          callOperation(fmodAsDFGOperation, result.fpr(), op1FPR, op2FPR);
> +        //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3748
>          callOperation(sqrt, result.fpr(), op1FPR);
> +        //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3807
>          callOperation(operationMathPow, resultFpr, xOperandfpr, yOperandfpr.fpr());
> +        //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3840
>      callOperation(operationMathPow, resultFpr, xOperandfpr, yOperandfpr);
> +    //checkException(); // Not Needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3852
>      callOperation(log, result.fpr(), op1FPR);
> +    //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5052
> +        slowPathCall(slowCase, this, operationNotifyWrite, NoResult, set, NeedToSpill, false)); // Does not need exception check.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5978
> +        //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5993
> +        //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6430
> +    //checkException(); // Not needed.

Yes, check not needed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1697
> +    //JITCompiler::Call callOperation(S_JITOperation_J operation, GPRReg result, GPRReg arg1Tag, GPRReg arg1Payload)
> +    //{
> +    //    m_jit.setupArguments(arg1Payload, arg1Tag);
> +    //    return appendCallSetResult(operation, result);
> +    //}

Have you done a 32-bit build to verify that this is not needed?  If so, let's remove it.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1887
>> +    void checkException()
> 
> I want to switch this name to checkExceptionAfterCall().

Sounds good.

[Saam Barati]

Created attachment 259591 [details]
patch

addressed Mark's feedback.

[Saam Barati]

Created attachment 259593 [details]
patch

[WebKit Commit Bot]

Attachment 259593 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:105:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 259594 [details]
patch

[WebKit Commit Bot]

Attachment 259594 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:105:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 259594 [details]
patch

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

r=me with some suggestions

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1505
> +        m_jit.jitAssertNoException();

Can we do something to avoid paying the cost of this check in release builds?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1897
> +    void checkExceptionAfterCall()

It looks like this function can be used not after a call too. I would name this function "exceptionCheck", or name it "checkException" and rename JIT::exceptionCheck to checkException, or eliminate this function because it's not onerous to call "m_jit.exceptionCheck()".

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1901
> +    // These methods add call instructions, with optional exception checks & setting results.

, optionally emitting exception checks and/or setting results.

> Source/JavaScriptCore/runtime/VM.h:623
> +    EncodedJSValue* m_exceptionFuzzBuffer { nullptr };

Let's use MallocPtr here, to avoid the need for manual destruction.

[Saam Barati]

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