[strong] Check arity of functions

[strong] Check arity of functions

In strong mode it is an error to call a function with too few
arguments.

This is enforced inside the ArgumentsAdaptorTrampoline.

This does not yet handle rest parameters

BUG=v8:3956
LOG=N
R=rossberg@chromium.org, dslomov@chromium.org

Committed: https://crrev.com/3226e980200035f37c86fef4b11fb307e17764a2
Cr-Commit-Position: refs/heads/master@{#28346}

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

PTAL

Other ports will be done once this approach has been approved of.

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

arv@chromium.org changed reviewers:
+ mstarzinger@chromium.org

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

+ mstarzinger for input on what needs to be done (if anything) for TF.

[Michael Starzinger, 2015-05-04 08:27:04]

The only thing I can think of right now is that the JSInliner needs to be
adapted to not inline in the cases that a strong-mode callee would throw.

[Michael Starzinger, 2015-05-04 09:50:24]

Can we also add some tests for constructor calls with mismatching arguments
counts?

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

Add construct tests

[arv (Not doing code reviews), 2015-05-04 15:59:58]

Do not inline in case of too few arguments

[arv (Not doing code reviews), 2015-05-04 16:08:26]

cleanup

[rossberg, 2015-05-05 14:26:20]

https://codereview.chromium.org/1115263004/diff/60001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1115263004/diff/60001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:368: if (is_strong(info.language_mode()) &&
Is this checking the right language mode? It is the mode of the inlinee that is
relevant here.

https://codereview.chromium.org/1115263004/diff/60001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1115263004/diff/60001/src/messages.js#newcode185
src/messages.js:185: strong_mode_too_few_arguments: ["In strong mode, calling a
function with too few arguments is deprecated"],
Nit: strong_arity

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:214: func4(1, 2); // FIXME
Why FIXME?

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
test/mjsunit/strong/function-arity.js:15: f.call(undefined);
Perhaps add f.call() and f.apply() (here and elsewhere)

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
test/mjsunit/strong/function-arity.js:23: f.apply(undefined, [undefined]);
Also add tests for .bind, %Call, %Apply, %_CallFunction  (here and elsewhere).
And we should have cctests exercising invocation through the API, because that
might take yet more paths. Cross-language-mode calls are also test-worthy,
especially with cross-mode inlining (that has been a tricky issue with the
implicit conversion changes, for example).

[Michael Starzinger, 2015-05-05 14:31:16]

https://codereview.chromium.org/1115263004/diff/60001/src/compiler/js-inlinin...
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1115263004/diff/60001/src/compiler/js-inlinin...
src/compiler/js-inlining.cc:368: if (is_strong(info.language_mode()) &&
On 2015/05/05 14:26:19, rossberg wrote:
> Is this checking the right language mode? It is the mode of the inlinee that
is
> relevant here.

Yes, I think it is using the correct CompilationInfo here. Here "info" refers to
the the inlinee (see lines 324 and 325 above). Maybe making that explicit by
lifting the language mode out into a local variable would avoid that confusion.
On the other hand, hoisting to a local variable needs to happen _after_ the
Compiler::ParseAndAnalyze step.

[arv (Not doing code reviews), 2015-05-05 16:46:17]

Addded tons of more tests

[arv (Not doing code reviews), 2015-05-05 16:46:29]

PTAL

https://codereview.chromium.org/1115263004/diff/60001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1115263004/diff/60001/src/messages.js#newcode185
src/messages.js:185: strong_mode_too_few_arguments: ["In strong mode, calling a
function with too few arguments is deprecated"],
On 2015/05/05 14:26:19, rossberg wrote:
> Nit: strong_arity

Done.

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/dec...
File test/mjsunit/strong/declaration-after-use.js (right):

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/dec...
test/mjsunit/strong/declaration-after-use.js:214: func4(1, 2); // FIXME
On 2015/05/05 14:26:20, rossberg wrote:
> Why FIXME?

Let me make that more explicit.

The arity checking is not correct when spread params are present. The number of
required arguments for func4 should be 1 and not 2.

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
test/mjsunit/strong/function-arity.js:15: f.call(undefined);
On 2015/05/05 14:26:20, rossberg wrote:
> Perhaps add f.call() and f.apply() (here and elsewhere)

Done.

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
test/mjsunit/strong/function-arity.js:23: f.apply(undefined, [undefined]);
On 2015/05/05 14:26:20, rossberg wrote:
> Also add tests for .bind, %Call, %Apply, %_CallFunction  (here and elsewhere).
> And we should have cctests exercising invocation through the API, because that
> might take yet more paths. Cross-language-mode calls are also test-worthy,
> especially with cross-mode inlining (that has been a tricky issue with the
> implicit conversion changes, for example).

Your wish is my command.

Done.

If you can think of any more tests I'm happy to add even more of'em.

[rossberg, 2015-05-05 16:51:57]

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/60001/test/mjsunit/strong/fun...
test/mjsunit/strong/function-arity.js:23: f.apply(undefined, [undefined]);
On 2015/05/05 14:26:20, rossberg wrote:
> Also add tests for .bind, %Call, %Apply, %_CallFunction  (here and elsewhere).
> And we should have cctests exercising invocation through the API, because that
> might take yet more paths. Cross-language-mode calls are also test-worthy,
> especially with cross-mode inlining (that has been a tricky issue with the
> implicit conversion changes, for example).

Oh, and super calls. :)

[arv (Not doing code reviews), 2015-05-05 17:12:58]

Added and disabled super tests

[arv (Not doing code reviews), 2015-05-05 17:13:36]

So super is broken in strong mode...

I'll also add some spread call tests after lunch...

[arv (Not doing code reviews), 2015-05-05 18:07:53]

moar tests

[rossberg, 2015-05-06 10:31:25]

On 2015/05/05 17:13:36, arv wrote:
> So super is broken in strong mode...
> 
> I'll also add some spread call tests after lunch...

Awesome tests, thanks!

Though what I actually meant by "super calls" is super constructor calls, which
might be extra hairy because of the new.target hack. A particular subcase
perhaps is default constructors.

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

Fix Crankshaft and change to use codegen for tests

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

PTAL

Is this ready for other ports?

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

Fix style nits

[rossberg, 2015-05-07 08:29:17]

Yes, looks good, ready for ports.

https://codereview.chromium.org/1115263004/diff/160001/test/mjsunit/strong/fu...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/160001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:5: // Flags: --strong-mode
--harmony-arrow-functions --harmony-reflect --harmony-spreadcalls
--allow-natives-syntax
Nit: line length (note that you can have multiple Flags: comments)

https://codereview.chromium.org/1115263004/diff/160001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:46: 'use strong';
Instead of having the directive outside the function, the more tricky case is
perhaps having it inside the function (consider lazy parsing). So I'd move it
into defs.

[rossberg, 2015-05-07 09:33:11]

Actually, we should have a few explicit tests for optimised functions.

[rossberg, 2015-05-07 09:35:03]

On 2015/05/07 09:33:11, rossberg wrote:
> Actually, we should have a few explicit tests for optimised functions.

...especially since assertThrows when passed a string will use eval, and eval
disables most optimisations.

[arv (Not doing code reviews), 2015-05-08 00:39:59]

Ports done and even more tests

[arv (Not doing code reviews), 2015-05-08 00:45:42]

PTAL

[arv (Not doing code reviews), 2015-05-08 15:24:21]

Ping

[rossberg, 2015-05-11 15:39:28]

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:5: // Flags: --strong-mode
--harmony-arrow-functions --harmony-reflect --harmony-spreadcalls
--allow-natives-syntax
Nit: line length

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:269: function f(x, y) {}
It's also worth testing the case where the callee is weak (and does not throw).

[rossberg, 2015-05-11 15:40:57]

Forgot the LGTM modulo last comments

[arv (Not doing code reviews), 2015-05-11 16:51:45]

more tests

[arv (Not doing code reviews), 2015-05-11 16:54:02]

Thanks for the review Andreas. 

All done, CQ'ing...

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:5: // Flags: --strong-mode
--harmony-arrow-functions --harmony-reflect --harmony-spreadcalls
--allow-natives-syntax
On 2015/05/11 15:39:28, rossberg wrote:
> Nit: line length

Done.

I fixed this but I guess I undid too much at one point...

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:269: function f(x, y) {}
On 2015/05/11 15:39:28, rossberg wrote:
> It's also worth testing the case where the callee is weak (and does not
throw).

Done.

[arv (Not doing code reviews), 2015-05-11 16:54:09]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-05-11 16:54:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1115263004/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-05-11 16:54:30]

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

[commit-bot: I haz the power, 2015-05-11 17:20:51]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-05-11 17:21:10]

Patchset 12 (id:??) landed as
https://crrev.com/3226e980200035f37c86fef4b11fb307e17764a2
Cr-Commit-Position: refs/heads/master@{#28346}

[rossberg, 2015-05-12 06:02:16]

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
File test/mjsunit/strong/function-arity.js (right):

https://codereview.chromium.org/1115263004/diff/180001/test/mjsunit/strong/fu...
test/mjsunit/strong/function-arity.js:269: function f(x, y) {}
On 2015/05/11 16:54:02, arv wrote:
> On 2015/05/11 15:39:28, rossberg wrote:
> > It's also worth testing the case where the callee is weak (and does not
> throw).
> 
> Done.

Sorry for the misunderstanding, I meant the case where a strong function calls a
weak one, i.e., variations of:

function f(x, y) {}
function g() { 'use strong'; f(1) }

which should not throw, of course.