Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2199673002:
Properly set function index in CallSite constructor (Closed)
DescriptionProperly set function index in CallSite constructor
BUG=632965
R=yangguo@chromium.org
Committed: https://crrev.com/061d082dd3cb038b7f14b0de1e92aa6b4e083a43
Cr-Commit-Position: refs/heads/master@{#38208}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #Patch Set 3 : Fix compilation in release mode #Patch Set 4 : Adapt second site #Patch Set 5 : Remove tests passing invalid parameters to the callsite constructor #
Messages
Total messages: 31 (22 generated)
The CQ bit was checked by jgruber@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
AFAIK the ToUint32 call does absolutely nothing when fun is a Smi >= 0. Therefore I did not add additional test-cases since I don't think this can be triggered when CallSites are constructed from a valid frame (and we're planning on removing external access to the CallSite constructor soon).
https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... File src/builtins/builtins-callsite.cc (right): https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... src/builtins/builtins-callsite.cc:66: Object::ToUint32(isolate, fun)); I think having the integer conversion in the original JS code was just defensive programming. Let's instead assert that this is a smi. And assert that wasm::GetNumberOfFunctions(receiver) is larger than the index. https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... src/builtins/builtins-callsite.cc:76: Object::ToInt32(isolate, pos)); Same here. Let's assert that this is a smi.
On 2016/08/01 09:11:46, Yang wrote: > https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... > File src/builtins/builtins-callsite.cc (right): > > https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... > src/builtins/builtins-callsite.cc:66: Object::ToUint32(isolate, fun)); > I think having the integer conversion in the original JS code was just defensive > programming. Let's instead assert that this is a smi. And assert that > wasm::GetNumberOfFunctions(receiver) is larger than the index. > > https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... > src/builtins/builtins-callsite.cc:76: Object::ToInt32(isolate, pos)); > Same here. Let's assert that this is a smi. lgtm otherwise.
The CQ bit was checked by jgruber@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...
The CQ bit was unchecked by commit-bot@chromium.org
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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by jgruber@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... File src/builtins/builtins-callsite.cc (right): https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... src/builtins/builtins-callsite.cc:66: Object::ToUint32(isolate, fun)); On 2016/08/01 09:11:46, Yang wrote: > I think having the integer conversion in the original JS code was just defensive > programming. Let's instead assert that this is a smi. And assert that > wasm::GetNumberOfFunctions(receiver) is larger than the index. Done. https://codereview.chromium.org/2199673002/diff/1/src/builtins/builtins-calls... src/builtins/builtins-callsite.cc:76: Object::ToInt32(isolate, pos)); On 2016/08/01 09:11:46, Yang wrote: > Same here. Let's assert that this is a smi. Done.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2199673002/#ps60001 (title: "Adapt second site")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10027) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by jgruber@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2199673002/#ps80001 (title: "Remove tests passing invalid parameters to the callsite constructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Properly set function index in CallSite constructor BUG=632965 R=yangguo@chromium.org ========== to ========== Properly set function index in CallSite constructor BUG=632965 R=yangguo@chromium.org Committed: https://crrev.com/061d082dd3cb038b7f14b0de1e92aa6b4e083a43 Cr-Commit-Position: refs/heads/master@{#38208} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/061d082dd3cb038b7f14b0de1e92aa6b4e083a43 Cr-Commit-Position: refs/heads/master@{#38208} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
