Bug 193751

Summary: Object Allocation Sinking phase can move a node that walks the stack into a place where the InlineCallFrame is no longer valid
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Severity: Normal CC: benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
ews-watchlist: commit-queue-
msaboff: review+
patch for landing none

Description Saam Barati 2019-01-23 17:45:29 PST
Comment 1 Saam Barati 2019-01-23 17:46:28 PST
Basically, we move a heap allocation elsewhere in the program. That heap allocation may walk the stack. Walking the stack may read fields w.r.t the inline call frame on the stack. However, by the time we actually walk the stack those fields may no longer be valid.
Comment 2 Saam Barati 2019-01-23 17:49:26 PST
Some ways to solve this:

1. Update CallSiteIndex to be the forExit CodeOrigin instead of the semantic origin. This should mostly Just Work. Some potential fallout is a stack trace may give you unexpected line/column info. The reason this is nice is it may fix other future bugs that run into similar issues reading an inline call frame's stack slots.

2. Update OAS to not perform such moving of code when we detect this scenario. I kind of like this since LICM/ArgumentsElimination models this scenario correctly and won't hoist/eliminate code in such a situation.

3. All of these heap allocations that move should use a DeferGCForAWhile. I don't like this solution since it means you can write programs that allocate indefinitely without GCing. But it would work.
Comment 3 Saam Barati 2019-01-23 17:50:00 PST
Comment 4 Saam Barati 2019-01-23 19:14:36 PST
Created attachment 359986 [details]
Comment 5 Saam Barati 2019-01-23 19:15:51 PST
This patch goes with approach (2). I'm open to hearing cases of doing (1) or (3).
Comment 6 EWS Watchlist 2019-01-23 21:01:23 PST
Comment on attachment 359986 [details]

Attachment 359986 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10868080

New failing tests:
Comment 7 Saam Barati 2019-01-23 21:19:32 PST
(In reply to Build Bot from comment #6)
> Comment on attachment 359986 [details]
> patch
> Attachment 359986 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/10868080
> New failing tests:
> stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-
> trace-is-still-valid.js.default
> apiTests

I'm not sure what's happening here. This passes for me locally as it's the test case I'm adding...
Comment 8 Saam Barati 2019-01-23 21:23:37 PST
Comment on attachment 359986 [details]

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

> JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js:1
> +//@ runDefault("useConcurrentJIT=0", "jitPolicyScale=0", "collectContinuously=1")

oops, I'm missing "--" before these options.
Comment 9 Saam Barati 2019-01-23 21:24:03 PST
Created attachment 359991 [details]
Comment 10 Michael Saboff 2019-01-24 09:48:14 PST
Comment on attachment 359991 [details]

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


> Source/JavaScriptCore/ChangeLog:11
> +        walk the stack at the point in the program that it moved the allocation to.

nit: *to* walk the ...
Comment 11 Saam Barati 2019-01-24 12:53:08 PST
Created attachment 360027 [details]
patch for landing
Comment 12 WebKit Commit Bot 2019-01-24 13:31:01 PST
Comment on attachment 360027 [details]
patch for landing

Clearing flags on attachment: 360027

Committed r240447: <https://trac.webkit.org/changeset/240447>
Comment 13 WebKit Commit Bot 2019-01-24 13:31:03 PST
All reviewed patches have been landed.  Closing bug.