Bug 172957 – We should not claim that SpecEmpty is filtered out of cell checks on 64 bit platforms

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

the following sample can crash safari 10.1.

class A { };

class B extends A {
    constructor(a, b) {
        var f = () => b ? this : {};
        if (a) {
            var val = f() == super();
        } else {
            super();
            var val = f();
        }
    }
};

for (var i=0; i < 10000; i++) {
    try {
        new B(true, true);
    } catch (e) {
    }
    var a = new B(false, true);
    var c = new B(true, false);
}

[Alexey Proskuryakov]

Created attachment 312144 [details]
crash log

[Alexey Proskuryakov]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                           	0x00004e19b5e03ca6 0 + 85872332520614
1   com.apple.JavaScriptCore      	0x00007fff9877bca0 llint_entry + 27522
2   com.apple.JavaScriptCore      	0x00007fff98774f3b vmEntryToJavaScript + 299
3   com.apple.JavaScriptCore      	0x00007fff9865017e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 158
4   com.apple.JavaScriptCore      	0x00007fff9861dd38 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 15192
5   com.apple.JavaScriptCore      	0x00007fff982e3f31 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 337
6   com.apple.WebCore             	0x00007fff9db351c9 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 313

[Radar WebKit Bug Importer]

<rdar://problem/32602704>

[Saam Barati]

This bug is really interesting. We have code like this:

```
block#1:
n: GetClosureVar(..., |this|) // this will load empty JSValue()
SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks like a cell
Branch(#2, #3)

Block#3:
x: GetLocal(locFoo)
y: CheckNotEmpty(@x)
```

However, AI will think that @x is a cell, hence not empty, and we'll crash because we eliminate node @y

[Filip Pizlo]

(In reply to Saam Barati from comment #4)
> This bug is really interesting. We have code like this:
> 
> ```
> block#1:
> n: GetClosureVar(..., |this|) // this will load empty JSValue()
> SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks
> like a cell
> Branch(#2, #3)
> 
> Block#3:
> x: GetLocal(locFoo)
> y: CheckNotEmpty(@x)
> ```
> 
> However, AI will think that @x is a cell, hence not empty, and we'll crash
> because we eliminate node @y

Sounds like AI should model cell checks as admitting empty on 64-bit.

[Saam Barati]

(In reply to Filip Pizlo from comment #5)
> (In reply to Saam Barati from comment #4)
> > This bug is really interesting. We have code like this:
> > 
> > ```
> > block#1:
> > n: GetClosureVar(..., |this|) // this will load empty JSValue()
> > SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks
> > like a cell
> > Branch(#2, #3)
> > 
> > Block#3:
> > x: GetLocal(locFoo)
> > y: CheckNotEmpty(@x)
> > ```
> > 
> > However, AI will think that @x is a cell, hence not empty, and we'll crash
> > because we eliminate node @y
> 
> Sounds like AI should model cell checks as admitting empty on 64-bit.

I was thinking of either doing this, or we can change the value representation for TDZ sentinel.

[Saam Barati]

Created attachment 312274 [details]
WIP

still a bunch of assertion failures, I need to audit more code

[Saam Barati]

Created attachment 312337 [details]
WIP

[Saam Barati]

Created attachment 312338 [details]
WIP

[Saam Barati]

Might be all that's needed

[Saam Barati]

(In reply to Saam Barati from comment #10)
> Might be all that's needed

it passes release JSC tests

[Build Bot]

Comment on attachment 312338 [details]
WIP

Attachment 312338 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3895910

New failing tests:
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-no-llint

[Saam Barati]

Created attachment 312357 [details]
patch

[Saam Barati]

Comment on attachment 312357 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:134
> +    WTFReportBacktrace();

oops, this should be removed.

[Saam Barati]

Created attachment 312359 [details]
patch

[Saam Barati]

Created attachment 312361 [details]
patch

[Saam Barati]

Comment on attachment 312361 [details]
patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447
> +        FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol, isNotSymbol(cell));

We can go either way for all these various speculateX functions. They all do lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the set. However, we could leave this code as is (this is how the DFG writes many of its speculation checks). This would leave these functions open to folks doing their own manual checks that haven't updated AI state.

[Saam Barati]

Comment on attachment 312361 [details]
patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447
>> +        FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol, isNotSymbol(cell));
> 
> We can go either way for all these various speculateX functions. They all do lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the set. However, we could leave this code as is (this is how the DFG writes many of its speculation checks). This would leave these functions open to folks doing their own manual checks that haven't updated AI state.

Alternatively, we could assert against the current state of things. So if folks started using this in the way described above, they'd hit the assert, and be forced to write the more general version.

[Filip Pizlo]

Comment on attachment 312361 [details]
patch

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

I think it's correct, but I think there are two style goofs:

- The cellCheckTypePassthrough is not specific to the DFG - it's just how cell checks work everywhere, including LLInt, Baseline, and JSValue.  I think it should be defined in SpeculatedType.h and follow the same naming convention as everything else (so SpecCellBlahBlah).

- There seem to be places where you don't use this type constant and instead you introduce #if's.  You should probably use the constant you introduced.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:63
> +// When doing a cell check, this is the type that passes through
> +static constexpr SpeculatedType cellCheckTypePassthrough = is64Bit() ? SpecCellOrEmpty : SpecCell;
> +

I think this should be SpecCellCheck, defined in SpeculatedType.h.  Even JSValue's isCell() has this issue, so it's not DFG-specific.

I'd call it SpecCellCheck, but I'm also OK with SpecCellChekTypePassthrough, or whatever.  Shorter names are better IMO, but I don't care too much.  I care more about this not being DFG-specific, and being named SpecBlah like all the other SpeculatedType constants.

> Source/JavaScriptCore/dfg/DFGUseKind.h:120
> +#if USE(JSVALUE64)
> +        return SpecCellOrEmpty;
> +#else
> +        return SpecCell;
> +#endif

Shouldn't this use SpecCellCheck, or whatever it ends up being called?

> Source/JavaScriptCore/dfg/DFGUseKind.h:128
> +#if USE(JSVALUE64)
> +        return SpecCellOrEmpty | SpecOther;
> +#else
>          return SpecCell | SpecOther;
> +#endif

Ditto.

> Source/JavaScriptCore/dfg/DFGUseKind.h:171
> +#if USE(JSVALUE64)
> +        return ~SpecCellOrEmpty;
> +#else
>          return ~SpecCell;
> +#endif

Ditto.

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447
>>> +        FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol, isNotSymbol(cell));
>> 
>> We can go either way for all these various speculateX functions. They all do lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the set. However, we could leave this code as is (this is how the DFG writes many of its speculation checks). This would leave these functions open to folks doing their own manual checks that haven't updated AI state.
> 
> Alternatively, we could assert against the current state of things. So if folks started using this in the way described above, they'd hit the assert, and be forced to write the more general version.

I guess speculateSymbol was modeled after speculateString.

[Saam Barati]

Created attachment 312617 [details]
patch

Addressed Fil's comments. I also like the name "SpecCellCheck", and decided to use that.

[WebKit Commit Bot]

Comment on attachment 312617 [details]
patch

Clearing flags on attachment: 312617

Committed r218137: <http://trac.webkit.org/changeset/218137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.