Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(109)

Issue 2770003002: Set the current context to the function's context when entering to LAP. (Closed)

Created:
3 years, 9 months ago by Yuki
Modified:
3 years, 4 months ago
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/heads/master
Project:
v8
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Total comments: 2

Patch Set 4 : . #

Total comments: 3

Patch Set 5 : Set the signature to the callback function. #

Patch Set 6 : Synced. #

Patch Set 7 : Made the unittest a little milder (not test a corner case). #

Total comments: 2

Patch Set 8 : Attempt to get the context in PropertyHandlerCompiler::GenerateApiAccessorCall (NOT WORKING) #

Total comments: 2

Patch Set 9 : Uses the accessor holder appropriately + made the test stricter again. #

Patch Set 10 : Synced. #

Patch Set 11 : Extracts the accessor holder more lazily and removed crashing DCHECKs. #

Patch Set 12 : Crashing at cctest test-accessors/AccessorIC #

Total comments: 3

Patch Set 13 : Fixed several things so that tests pass. #

Patch Set 14 : Synced. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+270 lines, -108 lines) Patch
M src/arm/code-stubs-arm.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +18 lines, -10 lines 0 comments Download
M src/arm64/code-stubs-arm64.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +21 lines, -13 lines 0 comments Download
M src/compiler/js-call-reducer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +4 lines, -2 lines 0 comments Download
M src/compiler/js-native-context-specialization.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M src/compiler/js-native-context-specialization.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +17 lines, -11 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +17 lines, -10 lines 0 comments Download
M src/ic/arm/handler-compiler-arm.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/arm64/handler-compiler-arm64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/ia32/handler-compiler-ia32.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/mips/handler-compiler-mips.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/mips64/handler-compiler-mips64.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/ppc/handler-compiler-ppc.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/s390/handler-compiler-s390.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/ic/x64/handler-compiler-x64.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M src/ic/x87/handler-compiler-x87.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M src/mips/code-stubs-mips.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +17 lines, -8 lines 0 comments Download
M src/mips64/code-stubs-mips64.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +18 lines, -9 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +6 lines, -2 lines 0 comments Download
M src/ppc/code-stubs-ppc.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +17 lines, -10 lines 0 comments Download
M src/s390/code-stubs-s390.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +18 lines, -11 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +18 lines, -11 lines 0 comments Download
M src/x87/code-stubs-x87.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +17 lines, -10 lines 0 comments Download
M test/unittests/api/v8-object-unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +71 lines, -0 lines 0 comments Download

Messages

Total messages: 126 (96 generated)
jochen (gone - plz use gerrit)
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.cc#newcode2843 src/x64/code-stubs-x64.cc:2843: if (this->is_lazy()) { you could move ...
3 years, 9 months ago (2017-03-24 15:10:15 UTC) #2
Yuki
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.cc#newcode2843 src/x64/code-stubs-x64.cc:2843: if (this->is_lazy()) { On 2017/03/24 15:10:15, jochen wrote: > ...
3 years, 8 months ago (2017-03-28 14:16:58 UTC) #3
jochen (gone - plz use gerrit)
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.cc#newcode2841 src/x64/code-stubs-x64.cc:2841: __ movp(scratch, FieldOperand(holder, HeapObject::kMapOffset)); On 2017/03/28 at 14:16:58, Yuki ...
3 years, 8 months ago (2017-03-28 14:43:39 UTC) #4
Yuki
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.cc#newcode2841 src/x64/code-stubs-x64.cc:2841: __ movp(scratch, FieldOperand(holder, HeapObject::kMapOffset)); On 2017/03/28 14:43:39, jochen wrote: ...
3 years, 8 months ago (2017-03-29 14:19:41 UTC) #5
jochen (gone - plz use gerrit)
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.cc#newcode2841 ...
3 years, 8 months ago (2017-03-29 14:22:48 UTC) #6
jochen (gone - plz use gerrit)
I prepared a CL to add documentation for the info.This() and info.Holder(). So the problem ...
3 years, 8 months ago (2017-04-21 16:06:12 UTC) #7
Yuki
On 2017/04/21 16:06:12, jochen (slow until May 2) wrote: > I prepared a CL to ...
3 years, 8 months ago (2017-04-25 09:03:43 UTC) #8
jochen (gone - plz use gerrit)
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.cc#newcode2758 src/x64/code-stubs-x64.cc:2758: if (this->is_lazy()) { maybe we can just enter the ...
3 years, 6 months ago (2017-06-14 09:26:01 UTC) #11
Yuki
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.cc#newcode2758 src/x64/code-stubs-x64.cc:2758: if (this->is_lazy()) { On 2017/06/14 09:26:01, jochen wrote: > ...
3 years, 6 months ago (2017-06-14 13:34:52 UTC) #14
Toon Verwaest
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.cc#newcode2739 src/x64/code-stubs-x64.cc:2739: __ Push(context); This is now the context after loading ...
3 years, 6 months ago (2017-06-19 08:45:05 UTC) #16
Yuki
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.cc#newcode2739 src/x64/code-stubs-x64.cc:2739: __ Push(context); On 2017/06/19 08:45:05, Toon Verwaest wrote: > ...
3 years, 6 months ago (2017-06-20 14:35:13 UTC) #17
Yuki
Thank you Toon and Jochen. Now this CL is passing the unittest even in the ...
3 years, 6 months ago (2017-06-21 14:20:18 UTC) #22
Toon Verwaest
Overall this looks good. Feel free to port to the other architectures.
3 years, 6 months ago (2017-06-21 19:07:55 UTC) #27
Yuki
Hi Toon and Jochen, I have a crash issue with this CL. Could you help ...
3 years, 6 months ago (2017-06-22 14:52:53 UTC) #63
Toon Verwaest
https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-compiler-x64.cc File src/ic/x64/handler-compiler-x64.cc (right): https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-compiler-x64.cc#newcode97 src/ic/x64/handler-compiler-x64.cc:97: __ Push(accessor_holder); On 2017/06/22 14:52:53, Yuki wrote: > It ...
3 years, 6 months ago (2017-06-23 12:11:43 UTC) #64
Yuki
https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-compiler-x64.cc File src/ic/x64/handler-compiler-x64.cc (right): https://codereview.chromium.org/2770003002/diff/360001/src/ic/x64/handler-compiler-x64.cc#newcode97 src/ic/x64/handler-compiler-x64.cc:97: __ Push(accessor_holder); On 2017/06/23 12:11:42, Toon Verwaest wrote: > ...
3 years, 5 months ago (2017-06-26 07:38:27 UTC) #65
Yuki
Hi Toon, I think that now this CL is ready for review. Could you take ...
3 years, 5 months ago (2017-06-26 13:39:38 UTC) #99
Yuki
Hi Toon and Jochen, Could you guys review this CL? I think that I've done ...
3 years, 5 months ago (2017-06-29 07:23:04 UTC) #104
Toon Verwaest
lgtm, thanks! sorry for the delay I fully agree it makes more sense to always ...
3 years, 5 months ago (2017-06-30 14:02:28 UTC) #107
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2770003002/540001
3 years, 5 months ago (2017-07-03 06:19:00 UTC) #109
commit-bot: I haz the power
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)
3 years, 5 months ago (2017-07-03 06:22:30 UTC) #111
Yuki
Hi Michael, could you review this CL as an owner of src/compiler/ ?
3 years, 5 months ago (2017-07-03 06:29:40 UTC) #113
Yuki
Tobias, could you review this CL as an owner of src/compiler/ ? (I didn't know ...
3 years, 5 months ago (2017-07-04 14:09:26 UTC) #115
Tobias Tebbi
On 2017/07/04 14:09:26, Yuki wrote: > Tobias, could you review this CL as an owner ...
3 years, 5 months ago (2017-07-05 09:26:29 UTC) #116
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2770003002/540001
3 years, 5 months ago (2017-07-05 09:28:02 UTC) #118
commit-bot: I haz the power
Committed patchset #14 (id:540001) as https://chromium.googlesource.com/v8/v8/+/18e73287dc65452bc2f952fc005b2251fc32c15a
3 years, 5 months ago (2017-07-05 09:57:47 UTC) #121
Michael Achenbach
Is this possibly breaking webkit unit tests? https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Mac/builds/11038
3 years, 5 months ago (2017-07-05 10:38:34 UTC) #122
Michael Achenbach
Also the experimental trybot suggests it: https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/23247
3 years, 5 months ago (2017-07-05 10:39:55 UTC) #124
Michael Achenbach
A revert of this CL (patchset #14 id:540001) has been created in https://codereview.chromium.org/2973593002/ by machenbach@chromium.org. ...
3 years, 5 months ago (2017-07-05 10:41:10 UTC) #125
Yuki
3 years, 4 months ago (2017-08-15 06:46:51 UTC) #126
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld 408576698