-
Notifications
You must be signed in to change notification settings - Fork 17.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime/internal/atomic: consider making Xadd panic on underflow #13876
Comments
Xadd is now an intrinsic on amd64, which will complicate matters. |
On the other hand, the intrinsic makes it cheaper to add a wrapper that checks for under/overflow (though the |
I don't think we'd need an explicit (Or does dereferencing possibly-nil pointers today also prevent inlining?) |
@bcmills |
So it does. (#TIL!) Still, why would the |
Both throw and explicit calls to panic do prevent inlining right now. It's only implicit panics (inserted by the compiler for things like bounds checks or triggered by signals) that don't inhibit inlining. Mid-stack inlining will probably make this a non-concern. There's nothing special about throw/panic that inhibits inlining. The inliner just sees them as calls, so the caller is a non-leaf function. |
I've sunk a lot of time in to debugging Xadds in the runtime that unintentionally underflow and cause the unsigned number to wrap, leading to some (often subtle) horrible thing happening later. This debugging effort means the runtime is now sprinkled with underflow checks in many places where it calls Xadd.
We should consider making Xadd itself do this check, or introducing another variant that does. One caveat is that we currently don't have a signed version of Xadd, so there are places where we use it on a *uint32, but cast that unsigned value to a signed value everywhere we use it. In this case we definitely don't want to panic on unsigned underflow, so we should introduce a signed Xadd for these that takes a *int32 and doesn't do this check (though it could panic on signed wrap-around).
/cc @michaelmatloob @rsc @dvyukov for any thoughts.
The text was updated successfully, but these errors were encountered: