Bug 199821 – ArgumentsEliminationPhase should insert KillStack nodes before PutStack nodes that it adds.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

<rdar://problem/52452328>

[Mark Lam]

Created attachment 374184 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 374184 [details]
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:254
>          }

I also would like to suggest that filling [mandatoryMinimum, limit] range with `setFlush(FlushedAt(ConflictingFlush));` (KillStack) to represent the precise availability information in this phase.

[Yusuke Suzuki]

Comment on attachment 374184 [details]
proposed patch.

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

>> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:254
>>          }
> 
> I also would like to suggest that filling [mandatoryMinimum, limit] range with `setFlush(FlushedAt(ConflictingFlush));` (KillStack) to represent the precise availability information in this phase.

Typo, [mandatoryMinimum, limit).

[EWS Watchlist]

Comment on attachment 374184 [details]
proposed patch.

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

New failing tests:
microbenchmarks/string-concat-long-convert.js.ftl-eager
microbenchmarks/string-concat-long.js.default
stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-validate-sampling-profiler
stress/varargs-varargs-inlined-exit.js.ftl-eager-no-cjit-b3o1
stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-eager
stress/load-varargs-then-inlined-call-and-exit.js.ftl-eager
stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-eager-no-cjit-b3o1
microbenchmarks/string-concat-long.js.ftl-eager-no-cjit-b3o1
microbenchmarks/string-concat-long-convert.js.ftl-eager-no-cjit
microbenchmarks/call-spread-call.js.ftl-eager-no-cjit
microbenchmarks/call-spread-apply.js.ftl-no-cjit-b3o0
stress/tailCallForwardArguments.js.ftl-eager-no-cjit-b3o1
stress/construct-varargs-inline-smaller-Foo.js.ftl-eager-no-cjit
microbenchmarks/call-spread-apply.js.default
stress/construct-varargs-inline-smaller-Foo.js.ftl-eager
stress/varargs-varargs-inlined-exit.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/call-spread-call.js.ftl-eager-no-cjit-b3o1
stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-eager
microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-no-put-stack-validate
stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-no-cjit-b3o0
stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-then-inlined-call.js.ftl-eager-no-cjit
stress/spread-multi-layers.js.bytecode-cache
stress/load-varargs-then-inlined-call-inlined.js.ftl-no-cjit-no-put-stack-validate
stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-small-pool
stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-eager
stress/spread-multi-layers.js.ftl-no-cjit-b3o0
microbenchmarks/call-spread-call.js.ftl-no-cjit-small-pool
stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-elimination-bounds-check-barely.js.ftl-eager-no-cjit-b3o1
stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/string-concat-long-convert.js.bytecode-cache
stress/load-varargs-elimination-bounds-check.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/call-spread-call.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/call-spread-apply.js.bytecode-cache
stress/varargs-varargs-inlined-exit.js.ftl-eager
stress/construct-varargs-inline-smaller-Foo.js.ftl-no-cjit-no-put-stack-validate
stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-eager-no-cjit
stress/load-varargs-then-inlined-call-inlined.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-elimination-bounds-check-barely.js.ftl-no-cjit-no-put-stack-validate
stress/load-varargs-then-inlined-call-inlined.js.ftl-eager
microbenchmarks/string-concat-long.js.ftl-eager
microbenchmarks/call-spread-apply.js.ftl-eager-no-cjit
stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-eager-no-cjit
microbenchmarks/string-concat-long.js.bytecode-cache
stress/spread-multi-layers.js.default
stress/load-varargs-then-inlined-call-inlined.js.ftl-eager-no-cjit-b3o1
stress/construct-varargs-inline-smaller-Foo.js.ftl-eager-no-cjit-b3o1
stress/varargs-varargs-inlined-exit.js.ftl-eager-no-cjit
stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-eager-no-cjit-b3o1
stress/spread-multi-layers.js.ftl-eager
stress/spread-multi-layers.js.ftl-eager-no-cjit-b3o1
microbenchmarks/string-concat-long.js.ftl-no-cjit-b3o0
stress/load-varargs-elimination-bounds-check-barely.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/string-concat-long.js.ftl-no-cjit-small-pool
microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-b3o0
stress/proxy-call.js.ftl-eager
stress/load-varargs-elimination-bounds-check.js.ftl-eager
microbenchmarks/string-concat-long.js.ftl-eager-no-cjit
stress/load-varargs-then-inlined-call.js.ftl-no-cjit-small-pool
stress/varargs-varargs-inlined-exit.js.ftl-no-cjit-no-put-stack-validate
stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-no-cjit-no-put-stack-validate
stress/load-varargs-then-inlined-call-inlined.js.ftl-no-cjit-small-pool
stress/load-varargs-then-inlined-call.js.ftl-no-cjit-no-put-stack-validate
stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-elimination-bounds-check.js.ftl-eager-no-cjit-b3o1
microbenchmarks/string-concat-long-convert.js.default
microbenchmarks/call-spread-apply.js.ftl-eager-no-cjit-b3o1
microbenchmarks/call-spread-call.js.ftl-no-cjit-no-put-stack-validate
stress/spread-multi-layers.js.ftl-eager-no-cjit
stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/call-spread-apply.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/string-concat-long-convert.js.ftl-eager-no-cjit-b3o1
stress/proxy-call.js.ftl-eager-no-cjit-b3o1
stress/load-varargs-then-inlined-call-and-exit.js.ftl-eager-no-cjit-b3o1
stress/load-varargs-elimination-bounds-check-barely.js.ftl-eager
stress/load-varargs-elimination-bounds-check.js.ftl-no-cjit-b3o0
microbenchmarks/call-spread-apply.js.ftl-no-cjit-no-put-stack-validate
stress/construct-varargs-inline-smaller-Foo.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-elimination-bounds-check-barely.js.ftl-eager-no-cjit
stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-no-cjit-b3o0
stress/load-varargs-then-inlined-call.js.ftl-eager
stress/load-varargs-elimination-bounds-check.js.ftl-no-cjit-no-put-stack-validate
stress/spread-multi-layers.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-eager-no-cjit-b3o1
stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/string-concat-long.js.ftl-no-cjit-validate-sampling-profiler
stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-b3o0
stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-eager-no-cjit
microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-small-pool
stress/load-varargs-elimination-bounds-check.js.ftl-eager-no-cjit
microbenchmarks/call-spread-call.js.ftl-eager
stress/varargs-varargs-inlined-exit.js.ftl-no-cjit-b3o0
microbenchmarks/call-spread-call.js.default
stress/load-varargs-then-inlined-call.js.ftl-eager-no-cjit-b3o1
stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-eager-no-cjit
microbenchmarks/call-spread-apply.js.ftl-eager
microbenchmarks/call-spread-call.js.bytecode-cache
stress/proxy-call.js.ftl-eager-no-cjit
stress/load-varargs-then-inlined-call-inlined.js.ftl-eager-no-cjit
microbenchmarks/string-concat-long.js.ftl-no-cjit-no-put-stack-validate
stress/spread-multi-layers.js.ftl-no-cjit-no-put-stack-validate
stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-eager-no-cjit-b3o1
stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-no-put-stack-validate
stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-eager
stress/load-varargs-then-inlined-call-and-exit.js.ftl-eager-no-cjit
stress/load-varargs-elimination-bounds-check-barely.js.ftl-no-cjit-b3o0
stress/load-varargs-then-inlined-call.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-validate-sampling-profiler
stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-no-cjit-b3o0
apiTests

