Set the current context to the function's context when entering to LAP.

Set the current context to the function's context when entering to LAP.

In case of LAP(lazy accessor pair), the function's creation context
must be equal to the accessor holder's creation context, so this CL
changes the current context to the accessor holder's creation context.

BUG=v8:6156
Review-Url: https://codereview.chromium.org/2770003002
Cr-Commit-Position: refs/heads/master@{#46406}
Committed: https://chromium.googlesource.com/v8/v8/+/18e73287dc65452bc2f952fc005b2251fc32c15a

[jochen (gone - plz use gerrit), 2017-03-24 15:10:14]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-03-24 15:10:15]

looks good overall!

https://codereview.chromium.org/2770003002/diff/40001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/40001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:2843: if (this->is_lazy()) {
you could move this entire block before like 2839, then you can use "scratch"

[Yuki, 2017-03-28 14:16:58]

https://codereview.chromium.org/2770003002/diff/40001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/40001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:2843: if (this->is_lazy()) {
On 2017/03/24 15:10:15, jochen wrote:
> you could move this entire block before like 2839, then you can use "scratch"

Done.

https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:2841: __ movp(scratch, FieldOperand(holder,
HeapObject::kMapOffset));
|holder| seems pointing to the receiver object instead of the prototype object. 
Is this expected?

I'd like to get the prototype object's creation context.  But, I'm not sure how
to get it...

NamedLoadHandlerCompiler::CompileLoadCallback() seems having |holder|, which is
passed to GenerateApiAccessorCall as |accessor_holder|.
https://cs.chromium.org/chromium/src/v8/src/ic/handler-compiler.cc?rcl=9e60c6...

PropertyHandlerCompiler::GenerateApiAccessorCall() seems looking up a holder,
but the result is CallOptimization::kHolderIsReceiver.
https://cs.chromium.org/chromium/src/v8/src/ic/x64/handler-compiler-x64.cc?rc...

Do you have any clues?

[jochen (gone - plz use gerrit), 2017-03-28 14:43:39]

https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:2841: __ movp(scratch, FieldOperand(holder,
HeapObject::kMapOffset));
On 2017/03/28 at 14:16:58, Yuki wrote:
> |holder| seems pointing to the receiver object instead of the prototype
object.  Is this expected?
> 
> I'd like to get the prototype object's creation context.  But, I'm not sure
how to get it...
> 
> NamedLoadHandlerCompiler::CompileLoadCallback() seems having |holder|, which
is passed to GenerateApiAccessorCall as |accessor_holder|.
>
https://cs.chromium.org/chromium/src/v8/src/ic/handler-compiler.cc?rcl=9e60c6...
> 
> PropertyHandlerCompiler::GenerateApiAccessorCall() seems looking up a holder,
but the result is CallOptimization::kHolderIsReceiver.
>
https://cs.chromium.org/chromium/src/v8/src/ic/x64/handler-compiler-x64.cc?rc...
> 
> Do you have any clues?

is that only the case for your test? In that case, I think you need to install a
signature on the accessor

[Yuki, 2017-03-29 14:19:41]

https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:2841: __ movp(scratch, FieldOperand(holder,
HeapObject::kMapOffset));
On 2017/03/28 14:43:39, jochen wrote:
> On 2017/03/28 at 14:16:58, Yuki wrote:
> > |holder| seems pointing to the receiver object instead of the prototype
> object.  Is this expected?
> > 
> > I'd like to get the prototype object's creation context.  But, I'm not sure
> how to get it...
> > 
> > NamedLoadHandlerCompiler::CompileLoadCallback() seems having |holder|, which
> is passed to GenerateApiAccessorCall as |accessor_holder|.
> >
>
https://cs.chromium.org/chromium/src/v8/src/ic/handler-compiler.cc?rcl=9e60c6...
> > 
> > PropertyHandlerCompiler::GenerateApiAccessorCall() seems looking up a
holder,
> but the result is CallOptimization::kHolderIsReceiver.
> >
>
https://cs.chromium.org/chromium/src/v8/src/ic/x64/handler-compiler-x64.cc?rc...
> > 
> > Do you have any clues?
> 
> is that only the case for your test? In that case, I think you need to install
a
> signature on the accessor

I only tested with the new unittest.  I installed the signature on the accessor
(in PS5), but no luck.  It still fails.

By the way, in Blink, info.Holder() points to the receiver object instead of the
prototype object.  Is |holder| here equivalent to
FunctionCallbackInfo::Holder(), or a different thing?

[jochen (gone - plz use gerrit), 2017-03-29 14:22:48]

On 2017/03/29 at 14:19:41, yukishiino wrote:
>
https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64.cc
> File src/x64/code-stubs-x64.cc (right):
> 
>
https://codereview.chromium.org/2770003002/diff/60001/src/x64/code-stubs-x64....
> src/x64/code-stubs-x64.cc:2841: __ movp(scratch, FieldOperand(holder,
HeapObject::kMapOffset));
> On 2017/03/28 14:43:39, jochen wrote:
> > On 2017/03/28 at 14:16:58, Yuki wrote:
> > > |holder| seems pointing to the receiver object instead of the prototype
> > object.  Is this expected?
> > > 
> > > I'd like to get the prototype object's creation context.  But, I'm not
sure
> > how to get it...
> > > 
> > > NamedLoadHandlerCompiler::CompileLoadCallback() seems having |holder|,
which
> > is passed to GenerateApiAccessorCall as |accessor_holder|.
> > >
> >
https://cs.chromium.org/chromium/src/v8/src/ic/handler-compiler.cc?rcl=9e60c6...
> > > 
> > > PropertyHandlerCompiler::GenerateApiAccessorCall() seems looking up a
holder,
> > but the result is CallOptimization::kHolderIsReceiver.
> > >
> >
https://cs.chromium.org/chromium/src/v8/src/ic/x64/handler-compiler-x64.cc?rc...
> > > 
> > > Do you have any clues?
> > 
> > is that only the case for your test? In that case, I think you need to
install a
> > signature on the accessor
> 
> I only tested with the new unittest.  I installed the signature on the
accessor (in PS5), but no luck.  It still fails.
> 
> By the way, in Blink, info.Holder() points to the receiver object instead of
the prototype object.  Is |holder| here equivalent to
FunctionCallbackInfo::Holder(), or a different thing?

it's the same thing.

Can you use the debugger for the first case where you hit the C++ code and try
to track down why it's not using the signature to find the holder?

[jochen (gone - plz use gerrit), 2017-04-21 16:06:12]

I prepared a CL to add documentation for the info.This() and info.Holder().

So the problem is that "holder" is basically the same as "this" for the API. To
get the internal holder, we'll need to pass it as an additional argument.

Toon tells me that he's currently rewriting the API call helper in C++, so once
that's done, it will be easier to pass in additional arguments

[Yuki, 2017-04-25 09:03:43]

On 2017/04/21 16:06:12, jochen (slow until May 2) wrote:
> I prepared a CL to add documentation for the info.This() and info.Holder().
> 
> So the problem is that "holder" is basically the same as "this" for the API.
To
> get the internal holder, we'll need to pass it as an additional argument.
> 
> Toon tells me that he's currently rewriting the API call helper in C++, so
once
> that's done, it will be easier to pass in additional arguments

Thanks for the update and I'm sorry that I haven't had time for this work these
weeks.
It's a great news for us.  Hope that I'll resume my work once Toon makes that
change.

[Yuki, 2017-06-14 09:13:35]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 09:13:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-06-14 09:26:01]

