Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724833002:
Migrate Object.prototype.valueOf to CSA (Closed)
DescriptionMigrate Object.prototype.valueOf to CSA
BUG=v8:6005
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2724833002
Cr-Commit-Position: refs/heads/master@{#43539}
Committed: https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc399a01
Patch Set 1 #Patch Set 2 : Delete NaN #
Total comments: 3
Patch Set 3 : Add set_object_value & update cctest #
Total comments: 3
Patch Set 4 : Separate CL & fixes #
Total comments: 1
Messages
Total messages: 37 (20 generated)
Patchset #1 (id:1) has been deleted
loorongjie@gmail.com changed reviewers: + bmeurer@chromium.org
https://codereview.chromium.org/2724833002/diff/40001/src/js/v8natives.js File src/js/v8natives.js (left): https://codereview.chromium.org/2724833002/diff/40001/src/js/v8natives.js#old... src/js/v8natives.js:117: "object_value_of", ObjectValueOf, Does this pass the test suite?
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2724833002/diff/40001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2724833002/diff/40001/src/bootstrapper.cc#new... src/bootstrapper.cc:1270: Builtins::kObjectPrototypeValueOf, 0, true); We still use the object_value_of field of the native context in Crankshaft, so you need to save the result of this there, i.e. Handle<JSFunction> object_value_of = ...; native_context()->set_object_value_of(*object_value_of);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
https://codereview.chromium.org/2724833002/diff/40001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2724833002/diff/40001/src/bootstrapper.cc#new... src/bootstrapper.cc:1270: Builtins::kObjectPrototypeValueOf, 0, true); On 2017/03/01 20:00:26, Benedikt Meurer wrote: > We still use the object_value_of field of the native context in Crankshaft, so > you need to save the result of this there, i.e. > > Handle<JSFunction> object_value_of = ...; > native_context()->set_object_value_of(*object_value_of); Done.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bmeurer@chromium.org changed reviewers: + yangguo@chromium.org
Thanks. It looks like the test-serialize.cc failure should be fixed by removing the failing test case; also there's a failure in DebugEvaluate. Adding yangguo@ for both.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
On 2017/03/02 05:22:22, Benedikt Meurer wrote: > Thanks. It looks like the test-serialize.cc failure should be fixed by removing > the failing test case; also there's a failure in DebugEvaluate. Adding yangguo@ > for both. I get a lot of DCHECK(!context.IsEmpty()) in cctest/test-serialize.cc and I have no idea how to fix that. You are suggesting to just remove the whole tests that have this DCHECK or just that line? I can't see how my change causes debug-evaluate-no-side-effect.js to fail.
Debug-evaluate uses the bytecode to determine whether a function is side-effect free or not. Since you are replacing it by a builtin, you have to add the builtin to the whitelist explicitly. Take a look at src/debug/debug-evaluate.cc Regarding DCHECK failures in test-serialize... if you are seeing DCHECK(!context.Empty()) failing, something more serious is going wrong. Context creation would have failed in that case. https://codereview.chromium.org/2724833002/diff/60001/src/js/v8natives.js File src/js/v8natives.js (left): https://codereview.chromium.org/2724833002/diff/60001/src/js/v8natives.js#old... src/js/v8natives.js:14: var NaN = %GetRootNaN(); Do we still need the runtime function for this too? This seems unrelated. So maybe split this into a separate CL? https://codereview.chromium.org/2724833002/diff/60001/test/cctest/test-serial... File test/cctest/test-serialize.cc (left): https://codereview.chromium.org/2724833002/diff/60001/test/cctest/test-serial... test/cctest/test-serialize.cc:871: CHECK(!IsCompiled("Object.valueOf")); This test is supposed to test that we do not include compiled code for built-in functions written in JS. Let's change this to Array.prototype.sort for now.
https://codereview.chromium.org/2724833002/diff/60001/src/js/v8natives.js File src/js/v8natives.js (left): https://codereview.chromium.org/2724833002/diff/60001/src/js/v8natives.js#old... src/js/v8natives.js:14: var NaN = %GetRootNaN(); On 2017/03/02 06:55:12, Yang wrote: > Do we still need the runtime function for this too? This seems unrelated. So > maybe split this into a separate CL? I forgot to remove this in https://codereview.chromium.org/2715793004/, will split this into new CL.
> Regarding DCHECK failures in test-serialize... if you are seeing > DCHECK(!context.Empty()) failing, something more serious is going wrong. Context > creation would have failed in that case. Now I don't see this DCHECK failing anymore. I think I know why it failed at first: I ran cctest test-serialize directly instead of through run-tests.py (because this takes 1 hour), cctest complained "Running multiple tests in sequence is deprecated and may cause bogus failure. Consider using tools/run-tests.py instead.". No error when running run-tests.py. Can I get another tryjob?
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/02 12:01:50, rongjie wrote: > > Regarding DCHECK failures in test-serialize... if you are seeing > > DCHECK(!context.Empty()) failing, something more serious is going wrong. > Context > > creation would have failed in that case. > > Now I don't see this DCHECK failing anymore. I think I know why it failed at > first: I ran cctest test-serialize directly instead of through run-tests.py > (because this takes 1 hour), cctest complained "Running multiple tests in > sequence is deprecated and may cause bogus failure. Consider using > tools/run-tests.py instead.". No error when running run-tests.py. > > Can I get another tryjob? lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks!
The CQ bit was checked by loorongjie@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488459301023990,
"parent_rev": "f662ae9781a0dfa2307f0c66161d8a535a88a6c8", "commit_rev":
"f93b27e639cca6a93ce9b6c535ece9b6cc399a01"}
Message was sent while issue was closed.
Description was changed from ========== Migrate Object.prototype.valueOf to CSA BUG=v8:6005 ========== to ========== Migrate Object.prototype.valueOf to CSA BUG=v8:6005 Review-Url: https://codereview.chromium.org/2724833002 Cr-Commit-Position: refs/heads/master@{#43539} Committed: https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc3...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/2730573004/ by machenbach@chromium.org. The reason for reverting is: Breaks layout tests: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... See also: https://github.com/v8/v8/wiki/Blink-layout-tests.
Message was sent while issue was closed.
Description was changed from ========== Migrate Object.prototype.valueOf to CSA BUG=v8:6005 Review-Url: https://codereview.chromium.org/2724833002 Cr-Commit-Position: refs/heads/master@{#43539} Committed: https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc3... ========== to ========== Migrate Object.prototype.valueOf to CSA BUG=v8:6005 Review-Url: https://codereview.chromium.org/2724833002 Cr-Commit-Position: refs/heads/master@{#43539} Committed: https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc3... ==========
As written on the bug report, the layout test is only failing because of enumeration order. Just move the lines down in the bootstrapper.cc and it should be fine. You can just reuse this CL with a fix uploaded. I'll make the blink bot mandatory, so the trybots will catch potential layout test failures. https://codereview.chromium.org/2724833002/diff/80001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2724833002/diff/80001/src/bootstrapper.cc#new... src/bootstrapper.cc:1269: Handle<JSFunction> object_value_of = SimpleInstallFunction( Just move this down until after "propertyIsEnumerable" is installed, then the layout test should pass.
Description was changed from ========== Migrate Object.prototype.valueOf to CSA BUG=v8:6005 Review-Url: https://codereview.chromium.org/2724833002 Cr-Commit-Position: refs/heads/master@{#43539} Committed: https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc3... ========== to ========== Migrate Object.prototype.valueOf to CSA BUG=v8:6005 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2724833002 Cr-Commit-Position: refs/heads/master@{#43539} Committed: https://chromium.googlesource.com/v8/v8/+/f93b27e639cca6a93ce9b6c535ece9b6cc3... ==========
On 2017/03/03 05:22:47, Benedikt Meurer wrote: > As written on the bug report, the layout test is only failing because of > enumeration order. Just move the lines down in the bootstrapper.cc and it > should be fine. > > You can just reuse this CL with a fix uploaded. I'll make the blink bot > mandatory, so the trybots will catch potential layout test failures. > > https://codereview.chromium.org/2724833002/diff/80001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/2724833002/diff/80001/src/bootstrapper.cc#new... > src/bootstrapper.cc:1269: Handle<JSFunction> object_value_of = > SimpleInstallFunction( > Just move this down until after "propertyIsEnumerable" is installed, then the > layout test should pass. making blink bot mandatory is undesirable whenever blink has a bad commit and fails because of something else.
On 2017/03/03 05:28:40, Yang wrote: > On 2017/03/03 05:22:47, Benedikt Meurer wrote: > > As written on the bug report, the layout test is only failing because of > > enumeration order. Just move the lines down in the bootstrapper.cc and it > > should be fine. > > > > You can just reuse this CL with a fix uploaded. I'll make the blink bot > > mandatory, so the trybots will catch potential layout test failures. > > > > https://codereview.chromium.org/2724833002/diff/80001/src/bootstrapper.cc > > File src/bootstrapper.cc (right): > > > > > https://codereview.chromium.org/2724833002/diff/80001/src/bootstrapper.cc#new... > > src/bootstrapper.cc:1269: Handle<JSFunction> object_value_of = > > SimpleInstallFunction( > > Just move this down until after "propertyIsEnumerable" is installed, then the > > layout test should pass. > > making blink bot mandatory is undesirable whenever blink has a bad commit and > fails because of something else. I mean mandatory for this CL by adding the CQ_INCLUDE_TRYBOTS line. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
