[maglev] Prevent constant folding `instanceof` checks [v8/v8 : main]
1 view
Skip to first unread message
Victor Gomes (Gerrit)
Aug 21, 2025, 7:00:44 AM8/21/25
to V8 LUCI CQ, Patrick Thier, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Patrick Thier
Victor Gomes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
2 comments
Patchset-level comments
File src/maglev/maglev-graph-builder.cc
Line 12426, Patchset 1:
if (alloc->is_returned_value_from_inline_call()) {Patrick Thier . resolved
Victor GomesThat's behind `DEBUG` right now.
Indeed. And I think it should continue like that.
After some thinking, the problem is not just here, but in TryGetPossibleMaps. It is reading the last object associated to the inlined allocation, an object that it might not be visible by the virtual object list. So I fixed that instead. We return no possible maps, which returns kMayBeInPrototypeChain.
TryGetPossibleMaps was actually wrong already, we need to do a FindAllocatedWith, since we could have 2 branches, the first modify the object in the InlinedAllocation (last modified object), and then the second needs to find the correct (previous version) of the object.
Related details
Attention is currently required from:
- Patrick Thier
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Thier (Gerrit)
Aug 21, 2025, 7:29:07 AM8/21/25
to Victor Gomes, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Patrick Thier voted and added 2 comments![]( "Open in Gerrit")
Votes added by Patrick Thier
| Code-Review | +1 |
2 comments
Patchset-level comments
Patrick Thier . resolved
LGTM
File src/maglev/maglev-interpreter-frame-state.h
Line 1297, Patchset 4 (Latest):
return VirtualObjectList(as_interpreted().last_virtual_object());Patrick Thier . unresolved
Please add a comment why we can only have a single VO here.
Related details
Attention is currently required from:
- Victor Gomes
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Aug 21, 2025, 7:51:42 AM8/21/25
to Patrick Thier, V8 LUCI CQ, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Victor Gomes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Victor Gomes
| Auto-Submit | +1 |
| Commit-Queue | +2 |
2 comments
Patchset-level comments
File src/maglev/maglev-interpreter-frame-state.h
Line 1297, Patchset 4:
return VirtualObjectList(as_interpreted().last_virtual_object());Patrick Thier . resolved
Please add a comment why we can only have a single VO here.
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Aug 21, 2025, 8:25:52 AM8/21/25
to Victor Gomes, Patrick Thier, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
V8 LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/maglev/maglev-interpreter-frame-state.h
Insertions: 2, Deletions: 0.
@@ -1294,6 +1294,8 @@
inline VirtualObjectList DeoptFrame::GetVirtualObjects() const {
if (type() == DeoptFrame::FrameType::kInterpretedFrame) {
+ // Recover virtual object list using the last object before the
+ // deopt frame creation.
return VirtualObjectList(as_interpreted().last_virtual_object());
}
DCHECK_NOT_NULL(parent());
```
Change information
Commit message:
[maglev] Prevent constant folding `instanceof` checks
... when the receiver is a virtual object that was created in a
different, non-eagerly inlined function.
To fix this, we move the virtual object list from the interpreter frame
state to the known node aspects. This gives `TryGetPossibleMaps` the
correct context to find the correct object for this allocation site.
If the object isn't found in the current scope's VO list, we can infer
it leaked from another inlining operation.
Fixed: 436778602
Change-Id: I53bca272d011d8d8be9177b2f82a9e824db62e39
Auto-Submit: Victor Gomes <victo...@chromium.org>
Commit-Queue: Victor Gomes <victo...@chromium.org>
Reviewed-by: Patrick Thier <pth...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#101970}
Files:
- M src/maglev/maglev-graph-builder.cc
- M src/maglev/maglev-interpreter-frame-state.cc
- M src/maglev/maglev-interpreter-frame-state.h
- M src/maglev/maglev-ir.h
- A test/mjsunit/maglev/regress-436778602.js
Change size: M
Delta: 5 files changed, 102 insertions(+), 84 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Patrick Thier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages