Bug 174321 - Allocation sinking phase should consider a CheckStructure that would fail as an escape
Summary: Allocation sinking phase should consider a CheckStructure that would fail as ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-10 14:27 PDT by Saam Barati
Modified: 2017-07-10 17:29 PDT (History)
12 users (show)

See Also:


Attachments
test IR editing (3.16 KB, patch)
2017-07-10 14:53 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (3.78 KB, patch)
2017-07-10 15:09 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing (3.78 KB, patch)
2017-07-10 16:16 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-07-10 14:27:15 PDT
Otherwise, we might try to materialize an allocation, and think the set of structures it might have is the empty set. This is clearly wrong.
Comment 1 Saam Barati 2017-07-10 14:38:08 PDT
<rdar://problem/32604963>
Comment 2 Saam Barati 2017-07-10 14:41:56 PDT
I can't write a program that exhibits this behavior. I know it's possible since I'm looking at a bunch of crashlogs that hit some assertions I added. I'll open a FIXME to try and generate test cases for this. I won't hold up the fix though. I was able to simulate the crash by manually editing the IR in a test I wrote which inserts a CheckStructure at a particular program point right before what would otherwise be an escape. When the escape tries to generate an object materialization, the object it's trying to materialize has the empty set of structures. This leads the phase to crash. Instead, I'm going to treat anything that would filter an allocations structure set to the empty set as an escape, since we've effectively proven that we would have exited at that point. This will lead us to generate a materialization right before such a CheckStructure (or MultiGetByOffset), and then when exiting that CheckStructure, we'll exit.
Comment 3 Saam Barati 2017-07-10 14:53:27 PDT
Created attachment 315032 [details]
test IR editing

Uploading my IR editing code in case anybody wants to repeat my experiment. I ran it on this program:
```
function escape() { }
noInline(escape);
noDFG(escape);

function foo(c, x) {
    let o1 = {};
    let o2 = {x, o1};
    o1.o2 = o2;
    if (c) {
        escape(o2);
    }
}
noInline(foo);

for (let i = 0; i < 100000; ++i) {
    foo(!!(i%2), i);
}
```
Comment 4 Saam Barati 2017-07-10 15:09:06 PDT
Created attachment 315033 [details]
patch
Comment 5 Keith Miller 2017-07-10 16:14:24 PDT
Comment on attachment 315033 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        objects in a cylce with each other, it would assume that each materialized

typo: cylce => cycle

> Source/JavaScriptCore/ChangeLog:14
> +        The abstract interpretation part of the phase will model whats in the heap.

typo: whats => what's
Comment 6 Saam Barati 2017-07-10 16:16:29 PDT
Created attachment 315040 [details]
patch for landing
Comment 7 WebKit Commit Bot 2017-07-10 17:29:40 PDT
Comment on attachment 315040 [details]
patch for landing

Clearing flags on attachment: 315040

Committed r219317: <http://trac.webkit.org/changeset/219317>
Comment 8 WebKit Commit Bot 2017-07-10 17:29:41 PDT
All reviewed patches have been landed.  Closing bug.