Bug 109387

Summary: DFG CompareEq(a, null) and CompareStrictEq(a, const) are unsound with respect to constant folding
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, jberlin, mark.lam, mhahnenberg, msaboff, oliver, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 109470    
Bug Blocks:    
Attachments:
Description Flags
the patch
none
the patch oliver: review+

Description Filip Pizlo 2013-02-10 13:47:18 PST
CompareEq(a, null) does strictly less speculation than CompareEq(a, b).  Hence if we do one run of CFA and eliminate checks based on seeing CompareEq(a, b) - which can happen because CompareEq(a, b) "proves" that we had done a check which we may later eliminate - and then later prove that b is null, then we will end up emitting zero checks.  And then weirdness ensues.
Comment 1 Filip Pizlo 2013-02-10 21:01:31 PST
Created attachment 187510 [details]
the patch

Not marking r? yet because I want to run more benchmarks...
Comment 2 Filip Pizlo 2013-02-10 22:36:47 PST
Created attachment 187514 [details]
the patch
Comment 3 Filip Pizlo 2013-02-11 11:23:05 PST
Landed in http://trac.webkit.org/changeset/142491
Comment 4 Jessie Berlin 2013-02-11 11:52:13 PST
(In reply to comment #3)
> Landed in http://trac.webkit.org/changeset/142491

This caused build breakage:

DFGSpeculativeJIT32_64.cpp:2113:13: error: enumeration values 'CompareEqConstant' and 'CompareStrictEqConstant' not handled in switch [-Werror,-Wswitch]
    switch (op) {
            ^
Comment 5 WebKit Review Bot 2013-02-11 12:31:07 PST
Re-opened since this is blocked by bug 109470
Comment 6 Jessie Berlin 2013-02-11 12:34:10 PST
Rollout is happening in https://bugs.webkit.org/show_bug.cgi?id=109470
Comment 7 Filip Pizlo 2013-02-11 14:24:54 PST
Landed in http://trac.webkit.org/changeset/142515