Reland r20692 "Check stack limit in ArgumentAdaptorTrampoline."

Reland r20692 "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=20751

[ulan, 2014-04-14 13:27:01]

One doesn't simply jump to a function in the arguments adaptor trampoline. :)

We need to put the return address into the stack. Otherwise, the frame is off by
one word, and the stack frame iterator gets confused.

The fix is in diff between patch set 2 and patch set 1. PTAL.

[Michael Starzinger, 2014-04-14 14:04:45]

LGTM.

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

Committed patchset #3 manually as r20751 (presubmit successful).

[haitao.feng, 2014-04-17 07:42:55]

https://codereview.chromium.org/236633006/diff/10008/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/236633006/diff/10008/src/x64/builtins-x64.cc#...
src/x64/builtins-x64.cc:1343: __ PositiveSmiTimesPowerOfTwoToInteger64(rdx, rax,
kPointerSizeLog2);
It seems that rax and rbx are raw integers, instead of SMI. Should this be:

"
  __ movp(rdx, rbx);
  __ shlp(rdx, Immediate(kPointerSizeLog2));
"
as other ports did?

Another comment is that rbx might be -1
(SharedFunctionInfo::kDontAdaptArgumentsSentinel), we might need to emit a check
before "__ movp(rdx, rbx);", I would suggest:
  __ subp(rcx, rdx);
+  __ testp(rcx, rcx);                              
+  __ j(negative, stack_overflow);
+  __ movp(rdx, rbx);
+  __ shlp(rdx, Immediate(kPointerSizeLog2));
  __ cmpp(rcx, rdx);
  __ j(less_equal, stack_overflow);

[ulan, 2014-04-17 08:32:35]

> It seems that rax and rbx are raw integers, instead of SMI.
I think rax is SMI, but you're right that we should use rbx (which is a raw int)
here to be consistent with other architectures.

I will upload a CL with this change:
  __ movp(rdx, rbx);
  __ shlp(rdx, Immediate(kPointerSizeLog2));

> Another comment is that rbx might be -1
This should be fine since we do signed comparison of rcx and rdx.

[haitao.feng, 2014-04-17 08:48:24]

On 2014/04/17 08:32:35, ulan wrote:
> > It seems that rax and rbx are raw integers, instead of SMI.
> I think rax is SMI, but you're right that we should use rbx (which is a raw
int)
From CallDescriptors::InitializeForIsolate in the code-stub-x64.cc, the rax'
representation is Representation::Integer32().

> here to be consistent with other architectures.
> 
> I will upload a CL with this change:
>   __ movp(rdx, rbx);
>   __ shlp(rdx, Immediate(kPointerSizeLog2));
> 
> > Another comment is that rbx might be -1
> This should be fine since we do signed comparison of rcx and rdx.

There is a corner case when rbx is -1, rdx is -8, and rcx might be -1, -2, -3,
-4, ...

[ulan, 2014-04-17 09:22:47]

> From CallDescriptors::InitializeForIsolate in the code-stub-x64.cc, the rax'
> representation is Representation::Integer32().
You're absolutely right, rax is raw integer. I was confused by another code.

> There is a corner case when rbx is -1, rdx is -8, and rcx might be -1, -2, -3,
The intention is to check against big number of arguments that overflow the
stack. Since we are not accounting for return address and frame setup anyway, it
can happen that we overflow the stack by small constant amount. That should be
safe and in my opinion is not worth slowing down the common code path with
additional checks.

I uploaded the fix: https://codereview.chromium.org/239703012/

Thanks for finding this bug!

[haitao.feng, 2014-04-17 11:05:34]

On 2014/04/17 09:22:47, ulan wrote:
> > From CallDescriptors::InitializeForIsolate in the code-stub-x64.cc, the rax'
> > representation is Representation::Integer32().
> You're absolutely right, rax is raw integer. I was confused by another code.
> 
> > There is a corner case when rbx is -1, rdx is -8, and rcx might be -1, -2,
-3,
> The intention is to check against big number of arguments that overflow the
> stack. Since we are not accounting for return address and frame setup anyway,
it
> can happen that we overflow the stack by small constant amount. That should be
> safe and in my opinion is not worth slowing down the common code path with
> additional checks.
I see. Thanks for the explanation.
 
> Thanks for finding this bug!
You are welcome!

[jbramley, 2014-04-24 08:39:45]

https://codereview.chromium.org/236633006/diff/10008/src/arm64/builtins-arm64.cc
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/236633006/diff/10008/src/arm64/builtins-arm64...
src/arm64/builtins-arm64.cc:1418: __ Mov(x11, jssp);
This move doesn't look necessary.

https://codereview.chromium.org/236633006/diff/10008/src/arm64/builtins-arm64...
src/arm64/builtins-arm64.cc:1586: __ Brk(0);
'__ Unreachable()' may be more appropriate.

[ulan, 2014-04-24 08:46:53]

On 2014/04/24 08:39:45, jbramley wrote:
>
https://codereview.chromium.org/236633006/diff/10008/src/arm64/builtins-arm64.cc
> File src/arm64/builtins-arm64.cc (right):
> 
>
https://codereview.chromium.org/236633006/diff/10008/src/arm64/builtins-arm64...
> src/arm64/builtins-arm64.cc:1418: __ Mov(x11, jssp);
> This move doesn't look necessary.
> 
>
https://codereview.chromium.org/236633006/diff/10008/src/arm64/builtins-arm64...
> src/arm64/builtins-arm64.cc:1586: __ Brk(0);
> '__ Unreachable()' may be more appropriate.

Thanks! I uploaded fix: https://codereview.chromium.org/252483002/