https://codereview.chromium.org/2770003002/diff/120001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/120001/src/x64/code-stubs-x64...
src/x64/code-stubs-x64.cc:2758: if (this->is_lazy()) {
maybe we can just enter the correct context before invoking this stub, i.e., in
https://cs.chromium.org/chromium/src/v8/src/ic/x64/handler-compiler-x64.cc?rc...
we still have the accessor_holder so we could just load the context from there
(in the lazy case) or from the callee (in the non-lazy case).

Do you want to give that a try?

[commit-bot: I haz the power, 2017-06-14 09:30:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 09:30:37]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[Yuki, 2017-06-14 13:34:52]

https://codereview.chromium.org/2770003002/diff/120001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/120001/src/x64/code-stubs-x64...
src/x64/code-stubs-x64.cc:2758: if (this->is_lazy()) {
On 2017/06/14 09:26:01, jochen wrote:
> maybe we can just enter the correct context before invoking this stub, i.e.,
in
>
https://cs.chromium.org/chromium/src/v8/src/ic/x64/handler-compiler-x64.cc?rc...
> we still have the accessor_holder so we could just load the context from there
> (in the lazy case) or from the callee (in the non-lazy case).
> 
> Do you want to give that a try?

I gave it a shot in PS8, but I failed so far.  Could you take a look at PS8 and
help me understand what's wrong?

I understand your suggestion to get the context from |accessor_holder|, but I
first tried to simply move the code to
PropertyHandlerCompiler::GenerateApiAccessorCall(), and it didn't work :( 
Replacing |holder| with |accessor_holder| didn't work, neither.

[Toon Verwaest, 2017-06-19 08:45:04]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2017-06-19 08:45:05]

https://codereview.chromium.org/2770003002/diff/140001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/140001/src/x64/code-stubs-x64...
src/x64/code-stubs-x64.cc:2739: __ Push(context);
This is now the context after loading the LAP context though, so we won't
restore correctly after the call returns below (&context_restore_operand).

[Yuki, 2017-06-20 14:35:13]

https://codereview.chromium.org/2770003002/diff/140001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/140001/src/x64/code-stubs-x64...
src/x64/code-stubs-x64.cc:2739: __ Push(context);
On 2017/06/19 08:45:05, Toon Verwaest wrote:
> This is now the context after loading the LAP context though, so we won't
> restore correctly after the call returns below (&context_restore_operand).

Ah, I see.  I confirmed that it worked locally.
Let me revise the CL tomorrow.

[Yuki, 2017-06-21 14:18:30]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 14:18:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 14:19:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 14:19:43]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/24465)

[Yuki, 2017-06-21 14:20:18]

Thank you Toon and Jochen.  Now this CL is passing the unittest even in the case
that the receiver object's context is different from the accessor holder's
context.

Could you guys take another look?

[Yuki, 2017-06-21 15:14:29]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 15:14:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 15:22:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 15:22:03]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/29497)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/29584)

[Toon Verwaest, 2017-06-21 19:07:55]

Overall this looks good. Feel free to port to the other architectures.

[Yuki, 2017-06-22 09:51:59]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 09:52:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 10:18:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 10:18:01]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/29633)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Yuki, 2017-06-22 10:48:08]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 10:48:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 10:53:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 10:53:02]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[Yuki, 2017-06-22 10:56:48]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 10:56:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-22 10:58:59]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 10:59:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 11:03:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 11:03:32]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[Yuki, 2017-06-22 12:54:06]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 12:54:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 12:57:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 12:57:22]

Dry run: Try jobs failed on following builders:
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[Yuki, 2017-06-22 13:05:58]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 13:06:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-22 13:08:12]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 13:08:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-22 13:08:28]

Patchset #12 (id:220001) has been deleted

[Yuki, 2017-06-22 13:08:45]

Patchset #12 (id:240001) has been deleted

[Yuki, 2017-06-22 13:09:05]

Patchset #12 (id:260001) has been deleted

[commit-bot: I haz the power, 2017-06-22 13:13:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 13:13:32]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[Yuki, 2017-06-22 14:07:28]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 14:07:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 14:13:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 14:13:49]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[Yuki, 2017-06-22 14:46:52]

Patchset #12 (id:280001) has been deleted

[Yuki, 2017-06-22 14:47:04]

Patchset #12 (id:300001) has been deleted

[Yuki, 2017-06-22 14:47:15]

Patchset #12 (id:320001) has been deleted

[Yuki, 2017-06-22 14:47:26]

Patchset #12 (id:340001) has been deleted

[Yuki, 2017-06-22 14:52:53]

Hi Toon and Jochen,

I have a crash issue with this CL.  Could you help me understand the cause?

https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-com...
File src/ic/x64/handler-compiler-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-com...
src/ic/x64/handler-compiler-x64.cc:97: __ Push(accessor_holder);
It looks like that I broke some ABI because I've added an extra data on stack,
which is not expected for someone.

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.v8%2Fv8_linux6...

Do you have any ideas who defines and checks this ABI and how I can add an
additional data?

[Toon Verwaest, 2017-06-23 12:11:43]

https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-com...
File src/ic/x64/handler-compiler-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-com...
src/ic/x64/handler-compiler-x64.cc:97: __ Push(accessor_holder);
On 2017/06/22 14:52:53, Yuki wrote:
> It looks like that I broke some ABI because I've added an extra data on stack,
> which is not expected for someone.
> 
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.v8%2Fv8_linux6...
> 
> Do you have any ideas who defines and checks this ABI and how I can add an
> additional data?

CallApiCallbackStub does 

2780   int stack_space = 0;
2781   Operand* stack_space_operand = &length_operand;
2782   stack_space = argc + FCA::kArgsLength + 1;
2783   stack_space_operand = nullptr;
2784   CallApiFunctionAndReturn(masm, api_function_address, thunk_ref,
callback_arg,
2785                            stack_space, stack_space_operand,
2786                            return_value_operand, &context_restore_operand);

I presume that length is now off-by-1, meaning you won't pop enough values on
the return. I presume you only want this argument to be present for the stub
call, so what about just conditionally pushing it after
PushReturnAddressFrom(scratch) (only if LAP) and popping it in the stub (if
LAP)?

[Yuki, 2017-06-26 07:38:27]

https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-com...
File src/ic/x64/handler-compiler-x64.cc (right):

https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-com...
src/ic/x64/handler-compiler-x64.cc:97: __ Push(accessor_holder);
On 2017/06/23 12:11:42, Toon Verwaest wrote:
> On 2017/06/22 14:52:53, Yuki wrote:
> > It looks like that I broke some ABI because I've added an extra data on
stack,
> > which is not expected for someone.
> > 
> >
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.v8%2Fv8_linux6...
> > 
> > Do you have any ideas who defines and checks this ABI and how I can add an
> > additional data?
> 
> CallApiCallbackStub does 
> 
> 2780   int stack_space = 0;
> 2781   Operand* stack_space_operand = &length_operand;
> 2782   stack_space = argc + FCA::kArgsLength + 1;
> 2783   stack_space_operand = nullptr;
> 2784   CallApiFunctionAndReturn(masm, api_function_address, thunk_ref,
> callback_arg,
> 2785                            stack_space, stack_space_operand,
> 2786                            return_value_operand,
&context_restore_operand);
> 
> I presume that length is now off-by-1, meaning you won't pop enough values on
> the return. I presume you only want this argument to be present for the stub
> call, so what about just conditionally pushing it after
> PushReturnAddressFrom(scratch) (only if LAP) and popping it in the stub (if
> LAP)?

Thanks!  You're right, now the tests pass.

What about always (unconditionally) pushing the accessor_holder on stack and
popping it?  Assuming that we're almost always using LAP, it's not that bad? 
It's a little confusing for me that the stack layout changes depending on LAP or
not.

[Yuki, 2017-06-26 07:58:51]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 07:58:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 08:03:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 08:03:45]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[Yuki, 2017-06-26 08:14:52]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 08:15:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 08:30:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 08:30:14]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/28115)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_node_linux64_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_node_linux64_rel/builds/...)

[Yuki, 2017-06-26 08:32:12]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 08:32:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 08:51:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 08:51:44]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/28120)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[Yuki, 2017-06-26 09:01:52]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 09:02:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-26 09:03:08]

Patchset #13 (id:380001) has been deleted

[Yuki, 2017-06-26 09:03:42]

Patchset #13 (id:400001) has been deleted

[Yuki, 2017-06-26 09:04:02]

Patchset #13 (id:420001) has been deleted

[Yuki, 2017-06-26 09:16:13]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 09:16:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-26 09:27:39]

Patchset #13 (id:440001) has been deleted

[commit-bot: I haz the power, 2017-06-26 09:37:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 09:37:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)

[Yuki, 2017-06-26 09:44:50]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 09:44:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-26 09:46:18]

Patchset #13 (id:460001) has been deleted

[commit-bot: I haz the power, 2017-06-26 10:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 10:08:25]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/24687)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Yuki, 2017-06-26 13:06:05]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 13:06:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-26 13:07:35]

Patchset #13 (id:480001) has been deleted

[Yuki, 2017-06-26 13:07:47]

Patchset #13 (id:500001) has been deleted

[Yuki, 2017-06-26 13:37:31]

Description was changed from

==========
Set the current context to the function's context when entering to LAP.

BUG=
==========

to

==========
Set the current context to the function's context when entering to LAP.

In case of LAP(lazy accessor pair), the function's creation context
must be equal to the accessor holder's creation context, so this CL
changes the current context to the accessor holder's creation context.

BUG=6156
==========

[Yuki, 2017-06-26 13:38:02]

Description was changed from

==========
Set the current context to the function's context when entering to LAP.

In case of LAP(lazy accessor pair), the function's creation context
must be equal to the accessor holder's creation context, so this CL
changes the current context to the accessor holder's creation context.

BUG=6156
==========

to

==========
Set the current context to the function's context when entering to LAP.

In case of LAP(lazy accessor pair), the function's creation context
must be equal to the accessor holder's creation context, so this CL
changes the current context to the accessor holder's creation context.

BUG=v8:6156
==========

[Yuki, 2017-06-26 13:39:38]

Hi Toon,

I think that now this CL is ready for review.
Could you take a look?

[commit-bot: I haz the power, 2017-06-26 14:32:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 14:32:01]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-06-29 07:20:02]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 07:20:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-06-29 07:23:04]

Hi Toon and Jochen,

Could you guys review this CL?  I think that I've done what I can do including
porting to all 9 architectures.

My only concern is that we seem not having trybots for all architectures.  E.g.
s390 seems not being tested.

[commit-bot: I haz the power, 2017-06-29 07:48:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 07:48:18]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2017-06-30 14:02:28]

lgtm, thanks!
sorry for the delay
I fully agree it makes more sense to always push it.

[Yuki, 2017-07-03 06:18:48]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2017-07-03 06:19:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-03 06:22:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-03 06:22:30]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/44415)

[Yuki, 2017-07-03 06:29:38]

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

[Yuki, 2017-07-03 06:29:40]

Hi Michael, could you review this CL as an owner of src/compiler/ ?

[Yuki, 2017-07-04 14:09:24]

yukishiino@chromium.org changed reviewers:
+ tebbi@google.com

[Yuki, 2017-07-04 14:09:26]

Tobias, could you review this CL as an owner of src/compiler/ ?  (I didn't know
Michael is ooo.)

[Tobias Tebbi, 2017-07-05 09:26:29]

On 2017/07/04 14:09:26, Yuki wrote:
> Tobias, could you review this CL as an owner of src/compiler/ ?  (I didn't
know
> Michael is ooo.)

src/compiler/* lgtm

[Yuki, 2017-07-05 09:27:48]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2017-07-05 09:28:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 09:57:33]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1499246868497250,
"parent_rev": "eaaff1ade085afdcb8af87a9dfe53c82768ce8e3", "commit_rev":
"18e73287dc65452bc2f952fc005b2251fc32c15a"}

[commit-bot: I haz the power, 2017-07-05 09:57:45]

Description was changed from

==========
Set the current context to the function's context when entering to LAP.

In case of LAP(lazy accessor pair), the function's creation context
must be equal to the accessor holder's creation context, so this CL
changes the current context to the accessor holder's creation context.

BUG=v8:6156
==========

to

==========
Set the current context to the function's context when entering to LAP.

In case of LAP(lazy accessor pair), the function's creation context
must be equal to the accessor holder's creation context, so this CL
changes the current context to the accessor holder's creation context.

BUG=v8:6156

Review-Url: https://codereview.chromium.org/2770003002
Cr-Commit-Position: refs/heads/master@{#46406}
Committed:
https://chromium.googlesource.com/v8/v8/+/18e73287dc65452bc2f952fc005b2251fc3...
==========

[commit-bot: I haz the power, 2017-07-05 09:57:47]

Committed patchset #14 (id:540001) as
https://chromium.googlesource.com/v8/v8/+/18e73287dc65452bc2f952fc005b2251fc3...

[Michael Achenbach, 2017-07-05 10:38:34]

Is this possibly breaking webkit unit tests?
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Mac/builds/11038

[Michael Achenbach, 2017-07-05 10:39:54]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2017-07-05 10:39:55]

Also the experimental trybot suggests it:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...

[Michael Achenbach, 2017-07-05 10:41:10]

A revert of this CL (patchset #14 id:540001) has been created in
https://codereview.chromium.org/2973593002/ by machenbach@chromium.org.

The reason for reverting is: Speculative: Seems to break webkit_unit_tests:
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Mac/builds/11038.

[Yuki, 2017-08-15 06:46:51]

On 2017/07/05 10:41:10, Michael Achenbach wrote:
> A revert of this CL (patchset #14 id:540001) has been created in
> https://codereview.chromium.org/2973593002/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: Speculative: Seems to break webkit_unit_tests:
>
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...
>
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Mac/builds/11038.

Just FYI, I'm now working to reland this CL at:
https://chromium-review.googlesource.com/c/563152