Issue metadata
Sign in to add a comment
|
DCHECK failure in *source_map != *feedback in feedback-vector.cc |
||||||||||||||||||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=6744125769252864 Fuzzer: ochang_js_fuzzer Job Type: linux_asan_d8_dbg Platform Id: linux Crash Type: DCHECK failure Crash Address: Crash State: *source_map != *feedback in feedback-vector.cc Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=54594:54595 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6744125769252864 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Jul 26
,
Jul 26
jkummerow, question:
I had originally written this to miss if it encounters a deprecated Map, e.g.
```
var obj1 = { x: 1 };
var obj2 = { x: 2 }; // same map
obj2.x = null; // deprecate map
function f() { return { ...obj1 } };
f(); // uninitialized with deprecated map -> miss
f(); // monomorphic with deprecated map -> miss (hit this DCHECK)
```
But, it seems like in this case it should be completely fine to fast-clone the object with the deprecated map.
Is it safe to get rid of the miss-if-deprecated line? It's really hard for me to tell, the only thing that seems to mark a map as deprecated (that I can find from a quick grep) is Map::TransitionToDataProperty(), but if an object is still holding the deprecated map, then doesn't that de facto mean that the map is still valid for that object, and should not result in a miss?
,
Jul 26
It's true that deprecated maps are still safe/valid to use, but we generally want to migrate all objects away from them ASAP to avoid polymorphism elsewhere. So please keep missing when a deprecated map is encountered. (If anything, maybe we should also miss when the target map has been deprecated...) You can extend the checks for IsClearedWeakHeapObject in both MONOMORPHIC and POLYMORPHIC to also take the overwriting branch for deprecated maps. I'm pretty sure that this DCHECK failure does not have security implications, so lifting restrictions.
,
Jul 26
Ok, fair enough
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/670fa86fd171670c86528fc0fad2b262ea19be74 commit 670fa86fd171670c86528fc0fad2b262ea19be74 Author: Caitlin Potter <caitp@igalia.com> Date: Fri Jul 27 19:37:39 2018 [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated An object with a deprecated Map which has already been cached in CloneObjectIC feedback is still a valid Map for fast cloning --- but to be consistent with other ICs, deprecated maps are ignored, and are expected to be transitioned away from. If the source object has a deprecated map, the instance is migrated. BUG=v8:7611, chromium:867958 R=jkummerow@chromium.org, mvstanton@chromium.org Change-Id: I9771b00400fb4dda45a62e874a31d9b50630d847 Reviewed-on: https://chromium-review.googlesource.com/1152414 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Caitlin Potter <caitp@igalia.com> Cr-Commit-Position: refs/heads/master@{#54758} [modify] https://crrev.com/670fa86fd171670c86528fc0fad2b262ea19be74/src/feedback-vector.cc [modify] https://crrev.com/670fa86fd171670c86528fc0fad2b262ea19be74/src/ic/ic.cc [add] https://crrev.com/670fa86fd171670c86528fc0fad2b262ea19be74/test/mjsunit/es9/regress/regress-867958.js
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/43098ecbe846d4af5537987f68b61f2f504aca5e commit 43098ecbe846d4af5537987f68b61f2f504aca5e Author: Deepti Gandluri <gdeepti@chromium.org> Date: Fri Jul 27 21:39:00 2018 Revert "[CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated" This reverts commit 670fa86fd171670c86528fc0fad2b262ea19be74. Reason for revert: Causes gc-stress bots to fail - https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket.appspot.com/8939819990688690880/+/steps/Check_-_d8/0/logs/object-spread-ic/0 https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket.appspot.com/8939819696982258192/+/steps/Check_-_d8/0/logs/object-spread-ic/0 Original change's description: > [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated > > An object with a deprecated Map which has already been cached in > CloneObjectIC feedback is still a valid Map for fast cloning --- but > to be consistent with other ICs, deprecated maps are ignored, and are > expected to be transitioned away from. > > If the source object has a deprecated map, the instance is migrated. > > BUG=v8:7611, chromium:867958 > R=jkummerow@chromium.org, mvstanton@chromium.org > > Change-Id: I9771b00400fb4dda45a62e874a31d9b50630d847 > Reviewed-on: https://chromium-review.googlesource.com/1152414 > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> > Commit-Queue: Caitlin Potter <caitp@igalia.com> > Cr-Commit-Position: refs/heads/master@{#54758} TBR=jkummerow@chromium.org,mvstanton@chromium.org,caitp@igalia.com Change-Id: Id17652ad6f3f09adb43848069549ad146d48b2d7 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:7611, chromium:867958 Reviewed-on: https://chromium-review.googlesource.com/1153747 Reviewed-by: Deepti Gandluri <gdeepti@chromium.org> Commit-Queue: Deepti Gandluri <gdeepti@chromium.org> Cr-Commit-Position: refs/heads/master@{#54760} [modify] https://crrev.com/43098ecbe846d4af5537987f68b61f2f504aca5e/src/feedback-vector.cc [modify] https://crrev.com/43098ecbe846d4af5537987f68b61f2f504aca5e/src/ic/ic.cc [delete] https://crrev.com/6cd560b265cc3af5e31bcc883c235e7680b8d0a8/test/mjsunit/es9/regress/regress-867958.js
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d9f6c685f008ab8b669492b1c10ec44a9883de76 commit d9f6c685f008ab8b669492b1c10ec44a9883de76 Author: Caitlin Potter <caitp@igalia.com> Date: Tue Jul 31 04:11:05 2018 Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated An object with a deprecated Map which has already been cached in CloneObjectIC feedback is still a valid Map for fast cloning --- but to be consistent with other ICs, deprecated maps are ignored, and are expected to be transitioned away from. If the source object has a deprecated map, the instance is migrated. BUG=v8:7611, chromium:867958 , chromium:868586 R=jkummerow@chromium.org, mvstanton@chromium.org Change-Id: I477aec6c8d0ae1e1648a70e85d2fd46146521d1c Reviewed-on: https://chromium-review.googlesource.com/1154143 Commit-Queue: Caitlin Potter <caitp@igalia.com> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Commit-Position: refs/heads/master@{#54799} [modify] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/src/feedback-vector.cc [modify] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/src/ic/ic.cc [add] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/test/mjsunit/es9/regress/regress-867958.js
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/3879e8bfe3c8c3fdf05e772f741feda3209304cf commit 3879e8bfe3c8c3fdf05e772f741feda3209304cf Author: Michael Achenbach <machenbach@chromium.org> Date: Tue Jul 31 06:29:49 2018 Revert "Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated" This reverts commit d9f6c685f008ab8b669492b1c10ec44a9883de76. Reason for revert: https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux%20-%20gc%20stress/17584 Original change's description: > Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated > > An object with a deprecated Map which has already been cached in > CloneObjectIC feedback is still a valid Map for fast cloning --- but > to be consistent with other ICs, deprecated maps are ignored, and are > expected to be transitioned away from. > > If the source object has a deprecated map, the instance is migrated. > > BUG=v8:7611, chromium:867958 , chromium:868586 > R=jkummerow@chromium.org, mvstanton@chromium.org > > Change-Id: I477aec6c8d0ae1e1648a70e85d2fd46146521d1c > Reviewed-on: https://chromium-review.googlesource.com/1154143 > Commit-Queue: Caitlin Potter <caitp@igalia.com> > Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> > Cr-Commit-Position: refs/heads/master@{#54799} TBR=jkummerow@chromium.org,mvstanton@chromium.org,caitp@igalia.com Change-Id: Ifcb422c3a692543490710d450590323524a6359a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:7611, chromium:867958 , chromium:868586 Reviewed-on: https://chromium-review.googlesource.com/1155593 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Michael Achenbach <machenbach@chromium.org> Cr-Commit-Position: refs/heads/master@{#54800} [modify] https://crrev.com/3879e8bfe3c8c3fdf05e772f741feda3209304cf/src/feedback-vector.cc [modify] https://crrev.com/3879e8bfe3c8c3fdf05e772f741feda3209304cf/src/ic/ic.cc [delete] https://crrev.com/d9f6c685f008ab8b669492b1c10ec44a9883de76/test/mjsunit/es9/regress/regress-867958.js
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/5caee70b664e81ccd57453a64757573aeb070107 commit 5caee70b664e81ccd57453a64757573aeb070107 Author: Caitlin Potter <caitp@igalia.com> Date: Wed Aug 01 00:30:11 2018 Reland "Reland [CloneObjectIC] overwrite monomorphic/polymorphic feedback if deprecated" An object with a deprecated Map which has already been cached in CloneObjectIC feedback is still a valid Map for fast cloning --- but to be consistent with other ICs, deprecated maps are ignored, and are expected to be transitioned away from. If the source object has a deprecated map, the instance is migrated. BUG=v8:7611, chromium:867958 , chromium:868586 , chromium:869342 , chromium:869347 , chromium:869293 R=jkummerow@chromium.org, mvstanton@chromium.org Reviewed-on: https://chromium-review.googlesource.com/1154143 Commit-Queue: Caitlin Potter <caitp@igalia.com> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#54799} Change-Id: I6e2f7b28c41bb9bd6255441da0f209a97bce5e8f Reviewed-on: https://chromium-review.googlesource.com/1157142 Cr-Commit-Position: refs/heads/master@{#54830} [modify] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/src/feedback-vector.cc [modify] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/src/ic/ic.cc [add] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/test/mjsunit/es9/regress/regress-867958.js [add] https://crrev.com/5caee70b664e81ccd57453a64757573aeb070107/test/mjsunit/es9/regress/regress-869342.js
,
Aug 1
ClusterFuzz has detected this issue as fixed in range 54829:54830. Detailed report: https://clusterfuzz.com/testcase?key=6744125769252864 Fuzzer: ochang_js_fuzzer Job Type: linux_asan_d8_dbg Platform Id: linux Crash Type: DCHECK failure Crash Address: Crash State: *source_map != *feedback in feedback-vector.cc Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=54594:54595 Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=54829:54830 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6744125769252864 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Aug 1
ClusterFuzz testcase 6744125769252864 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by ClusterFuzz
, Jul 26Owner: ca...@igalia.com
Status: Assigned (was: Untriaged)