Check stack limit in ArgumentAdaptorTrampoline.

Check stack limit in ArgumentAdaptorTrampoline.
BUG=353058
LOG=N
TEST=mjsunit/regress/regress-353058
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20692

[ulan, 2014-04-08 09:41:15]

Danno, Michael, does this look good to you?

Feel free to suggest another reviewer if you know somebody who is more familiar
with argument adaptors.

[Michael Starzinger, 2014-04-08 11:58:39]

It's looking good. Do we have performance measurements of how this impacts
runtime? One optimization I could think of would be to only do the stack-check
if we actually need to adapt arguments instead of unconditionally at the
beginning, but that would complicate the code a little bit.

https://codereview.chromium.org/215853005/diff/80001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/215853005/diff/80001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:1406: __ InvokeBuiltin(Builtins::STACK_OVERFLOW,
JUMP_FUNCTION);
The assumption is that this invocation never returns, can we add an int3() or
hlt() or something like to guard that?

https://codereview.chromium.org/215853005/diff/80001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/215853005/diff/80001/src/runtime.js#newcode472
src/runtime.js:472: function STACK_OVERFLOW(length) {
nit: I don't see a particular reason why we need to keep both of these
functions. But I agree that "STACK_OVERFLOW" is a better name, can we drop
"APPLY_OVERFLOW" in favor of this one?

[ulan, 2014-04-08 14:00:55]

Thank you for review! I addressed comments and added arm64/x64.

> Do we have performance measurements of how this impacts runtime?
ia32 didn't impact the benchmarks. I will run other archs.

https://codereview.chromium.org/215853005/diff/80001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/215853005/diff/80001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:1406: __ InvokeBuiltin(Builtins::STACK_OVERFLOW,
JUMP_FUNCTION);
On 2014/04/08 11:58:40, Michael Starzinger wrote:
> The assumption is that this invocation never returns, can we add an int3() or
> hlt() or something like to guard that?

Done.

https://codereview.chromium.org/215853005/diff/80001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/215853005/diff/80001/src/runtime.js#newcode472
src/runtime.js:472: function STACK_OVERFLOW(length) {
On 2014/04/08 11:58:40, Michael Starzinger wrote:
> nit: I don't see a particular reason why we need to keep both of these
> functions. But I agree that "STACK_OVERFLOW" is a better name, can we drop
> "APPLY_OVERFLOW" in favor of this one?

Done. Removed APPLY_OVERFLOW

[Michael Starzinger, 2014-04-11 08:33:09]

LGTM.

https://codereview.chromium.org/215853005/diff/100001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/215853005/diff/100001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:1410: 
nit: Only two empty newlines here.

[ulan, 2014-04-11 13:25:56]

https://codereview.chromium.org/215853005/diff/100001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/215853005/diff/100001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:1410: 
On 2014/04/11 08:33:10, Michael Starzinger wrote:
> nit: Only two empty newlines here.

Done. Landing.

[ulan, 2014-04-11 13:39:44]

Committed patchset #5 manually as r20692 (presubmit successful).