[maglev] Fix a bug in (a+b)|0 omitting overflow checks [v8/v8 : main]
0 views
Skip to first unread message
Marja Hölttä (Gerrit)
Oct 24, 2025, 8:11:06 AM (8 days ago) Oct 24
to Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Leszek Swirski
Marja Hölttä added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Leszek Swirski
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Oct 24, 2025, 8:34:16 AM (8 days ago) Oct 24
to Marja Hölttä, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Marja Hölttä
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
File src/maglev/maglev-truncation.h
Line 99, Patchset 6 (Latest):
ProcessResult Process(Identity* node) { return ProcessResult::kContinue; }Leszek Swirski . unresolved
there's an overload for Identity which means that the generic `UnsetCanTruncateToInt32Inputs` calling path won't be called. Probably you should actually use this overload, and add one for ReturnedValue.
Related details
Attention is currently required from:
- Marja Hölttä
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Oct 27, 2025, 4:28:01 AM (5 days ago) Oct 27
to Marja Hölttä, Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Marja Hölttä
Victor Gomes added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Victor Gomes . resolved
Please check in pinpoint that this fix will not regress turbolev-future. This might break truncation altogether. If that's the case we should think of a better way to fix this.
Related details
Attention is currently required from:
- Marja Hölttä
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Oct 27, 2025, 5:10:53 AM (5 days ago) Oct 27
to Marja Hölttä, Victor Gomes, Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Marja Hölttä
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10ebb03ad10000
Related details
Attention is currently required from:
- Marja Hölttä
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Oct 27, 2025, 7:32:53 AM (5 days ago) Oct 27
to chrom...@appspot.gserviceaccount.com, Victor Gomes, Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Victor Gomes
Marja Hölttä added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Victor Gomes . resolved
Please check in pinpoint that this fix will not regress turbolev-future. This might break truncation altogether. If that's the case we should think of a better way to fix this.
Attention is currently required from:
- Victor Gomes
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Gomes (Gerrit)
Oct 27, 2025, 7:34:14 AM (5 days ago) Oct 27
to Marja Hölttä, chrom...@appspot.gserviceaccount.com, Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Attention needed from Marja Hölttä
Victor Gomes added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Victor Gomes . resolved
Marja HölttäPlease check in pinpoint that this fix will not regress turbolev-future. This might break truncation altogether. If that's the case we should think of a better way to fix this.
https://pinpoint-dot-chromeperf.appspot.com/job/10ebb03ad10000 looks fine
Attention is currently required from:
- Marja Hölttä
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Oct 27, 2025, 8:02:26 AM (5 days ago) Oct 27
to chrom...@appspot.gserviceaccount.com, Victor Gomes, Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Marja Hölttä added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File src/maglev/maglev-truncation.h
Line 99, Patchset 6:
ProcessResult Process(Identity* node) { return ProcessResult::kContinue; }Leszek Swirski . resolved
there's an overload for Identity which means that the generic `UnsetCanTruncateToInt32Inputs` calling path won't be called. Probably you should actually use this overload, and add one for ReturnedValue.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Oct 27, 2025, 8:02:27 AM (5 days ago) Oct 27
to chrom...@appspot.gserviceaccount.com, Victor Gomes, Leszek Swirski, V8 LUCI CQ, AyeAye, leszek...@chromium.org, v8-re...@googlegroups.com, verwaes...@chromium.org, victorgo...@chromium.org
Marja Hölttä voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Oct 27, 2025, 8:22:46 AM (5 days ago) Oct 27
to Marja Hölttä, chrom...@appspot.gserviceaccount.com, Victor Gomes, Leszek Swirski, AyeAye, 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
6 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-truncation.h
Insertions: 17, Deletions: 9.
@@ -96,7 +96,13 @@
return ProcessResult::kContinue;
}
- ProcessResult Process(Identity* node) { return ProcessResult::kContinue; }
+ ProcessResult Process(Identity* node) {
+ return ProcessSingleInputSpecialNode(node);
+ }
+ ProcessResult Process(ReturnedValue* node) {
+ return ProcessSingleInputSpecialNode(node);
+ }
+
ProcessResult Process(Dead* node) { return ProcessResult::kContinue; }
// TODO(victorgomes): We can only truncate CheckedHoleyFloat64ToInt32
@@ -129,14 +135,6 @@
if constexpr (IsFixedInputNode<NodeT>()) {
return UnsetCanTruncateToInt32ForFixedInputNodes<NodeT, 0>(node);
}
- if constexpr (std::is_same_v<NodeT, ReturnedValue> ||
- std::is_same_v<NodeT, Identity>) {
- ValueNode* input_node = node->input_node(0);
- if (input_node->value_representation() != ValueRepresentation::kTagged) {
- input_node->set_can_truncate_to_int32(false);
- }
- return;
- }
#ifdef DEBUG
for (Input input : node->inputs()) {
DCHECK(!input.node()->can_truncate_to_int32());
@@ -144,6 +142,16 @@
#endif // DEBUG
}
+ ProcessResult ProcessSingleInputSpecialNode(ValueNode* node) {
+ if (!node->can_truncate_to_int32()) {
+ ValueNode* input_node = node->input_node(0);
+ if (input_node->value_representation() != ValueRepresentation::kTagged) {
+ input_node->set_can_truncate_to_int32(false);
+ }
+ }
+ return ProcessResult::kContinue;
+ }
+
void UnsetCanTruncateToInt32ForDeoptFrameInput(ValueNode* node) {
// TODO(victorgomes): Technically if node is in the int32 range, this use
// would still allow truncation.
```
Change information
Commit message:
[maglev] Fix a bug in (a+b)|0 omitting overflow checks
If a function returns the result of (a+b)|0, we cannot omit the overflow
check.
This gotcha here is that UnsetCanTruncateToInt32Inputs only worked for
FixedInputNodes, and ReturnedValue is not a FixedInputNode.
Fixed: 454314508
Change-Id: Id3a5ae99332031bc864bd9928f96102f44f10802
Commit-Queue: Marja Hölttä <ma...@chromium.org>
Reviewed-by: Leszek Swirski <les...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#103360}
Files:
- M src/maglev/maglev-truncation.h
- A test/mjsunit/turbolev/regress-454314508.js
Change size: M
Delta: 2 files changed, 61 insertions(+), 8 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Leszek Swirski
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages