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

Document arithmetic overflow behavior for atomic fetch_add and fetch_sub operations #34618

Closed
mbrubeck opened this issue Jul 3, 2016 · 12 comments

Comments

@mbrubeck
Copy link
Contributor

mbrubeck commented Jul 3, 2016

Are these operations guaranteed to wrap on overflow? The documentation currently says nothing.

@nagisa
Copy link
Member

nagisa commented Jul 3, 2016

undefined behaviour is all we can guarantee, I think.

@nagisa
Copy link
Member

nagisa commented Jul 3, 2016

Though it might be possible to implement overflow-checked fetch_add/sub in overflow-checked mode by doing CAS under the cover, to match the behaviour of operations with regular integers.

@hanna-kruppe
Copy link
Contributor

LangRef doesn't say anything about overflow. Perhaps LLVM effectively assumes nsw/nuw but since that is not the default for the analogous "normal" operations, it's really impossible to guess. Perhaps ask LLVM developers?

@eefriedman
Copy link
Contributor

The LLVM atomicrmw operations are guaranteed to wrap on overflow. (I would never have guessed that anyone would assume anything else; I'll look into getting it documented.)

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jul 3, 2016

undefined behaviour is all we can guarantee, I think.

I hope you mean we guarantee no undefined behavior, since these are all safe functions.

@nagisa
Copy link
Member

nagisa commented Jul 3, 2016

@mbrubeck usually overflowing an integer during operations is considered to be UB, which is why I said that.

@eefriedman where does LLVM guarantee that?

@eefriedman
Copy link
Contributor

http://llvm.org/docs/LangRef.html#add-instruction : "If the sum has unsigned overflow, the result returned is the mathematical result modulo 2n, where n is the bit width of the result." Also, it would be impossible to implement C atomic add on top of atomicrmw if it had any other behavior.

@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jul 3, 2016

usually overflowing an integer during operations is considered to be UB, which is why I said that.

Only in C and C++, right? In Rust, arithmetic overflow is never undefined behavior.

@nagisa
Copy link
Member

nagisa commented Jul 3, 2016

I don’t remember Rust making any guarantees about things which happen if integers overflow when overflow checks are not enabled, but it might just be me and my bad memory.

@nagisa
Copy link
Member

nagisa commented Jul 3, 2016

RFC 560 claims overflows are not UB in the C sense of UB, but still are a program error. I think we should strive to have atomics behave similarly.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jul 3, 2016

Safe code must never be able to trigger UB. And although "unspecified" results on overflow (i.e., some result which may not be relied on but has no side effects) were considered for a while, the RFC now says that it always wraps:

The operations +, -, *, can underflow and overflow. When checking is enabled this will panic. When checking is disabled this will two's complement wrap.

@Amanieu
Copy link
Member

Amanieu commented Jul 4, 2016

The C++11 atomic ops guarantee wrapping behavior for add and sub (for both signed and unsigned integers). We should do the same.

bors added a commit that referenced this issue Apr 2, 2017
…hton

Add a note about overflow for fetch_add/fetch_sub

Fixes #40916
Fixes #34618

r? @steveklabnik
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 3, 2017
… r=alexcrichton

Add a note about overflow for fetch_add/fetch_sub

Fixes rust-lang#40916
Fixes rust-lang#34618

r? @steveklabnik
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 3, 2017
… r=alexcrichton

Add a note about overflow for fetch_add/fetch_sub

Fixes rust-lang#40916
Fixes rust-lang#34618

r? @steveklabnik
arielb1 pushed a commit to arielb1/rust that referenced this issue Apr 5, 2017
… r=alexcrichton

Add a note about overflow for fetch_add/fetch_sub

Fixes rust-lang#40916
Fixes rust-lang#34618

r? @steveklabnik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants