Skip to content
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

Open
aclements opened this issue Jan 8, 2016 · 6 comments
Open

runtime/internal/atomic: consider making Xadd panic on underflow #13876

aclements opened this issue Jan 8, 2016 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@aclements
Copy link
Member

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.

@aclements aclements added this to the Go1.7Early milestone Jan 8, 2016
@bradfitz bradfitz modified the milestones: Go1.8Early, Go1.7Early May 5, 2016
@randall77 randall77 changed the title runtime/atomic: consider making Xadd panic on underflow runtime/internal/atomic: consider making Xadd panic on underflow Sep 1, 2016
@randall77
Copy link
Contributor

Xadd is now an intrinsic on amd64, which will complicate matters.

@aclements
Copy link
Member Author

On the other hand, the intrinsic makes it cheaper to add a wrapper that checks for under/overflow (though the throw would still prevent inlining of the wrapper.)

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 30, 2016
@rsc rsc modified the milestones: Unplanned, Go1.8Early Oct 18, 2016
@bcmills
Copy link
Contributor

bcmills commented Apr 14, 2017

I don't think we'd need an explicit throw: with a cmov to a guaranteed-invalid address, we could capture the signal with the same SIGSEGV handler that we use today for nil-pointer dereferences.

(Or does dereferencing possibly-nil pointers today also prevent inlining?)

@josharian
Copy link
Contributor

@bcmills cmov always faults when given an invalid address, even if the condition is not satisfied.

@bcmills
Copy link
Contributor

bcmills commented Apr 16, 2017

So it does. (#TIL!)

Still, why would the throw prevent inlining when panics in general do not? (And does mid-stack inlining improve the situation at all?)

@aclements
Copy link
Member Author

Still, why would the throw prevent inlining when panics in general do not? (And does mid-stack inlining improve the situation at all?)

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.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants