<rdar://problem/52452328>
Created attachment 374184 [details] proposed patch.
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.
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).
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
(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.
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()".
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.
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.
Today, we (Mark and I) found the issue in argument-elimination phase, and I now believe fixing argument-elimination phase is the right fix.
Created attachment 374282 [details] proposed patch.
Thanks for the review. Landed in r247532: <http://trac.webkit.org/r247532>.
Comment on attachment 374282 [details] proposed patch. LGTM too