Object Allocation Sinking phase can move a node that walks the stack into a place where the InlineCallFrame is no longer valid

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

<rdar://problem/47280215>

[Saam Barati]

Created attachment 359986 [details]
patch

[Saam Barati]

This patch goes with approach (2). I'm open to hearing cases of doing (1) or (3).

[EWS Watchlist]

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

[Saam Barati]

(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...

[Saam Barati]

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.

[Saam Barati]

Created attachment 359991 [details]
patch

[Michael Saboff]

Comment on attachment 359991 [details]
patch

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

r=me

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

nit: *to* walk the ...

[Saam Barati]

Created attachment 360027 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 360027 [details]
patch for landing

Clearing flags on attachment: 360027

Committed r240447: <https://trac.webkit.org/changeset/240447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.