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

Stabilize -C overflow-checks #1535

Merged
merged 2 commits into from Apr 21, 2016

Conversation

Projects
None yet
8 participants
@brson
Copy link
Contributor

brson commented Mar 9, 2016

Stabilize the -C overflow-checks command line argument.

This is an easy way to turn on overflow checks in release builds
without otherwise turning on debug assertions, via the -C debug-assertions flag. In stable Rust today you can't get one without
the other.

Rendered.

@@ -1,36 +0,0 @@
- Feature Name: (fill me in with a unique ident, my_awesome_feature)

This comment has been minimized.

@sfackler

sfackler Mar 9, 2016

Member

Probably don't want to delete htis.

```

This may also be accomplished by Cargo's pending support for passing
arbitrary flags to rustc.

This comment has been minimized.

@nrc

nrc Mar 13, 2016

Member

I prefer to wait for this, rather than add more ad hoc options to Cargo

[alternatives]: #alternatives

The flag could instead be tied to crates such that any time code from
that crate is inlined/monomorphized it turns on overflow checks.

This comment has been minimized.

@nrc

nrc Mar 13, 2016

Member

I would like to hear people's opinions on this - it seems significant, but I don't have a good idea of how significant.

This comment has been minimized.

@golddranks

golddranks Mar 14, 2016

I'd imagine that if the option was crate-specific, that would be an incentive for the crate authors to rely on specific overflow semantics, which is undesirable.

@ubsan

This comment has been minimized.

Copy link
Contributor

ubsan commented Mar 15, 2016

This sounds amazing :D

@main--

This comment has been minimized.

Copy link

main-- commented Mar 15, 2016

I'd imagine that if the option were crate-specific, that would be an incentive for the crate authors to rely on specific overflow semantics, which is undesirable.

Is it? I'd say it's a perfectly reasonable choice for a crate to request overflow checks on all arithmetic operations except where explicitly disabled by using Wrapping<T>. I can for example see myself enabling this for a crate that deals heavily with unsafe code where integer overflow bugs could be disastrous.

@golddranks

This comment has been minimized.

Copy link

golddranks commented Mar 15, 2016

Ah, indeed, I was thinking it another way: if a crate has the option to NOT have overflow checks, then it might be a port to subtle undefined behaviour on different architectures.

Edit: To elaborate, the scenario I was thinking is: even if the checks are always on on debug, the crate author might be the only person to run the crate on debug, and not uncover some bugs. If he/she has the power to opt out of the overflow checks for the release version of his/her crate, then no users ever see that crate panicking on overflow, and they see the effects of overflows as device-specific functionality. Then, as an even rarer case, some user with a different architecture uses the crate, and once in a decade, his software is going to crash, and nobody ever knows why.

On the other hand, if the users are able to change the behaviour of the crate to panicking, then more people are likely to do so, and the bugs are found with a larger probability.

So, I wouldn't mind if there were a per-crate option: "always panicking" OR "either panics or silently overflows, decided by downstream", as long as there is no per-crate option "always silently overflow".

@golddranks

This comment has been minimized.

Copy link

golddranks commented Mar 15, 2016

Actually, now that I think of it, it would be pretty nice if you could build with overflow-checks on or off, but an individual crate or function could opt from "downstream decides" to "always panic".

Just a confirmation: as it currently stands in the RFC text, the flag -C overflow-checks=off doesn't preclude the compiler from doing per-crate or per-function overflow checks, if such functionality were later added, does it?

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Apr 5, 2016

Just a confirmation: as it currently stands in the RFC text, the flag -C overflow-checks=off doesn't preclude the compiler from doing per-crate or per-function overflow checks, if such functionality were later added, does it?

I think that's right. If we had explicit overflow annotations in code they would take precedence.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 5, 2016

🔔 This RFC is now entering its week-long final comment period 🔔

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 21, 2016

The tools team discussed this RFC during triage the other day and the conclusion was to merge. Thanks again for the discussion!

@alexcrichton alexcrichton merged commit 65fcd92 into rust-lang:master Apr 21, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 21, 2016

Tracking issue: rust-lang/rust#33134

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