[Yusuke Suzuki]

(In reply to Build Bot from comment #4)
> Comment on attachment 374184 [details]
> proposed patch.
> 
> Attachment 374184 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12747824
> 
> New failing tests:
> microbenchmarks/string-concat-long-convert.js.ftl-eager
> microbenchmarks/string-concat-long.js.default
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-validate-
> sampling-profiler
> stress/varargs-varargs-inlined-exit.js.ftl-eager-no-cjit-b3o1
> stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-eager
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-eager
> stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-eager-no-cjit-b3o1
> microbenchmarks/string-concat-long.js.ftl-eager-no-cjit-b3o1
> microbenchmarks/string-concat-long-convert.js.ftl-eager-no-cjit
> microbenchmarks/call-spread-call.js.ftl-eager-no-cjit
> microbenchmarks/call-spread-apply.js.ftl-no-cjit-b3o0
> stress/tailCallForwardArguments.js.ftl-eager-no-cjit-b3o1
> stress/construct-varargs-inline-smaller-Foo.js.ftl-eager-no-cjit
> microbenchmarks/call-spread-apply.js.default
> stress/construct-varargs-inline-smaller-Foo.js.ftl-eager
> stress/varargs-varargs-inlined-exit.js.ftl-no-cjit-validate-sampling-profiler
> microbenchmarks/call-spread-call.js.ftl-eager-no-cjit-b3o1
> stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-eager
> microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-no-put-stack-
> validate
> stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-no-cjit-b3o0
> stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-no-
> cjit-validate-sampling-profiler
> stress/load-varargs-then-inlined-call.js.ftl-eager-no-cjit
> stress/spread-multi-layers.js.bytecode-cache
> stress/load-varargs-then-inlined-call-inlined.js.ftl-no-cjit-no-put-stack-
> validate
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-small-pool
> stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-eager
> stress/spread-multi-layers.js.ftl-no-cjit-b3o0
> microbenchmarks/call-spread-call.js.ftl-no-cjit-small-pool
> stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-no-cjit-validate-
> sampling-profiler
> stress/load-varargs-elimination-bounds-check-barely.js.ftl-eager-no-cjit-b3o1
> stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-no-cjit-no-put-stack-
> validate
> microbenchmarks/string-concat-long-convert.js.bytecode-cache
> stress/load-varargs-elimination-bounds-check.js.ftl-no-cjit-validate-
> sampling-profiler
> microbenchmarks/call-spread-call.js.ftl-no-cjit-validate-sampling-profiler
> microbenchmarks/call-spread-apply.js.bytecode-cache
> stress/varargs-varargs-inlined-exit.js.ftl-eager
> stress/construct-varargs-inline-smaller-Foo.js.ftl-no-cjit-no-put-stack-
> validate
> stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-eager-no-cjit
> stress/load-varargs-then-inlined-call-inlined.js.ftl-no-cjit-validate-
> sampling-profiler
> stress/load-varargs-elimination-bounds-check-barely.js.ftl-no-cjit-no-put-
> stack-validate
> stress/load-varargs-then-inlined-call-inlined.js.ftl-eager
> microbenchmarks/string-concat-long.js.ftl-eager
> microbenchmarks/call-spread-apply.js.ftl-eager-no-cjit
> stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-eager-no-cjit
> microbenchmarks/string-concat-long.js.bytecode-cache
> stress/spread-multi-layers.js.default
> stress/load-varargs-then-inlined-call-inlined.js.ftl-eager-no-cjit-b3o1
> stress/construct-varargs-inline-smaller-Foo.js.ftl-eager-no-cjit-b3o1
> stress/varargs-varargs-inlined-exit.js.ftl-eager-no-cjit
> stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-
> eager-no-cjit-b3o1
> stress/spread-multi-layers.js.ftl-eager
> stress/spread-multi-layers.js.ftl-eager-no-cjit-b3o1
> microbenchmarks/string-concat-long.js.ftl-no-cjit-b3o0
> stress/load-varargs-elimination-bounds-check-barely.js.ftl-no-cjit-validate-
> sampling-profiler
> microbenchmarks/string-concat-long.js.ftl-no-cjit-small-pool
> microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-b3o0
> stress/proxy-call.js.ftl-eager
> stress/load-varargs-elimination-bounds-check.js.ftl-eager
> microbenchmarks/string-concat-long.js.ftl-eager-no-cjit
> stress/load-varargs-then-inlined-call.js.ftl-no-cjit-small-pool
> stress/varargs-varargs-inlined-exit.js.ftl-no-cjit-no-put-stack-validate
> stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-no-
> cjit-no-put-stack-validate
> stress/load-varargs-then-inlined-call-inlined.js.ftl-no-cjit-small-pool
> stress/load-varargs-then-inlined-call.js.ftl-no-cjit-no-put-stack-validate
> stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-no-cjit-validate-
> sampling-profiler
> stress/load-varargs-elimination-bounds-check.js.ftl-eager-no-cjit-b3o1
> microbenchmarks/string-concat-long-convert.js.default
> microbenchmarks/call-spread-apply.js.ftl-eager-no-cjit-b3o1
> microbenchmarks/call-spread-call.js.ftl-no-cjit-no-put-stack-validate
> stress/spread-multi-layers.js.ftl-eager-no-cjit
> stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-no-cjit-
> validate-sampling-profiler
> microbenchmarks/call-spread-apply.js.ftl-no-cjit-validate-sampling-profiler
> stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-no-cjit-no-put-
> stack-validate
> microbenchmarks/string-concat-long-convert.js.ftl-eager-no-cjit-b3o1
> stress/proxy-call.js.ftl-eager-no-cjit-b3o1
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-eager-no-cjit-b3o1
> stress/load-varargs-elimination-bounds-check-barely.js.ftl-eager
> stress/load-varargs-elimination-bounds-check.js.ftl-no-cjit-b3o0
> microbenchmarks/call-spread-apply.js.ftl-no-cjit-no-put-stack-validate
> stress/construct-varargs-inline-smaller-Foo.js.ftl-no-cjit-validate-sampling-
> profiler
> stress/load-varargs-elimination-bounds-check-barely.js.ftl-eager-no-cjit
> stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-no-cjit-b3o0
> stress/load-varargs-then-inlined-call.js.ftl-eager
> stress/load-varargs-elimination-bounds-check.js.ftl-no-cjit-no-put-stack-
> validate
> stress/spread-multi-layers.js.ftl-no-cjit-validate-sampling-profiler
> stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-eager-no-cjit-
> b3o1
> stress/load-varargs-then-inlined-call-and-exit-strict.js.ftl-no-cjit-no-put-
> stack-validate
> microbenchmarks/string-concat-long.js.ftl-no-cjit-validate-sampling-profiler
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-b3o0
> stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-
> eager-no-cjit
> microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-small-pool
> stress/load-varargs-elimination-bounds-check.js.ftl-eager-no-cjit
> microbenchmarks/call-spread-call.js.ftl-eager
> stress/varargs-varargs-inlined-exit.js.ftl-no-cjit-b3o0
> microbenchmarks/call-spread-call.js.default
> stress/load-varargs-then-inlined-call.js.ftl-eager-no-cjit-b3o1
> stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-eager-no-cjit
> microbenchmarks/call-spread-apply.js.ftl-eager
> microbenchmarks/call-spread-call.js.bytecode-cache
> stress/proxy-call.js.ftl-eager-no-cjit
> stress/load-varargs-then-inlined-call-inlined.js.ftl-eager-no-cjit
> microbenchmarks/string-concat-long.js.ftl-no-cjit-no-put-stack-validate
> stress/spread-multi-layers.js.ftl-no-cjit-no-put-stack-validate
> stress/varargs-varargs-inlined-exit-strict-mode.js.ftl-eager-no-cjit-b3o1
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-no-cjit-no-put-stack-
> validate
> stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-eager
> stress/load-varargs-then-inlined-call-and-exit.js.ftl-eager-no-cjit
> stress/load-varargs-elimination-bounds-check-barely.js.ftl-no-cjit-b3o0
> stress/load-varargs-then-inlined-call.js.ftl-no-cjit-validate-sampling-
> profiler
> microbenchmarks/string-concat-long-convert.js.ftl-no-cjit-validate-sampling-
> profiler
> stress/get-argument-by-val-in-inlined-varargs-call-out-of-bounds.js.ftl-no-
> cjit-b3o0
> apiTests

I'm not sure yet why this crash is caused. But here is my rough guess.
This is caused by inconsistency between argument-count-information in DFG argument elimination phase and availability phase.
In DFG argument elimination phase, we drive more analysis to determine static argument count of LoadVarargs by leveraging inlineCallFrame information. On the other hand, availability analysis simply says it is mandatory-minimum. I'm guessing that DFG argument elimination emits nodes which heavily rely on that LoadVarargs emits N elements, which are larger than mandatory-minimum.
Without using this information in OSR availability analysis, we fail to retrieve OSR information for that nodes. For example,

LoadVarargs(mandatoryMinimum = 0, limit = 3, => @1, @2, @3)
... inlining ...
    SomeNode(...) whose OSR exit retrieves @3. And when emitting this node, we heavily assumed on that @3 exists in the stack. Based on more detailed analysis in argument elimination phase.

[Yusuke Suzuki]

And, I now understand that AI should say makeHeap for known args (that count also needs to be the count in DFG argument elimination phase), and for the other nodes, I think we can say, "(...).merge(SpecHeapTop)" instead of "makeFullTop()".

[Saam Barati]

Comment on attachment 374184 [details]
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:475
> +            m_state.operand(data->start.offset() + i).makeFullTop();

this feels weird given they can literally be any bits. Like, if we actually wrote here at runtime, HeapTop is the correct value. But if we didn’t, it’s garbage. Why not just clear the AI value. What does AI do for modeling the nodes that could actually read from these stack slots? I think that might be the GetArgumentByIndex (or similar) that has some logic for OOB argument accesses.

Wasn’t the bug here when we do tail call loop optimization to an inlined varargs frame?

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:996
> +                                // FIXME: should we use mandatoryMinimum here instead of limit?

This seems fine but also not needed.

[Yusuke Suzuki]

Another guess: The original LoadVarargs for inlinee in reflect is a bit tricky. It is initially created as LoadVarargs w/ mandatoryMinimum=1, because `inlinee` function's parameter is 1. However, we never pass the arguments object which has length >=1: arguments is always empty. So, if LoadVarargs exists, it always causes OSR exit. I think this is a bit missing case of arity-fixup + LoadVarargs. And I think we should fix it anyway: prohibiting inlining, or come up with a good idea handling this.
But in this script, we do not perform OSR exit for this. This is because we exploit that we can remove LoadVarargs in argument elimination phase. This sounds interesting, and I'm not surprised if we have a bug with arity-fixup + argument elimination + mandatoryMinimum.

[Yusuke Suzuki]

Today, we (Mark and I) found the issue in argument-elimination phase, and I now believe fixing argument-elimination phase is the right fix.

[Mark Lam]

Created attachment 374282 [details]
proposed patch.

[Mark Lam]

Thanks for the review.  Landed in r247532: <http://trac.webkit.org/r247532>.

[Saam Barati]

Comment on attachment 374282 [details]
proposed patch.

LGTM too