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

Comments

@aclements
Copy link
Member

commented Jan 8, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2016

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

@aclements

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2016

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.)

@bcmills

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

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

@bcmills

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.