Fix arguments object materialization during deopt.

Fix arguments object materialization during deopt.

This fixes materialization of arguments objects for strict mode functions during
deoptimization. We materialize arguments from the stack area where optimized
code pushes the arguments when entering the inlined environment. For adapted
invocations we use the arguments adaptor frame for materialization.

R=svenpanne@chromium.org
BUG=v8:2261
TEST=mjsunit/regress/regress-2261,mjsunit/compiler/inline-arguments

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

[Michael Starzinger, 2012-09-11 10:45:38]

The CL only contains the ia32 port so far, but I wanted to get a first round of
comments before porting to other architectures.

[Michael Starzinger, 2012-09-11 11:46:42]

Ported to other architectures.

[Sven Panne, 2012-09-12 07:14:22]

LGTM if nits are addressed and test cases extended.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
src/arm/lithium-codegen-arm.cc:468: int* args_index,
Nit: args_foo => arguments_foo for consistency here and at similar places.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
File src/arm/lithium-codegen-arm.h (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
src/arm/lithium-codegen-arm.h:152: int* arguments_index,
I think later it will be very hard to understand what arguments_index actually
is, so we should document this somehow, but I am not sure where.

Perhaps arguments_offset might be a better name? I'll leave this up to you, but
we need some kind of comment in any case.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#...
src/deoptimizer.cc:679: // Materialize all heap numbers before looking at
arguments.
Describe why this is necessary to do before arguments objects materialization.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#...
src/deoptimizer.cc:718: if (!frame->has_adapted_arguments()) {
Can rearrange things a bit here so that we have a single 'if'?

   if (frame->has_adapted_arguments()) {
      ...
   } else {
      ...
   }

Seeing the different cases is a bit hard currently, but this might be due to the
inherent complexity of the problem. :-)

https://chromiumcodereview.appspot.com/10908194/diff/3001/test/mjsunit/regres...
File test/mjsunit/regress/regress-2261.js (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/test/mjsunit/regres...
test/mjsunit/regress/regress-2261.js:32: 
As was dicussed already offline, it would be good to have test cases which
involve an arguments adaptor (both with too few/to many arguments), even if it
is not easily possible to do without extensive copy-n-paste. I would very much
prefer a broader test coverage for such a tricky area over copy-n-paste-free
code. :-)

Probably these tests should live in mjsunit/compiler then.

[Michael Starzinger, 2012-09-12 10:07:28]

Addressed most of the comments. Will now work on improving test coverage.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
src/arm/lithium-codegen-arm.cc:468: int* args_index,
On 2012/09/12 07:14:22, Sven Panne wrote:
> Nit: args_foo => arguments_foo for consistency here and at similar places.

Done.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
File src/arm/lithium-codegen-arm.h (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-cod...
src/arm/lithium-codegen-arm.h:152: int* arguments_index,
On 2012/09/12 07:14:22, Sven Panne wrote:
> I think later it will be very hard to understand what arguments_index actually
> is, so we should document this somehow, but I am not sure where.
> 
> Perhaps arguments_offset might be a better name? I'll leave this up to you,
but
> we need some kind of comment in any case.

Done. I added the documentation into LCodeGen::WriteTranslation where it's
actually used. About the name, I would still like to keep it named index,
because it is an operand index (same as LOperand::index) and follows the same
rules. That's in sync with the usage of "index" throughout lithium.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#...
src/deoptimizer.cc:679: // Materialize all heap numbers before looking at
arguments.
On 2012/09/12 07:14:22, Sven Panne wrote:
> Describe why this is necessary to do before arguments objects materialization.

Done.

https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#...
src/deoptimizer.cc:718: if (!frame->has_adapted_arguments()) {
On 2012/09/12 07:14:22, Sven Panne wrote:
> Can rearrange things a bit here so that we have a single 'if'?
> 
>    if (frame->has_adapted_arguments()) {
>       ...
>    } else {
>       ...
>    }
> 
> Seeing the different cases is a bit hard currently, but this might be due to
the
> inherent complexity of the problem. :-)

Done. I removed the static helper method completely and merged it into this
method. It is now clearer when we use the output frame and when we use the
deoptimization info to materialize the arguments object.

[Michael Starzinger, 2012-09-12 12:04:25]

Improved test coverage. Landing.

https://chromiumcodereview.appspot.com/10908194/diff/3001/test/mjsunit/regres...
File test/mjsunit/regress/regress-2261.js (right):

https://chromiumcodereview.appspot.com/10908194/diff/3001/test/mjsunit/regres...
test/mjsunit/regress/regress-2261.js:32: 
On 2012/09/12 07:14:22, Sven Panne wrote:
> As was dicussed already offline, it would be good to have test cases which
> involve an arguments adaptor (both with too few/to many arguments), even if it
> is not easily possible to do without extensive copy-n-paste. I would very much
> prefer a broader test coverage for such a tricky area over copy-n-paste-free
> code. :-)
> 
> Probably these tests should live in mjsunit/compiler then.

Done. Added a generic test case to mjsunit/compiler/inline-arguments that tests
all combinations of arguments adapters for two-level inlining.