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 treegraph
 
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

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