fix(cranelift): use saturating_sub in PassTimes::total to prevent panic#12694
Closed
A386official wants to merge 2 commits intobytecodealliance:mainfrom
Closed
fix(cranelift): use saturating_sub in PassTimes::total to prevent panic#12694A386official wants to merge 2 commits intobytecodealliance:mainfrom
A386official wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
PassTimes::total() subtracts child duration from total duration using the `-` operator, which panics on underflow. When timing tokens are dropped out of order (possible in release builds where the debug_assert in Drop is elided), child can exceed total, causing: "overflow when subtracting durations" The Display impl already handles this correctly using checked_sub. Use saturating_sub in total() for consistency and to prevent the panic in production. Fixes #12692
Break method chain across multiple lines to satisfy rustfmt's line length requirements. Signed-off-by: A386official <a386official@users.noreply.github.com>
Author
|
Closing in favor of #12693 which addresses the same issue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
PassTimes::total()panics with "overflow when subtracting durations" when compiling certain WASM modules. This was reported by Zed via telemetry on Windows x86 with Cranelift 33.0.2.Stack trace:
Root Cause
PassTimes::total()computes self-time per pass asp.total - p.childusing the-operator onDuration, which panics on underflow. When timing tokens are dropped out of order (which is guarded by adebug_assertthat is elided in release builds), the accumulatedchildduration can exceedtotal, triggering the panic.The
Displayimplementation forPassTimesalready handles this edge case correctly by usingchecked_sub:But
total()was not given the same treatment.Fix
Replace
p.total - p.childwithp.total.saturating_sub(p.child)inPassTimes::total(). This is consistent with how theDisplayimpl handles the same subtraction and prevents the panic in production builds where the ordering assertion is not checked.Closes #12692