[strong] Function arity check should be based on required parameters

[strong] Function arity check should be based on required parameters

Also check whether the arguments count is smaller than the number of
required parameters which is the same as the SharedFunctionInfo length.

BUG=v8:4102
LOG=N
R=rossberg@chromium.org

Committed: https://crrev.com/78f0452d310221ac74b6221140d376d8302c4579
Cr-Commit-Position: refs/heads/master@{#28491}

[arv (Not doing code reviews), 2015-05-14 19:55:14]

PTAL

ia32 works but x64 does not. Do you have any idea?

[arv (Not doing code reviews), 2015-05-15 14:33:11]

Ping

[rossberg, 2015-05-18 16:35:42]

LGTM, in what way does it not work on x64?

[arv (Not doing code reviews), 2015-05-18 17:01:18]

On 2015/05/18 16:35:42, rossberg wrote:
> LGTM, in what way does it not work on x64?

Conceptually it is right but we store these things in weird ways to save space:

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.h&l...

I'm trying to figure out how to get it out. There is a
LoadSharedFunctionInfoSpecialField but that is not quite right either.

[arv (Not doing code reviews), 2015-05-18 18:06:08]

Fix x64

[arv (Not doing code reviews), 2015-05-18 18:08:17]

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

https://codereview.chromium.org/1133933003/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1663: // See comment near kLengthOffset in src/objects.h
I'm not sure I got this right for Big Endian (I only tried it on x86-x64). Who
can review this part?

[arv (Not doing code reviews), 2015-05-18 19:47:54]

arm64

[arv (Not doing code reviews), 2015-05-18 19:59:02]

mips

[arv (Not doing code reviews), 2015-05-18 20:09:22]

mips64

[arv (Not doing code reviews), 2015-05-18 20:27:25]

git rebase

[arv (Not doing code reviews), 2015-05-18 20:35:17]

Mips people, please can you take a look at mips64 in particaluar. See
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.h&l...
and the comment above at
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.h&l...

It is not clear to me if kPointerSize can ever be kInt32Size on mips64.

Same question goes for ARM64 if someone knows about that?

[paul.l..., 2015-05-18 22:04:00]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-05-18 22:04:01]

mips and mips64 ports LGTM. kPointerSize can NOT be kInt32Size on mips64. (I
think that is used only to support the x32 ABI variant on x64.)

I don't think that arm64 supports that either. On arm64, the Ldrsh() looks
suspicious to me, as that is doing a 16-bit load. Even on x64, when the pointer
size is 32 bits, these fields are still 32-bits wide. But of course I'd defer to
Rodolph or any other arm person on this.

https://codereview.chromium.org/1133933003/diff/100001/src/mips64/builtins-mi...
File src/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/1133933003/diff/100001/src/mips64/builtins-mi...
src/mips64/builtins-mips64.cc:1764: __ srl(a5, a5, 1);
A couple nits that I think are completely unimportant for this usage: lw() will
sign extend (lwu() will zero-extend). If you expect negative numbers then your
shift should be sra(). Since this is a parameter count, it won't be negative,
and it won't be close to 2^31 so I don't think either comment applies here.

[paul.l..., 2015-05-18 22:11:47]

https://codereview.chromium.org/1133933003/diff/100001/src/mips64/builtins-mi...
File src/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/1133933003/diff/100001/src/mips64/builtins-mi...
src/mips64/builtins-mips64.cc:1764: __ srl(a5, a5, 1);
On 2015/05/18 22:04:01, paul.l... wrote:
> A couple nits that I think are completely unimportant for this usage: lw()
will
> sign extend (lwu() will zero-extend). If you expect negative numbers then your
> shift should be sra(). Since this is a parameter count, it won't be negative,
> and it won't be close to 2^31 so I don't think either comment applies here.

Actually, if you expected negative numbers, you should then use dsra(), the
double-word/64-bit arithmetic shift. Still a nit for this usage.

[arv (Not doing code reviews), 2015-05-19 14:26:51]

On 2015/05/18 22:04:01, paul.l... wrote:
> mips and mips64 ports LGTM. kPointerSize can NOT be kInt32Size on mips64. (I
> think that is used only to support the x32 ABI variant on x64.)

Thanks.

> I don't think that arm64 supports that either. On arm64, the Ldrsh() looks
> suspicious to me, as that is doing a 16-bit load. Even on x64, when the
pointer
> size is 32 bits, these fields are still 32-bits wide. But of course I'd defer
to
> Rodolph or any other arm person on this.

I'll add DCHECKS to enforce this.

>
https://codereview.chromium.org/1133933003/diff/100001/src/mips64/builtins-mi...
> File src/mips64/builtins-mips64.cc (right):
> 
>
https://codereview.chromium.org/1133933003/diff/100001/src/mips64/builtins-mi...
> src/mips64/builtins-mips64.cc:1764: __ srl(a5, a5, 1);
> A couple nits that I think are completely unimportant for this usage: lw()
will
> sign extend (lwu() will zero-extend). If you expect negative numbers then your
> shift should be sra(). Since this is a parameter count, it won't be negative,
> and it won't be close to 2^31 so I don't think either comment applies here.

[Rodolph Perfetta, 2015-05-19 14:37:04]

rodolph.perfetta@gmail.com changed reviewers:
+ rodolph.perfetta@gmail.com

[Rodolph Perfetta, 2015-05-19 14:37:06]

Paul is right, you can't have 32-bit pointer in arm64.

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

https://codereview.chromium.org/1133933003/diff/100001/src/arm/builtins-arm.c...
src/arm/builtins-arm.cc:1737: __ cmp(r0, r4);
The two instructions above can be merged:

  __ cmp(r0, Operand::SmiUntag(r4));

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

https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
src/arm64/builtins-arm64.cc:1753: __ Ldrsh(scratch2,
This will load a 16bit value which is not what you meant. Load into a W register
(32-bit) should accomplish what you intended:

  __ Ldr(scratch2.W(), FieldMem...);

https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
src/arm64/builtins-arm64.cc:1756: __ Cmp(argc_actual, scratch2);
the two instructions above can be merged:

  __ Cmp(argc_actual, Operand(scratch2, LSR, 1));

https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
src/arm64/builtins-arm64.cc:1765: __ bind(&no_strong_error);
No part of your patch but we should use the macro assembler so Bind(...)

[Rodolph Perfetta, 2015-05-19 14:41:43]

On 2015/05/19 14:37:06, Rodolph Perfetta wrote:
> Paul is right, you can't have 32-bit pointer in arm64.
> 
> https://codereview.chromium.org/1133933003/diff/100001/src/arm/builtins-arm.cc
> File src/arm/builtins-arm.cc (right):
> 
>
https://codereview.chromium.org/1133933003/diff/100001/src/arm/builtins-arm.c...
> src/arm/builtins-arm.cc:1737: __ cmp(r0, r4);
> The two instructions above can be merged:
> 
>   __ cmp(r0, Operand::SmiUntag(r4));
> 
>
https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
> File src/arm64/builtins-arm64.cc (right):
> 
>
https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
> src/arm64/builtins-arm64.cc:1753: __ Ldrsh(scratch2,
> This will load a 16bit value which is not what you meant. Load into a W
register
> (32-bit) should accomplish what you intended:
> 
>   __ Ldr(scratch2.W(), FieldMem...);
> 
>
https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
> src/arm64/builtins-arm64.cc:1756: __ Cmp(argc_actual, scratch2);
> the two instructions above can be merged:
> 
>   __ Cmp(argc_actual, Operand(scratch2, LSR, 1));
> 
>
https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
> src/arm64/builtins-arm64.cc:1765: __ bind(&no_strong_error);
> No part of your patch but we should use the macro assembler so Bind(...)

One last precision, my arm64 code assumes SharedFunctionInfo::length is 32-bit
unsigned.

[arv (Not doing code reviews), 2015-05-19 14:54:45]

On 2015/05/19 14:41:43, Rodolph Perfetta wrote:
> One last precision, my arm64 code assumes SharedFunctionInfo::length is 32-bit
> unsigned.

What was that related to?

The storage of the length is a bit special on 64 bit archs. See the comment at
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.h&l...

[arv (Not doing code reviews), 2015-05-19 14:56:13]

This is awesome feedback. Makes me wish all assembly was reviewed by experts
like you.

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

https://codereview.chromium.org/1133933003/diff/100001/src/arm/builtins-arm.c...
src/arm/builtins-arm.cc:1737: __ cmp(r0, r4);
On 2015/05/19 14:37:05, Rodolph Perfetta wrote:
> The two instructions above can be merged:
> 
>   __ cmp(r0, Operand::SmiUntag(r4));

Done.

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

https://codereview.chromium.org/1133933003/diff/100001/src/arm64/builtins-arm...
src/arm64/builtins-arm64.cc:1765: __ bind(&no_strong_error);
On 2015/05/19 14:37:06, Rodolph Perfetta wrote:
> No part of your patch but we should use the macro assembler so Bind(...)

I added that bind so I'm happy to change it to Bind to get the assertion.

[arv (Not doing code reviews), 2015-05-19 14:56:59]

Added dchecks and cleaned up assembly based on feedback

[paul.l..., 2015-05-19 15:30:57]

https://codereview.chromium.org/1133933003/diff/120001/src/arm64/builtins-arm...
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/1133933003/diff/120001/src/arm64/builtins-arm...
src/arm64/builtins-arm64.cc:1755: FieldMemOperand(scratch1,
SharedFunctionInfo::kLengthOffset));
Ldrsh() should be Ldr() , per previous comment.
(SharedFunctionInfo::kLengthOffset is a 32-bit unsigned value)

[arv (Not doing code reviews), 2015-05-19 15:54:19]

Use Ldr on arm64

[arv (Not doing code reviews), 2015-05-19 15:54:38]

Thanks

https://codereview.chromium.org/1133933003/diff/120001/src/arm64/builtins-arm...
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/1133933003/diff/120001/src/arm64/builtins-arm...
src/arm64/builtins-arm64.cc:1755: FieldMemOperand(scratch1,
SharedFunctionInfo::kLengthOffset));
On 2015/05/19 15:30:56, paul.l... wrote:
> Ldrsh() should be Ldr() , per previous comment.
> (SharedFunctionInfo::kLengthOffset is a 32-bit unsigned value)

Done.

[Rodolph Perfetta, 2015-05-19 16:06:18]

On 2015/05/19 15:54:38, arv wrote:
> Thanks
> 
>
https://codereview.chromium.org/1133933003/diff/120001/src/arm64/builtins-arm...
> File src/arm64/builtins-arm64.cc (right):
> 
>
https://codereview.chromium.org/1133933003/diff/120001/src/arm64/builtins-arm...
> src/arm64/builtins-arm64.cc:1755: FieldMemOperand(scratch1,
> SharedFunctionInfo::kLengthOffset));
> On 2015/05/19 15:30:56, paul.l... wrote:
> > Ldrsh() should be Ldr() , per previous comment.
> > (SharedFunctionInfo::kLengthOffset is a 32-bit unsigned value)
> 
> Done.

arm and arm64 part looks good to me.

[arv (Not doing code reviews), 2015-05-19 18:42:25]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-05-19 18:42:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1133933003/#ps140001
(title: "Use Ldr on arm64")

[commit-bot: I haz the power, 2015-05-19 18:42:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133933003/140001

[commit-bot: I haz the power, 2015-05-19 18:44:26]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-05-19 18:44:44]

Patchset 8 (id:??) landed as
https://crrev.com/78f0452d310221ac74b6221140d376d8302c4579
Cr-Commit-Position: refs/heads/master@{#28491}