We shouldn't promote LoadVarargs to a sequence of GetStacks and PutStacks if doing...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 May 2015 18:18:10 +0000 (18:18 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 May 2015 18:18:10 +0000 (18:18 +0000)
commitcb5b2fc83520ac7782a3b37c1de5caa49765eabf
treece73f6cbf50cb0a550a78c98d8bfa170ecfdd14f
parent3c49b296fa409caa0f7a6031b4dd528ab13ffd06
We shouldn't promote LoadVarargs to a sequence of GetStacks and PutStacks if doing so would exceed the LoadVarargs' limit
https://bugs.webkit.org/show_bug.cgi?id=144851

Reviewed by Michael Saboff.
Source/JavaScriptCore:

LoadVarargs loads arguments from some object and puts them on the stack. The region of
stack is controlled by a bunch of meta-data, including InlineCallFrame. InlineCallFrame
shouldn't really be edited after ByteCodeParser, so we cannot convert LoadVarargs to
something that uses more stack than the LoadVarargs wanted to.

This check was missing in the ArgumentsEliminationPhase's LoadVarargs->GetStack+PutStack
promoter. This is an important promotion rule for performance, and in cases where we are
compiling truly hot code, the LoadVarargs limit will be at least as big as the length of
the phantom arguments array that this phase sees. The LoadVarargs limit is based on
profiling and the phantom arguments array is a proof; in most cases the profiling is more
conservative.

But, you could write some crazy code where the statically obvious arguments array value is
bigger than what the profiling would have told you. When this happens, this promotion
effectively removes a bounds check. This either results in us clobbering a bunch of stack,
or it means that we never initialize a region of the stack that a later operation will read
(the uninitialization happens because PutStackSinkingPhase removes PutStacks that appear
unnecessary, and a GetMyArgumentByVal will claim not to use the region of the stack outside
the original LoadVarargs limit).

* dfg/DFGArgumentsEliminationPhase.cpp:
* tests/stress/load-varargs-elimination-bounds-check-barely.js: Added.
(foo):
(bar):
(baz):
* tests/stress/load-varargs-elimination-bounds-check.js: Added.
(foo):
(bar):
(baz):

LayoutTests:

* js/regress/load-varargs-elimination-expected.txt: Added.
* js/regress/load-varargs-elimination.html: Added.
* js/regress/script-tests/load-varargs-elimination.js: Added.
(foo):
(bar):
(baz):
* js/regress/sink-huge-activation-expected.txt: Added.
* js/regress/sink-huge-activation.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@184110 268f45cc-cd09-0410-ab3c-d52691b4dbfc
LayoutTests/ChangeLog
LayoutTests/js/regress/load-varargs-elimination-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/load-varargs-elimination.html [new file with mode: 0644]
LayoutTests/js/regress/script-tests/load-varargs-elimination.js [new file with mode: 0644]
LayoutTests/js/regress/sink-huge-activation-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/sink-huge-activation.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
Source/JavaScriptCore/tests/stress/load-varargs-elimination-bounds-check-barely.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/load-varargs-elimination-bounds-check.js [new file with mode: 0644]