Bug 109387 - DFG CompareEq(a, null) and CompareStrictEq(a, const) are unsound with respect to constant folding
Summary: DFG CompareEq(a, null) and CompareStrictEq(a, const) are unsound with respect...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 109470
Blocks:
  Show dependency tree / graph
 
Reported: 2013-02-10 13:47 PST by Filip Pizlo
Modified: 2013-02-11 14:24 PST (History)
9 users (show)

See Also:

Attachments
the patch (12.22 KB, patch)
2013-02-10 21:01 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (14.58 KB, patch)
2013-02-10 22:36 PST, Filip Pizlo 		oliver: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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