|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>|
|Severity:||Normal||CC:||benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki|
|Version:||WebKit Nightly Build|
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 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] 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
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] patch 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 10 Michael Saboff 2019-01-24 09:48:14 PST
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.