rustbot
commented
9 months ago This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:
@rustbot concern reason-for-concern
<description of the concern> Concerns can be lifted with:
@rustbot resolve reason-for-concern See documentation at https://forge.rust-lang.org
cc @rust-lang/compiler @rust-lang/compiler-contributors
oli-obk
apiraino
Proposal
Integer overflows are not good (citation needed). They haven't caused us huge trouble in the past, but we also haven't turned on the lights for our users, so maybe people are hitting them all the time in the dark (this seems a bit unlikely) but even if they aren't there is a clear benefit in preventing bugs. Especially as an overflow could result in some quite bad bugs (I don't think this has happened before, but could always happen).
Examples of previous (not that bad) integer overflow bugs: rust-lang/rust#104352 rust-lang/rust#119497. I've also done a crater run of overflow checks recently and they were similarly uninteresting, only finding one of the bugs above: https://crater-reports.s3.amazonaws.com/pr-119440-1/index.html
This motivation seems solid but a bit weak. Let's look whether it holds up against the downsides.
Glad you asked! So did I. I'm not a benchmark suite, so asked rustc-perf instead. Many significant icount regression, few significant cycle regressions. Cachegrind revealed that it was mostly just checks in a few hot functions like siphash, so I removed their checks and tried again, now with mostly sub 1% icount regressions and no significant cycle count or wall time regressions.
rustc-perf did say ACTION NEEDED on the perf report so I'm doing the action of proposing to merge this.
Note that this also increases librustc_driver.so binary size by 3%. It's the only downside I see, which is not very big (literally) but also not awesome of course.
The icount regressions are large, but I have no reason to trust them to be useful. We use icounts as the primary metrics because they're a very not-noisy mostly decent-for-the-average-perf-change metric, but here they're pretty bad, since they penalize well-predicted branches (which this is, unless we start having a large number of overflows :D). So I don't really care about them, cycles are more revealing, and they don't say anything, and I'm interpreting their silence as an approval.
Another slight downside is that it probably does make sense to remove the checks from the hot code, which makes the code a bit uglier as it requires using explicit methods (I used wrapping in my PR, but it probably makes sense to have some sort of "overflow checks when debug_assertions are enabled" method) for them. It might also require care about this in the future if any very hot integer arithmetic is added and it might be non-obvious to contributors that their integer operations are being overflow checked.
One open question is the scope of this. We certainly don't want to do it for the standard library as well in this proposal (as for integer heavy code, there certainly is a perf impact). Doing it for all tooling sounds fine to me too, as the tooling probably also has the same perf effect (rustdoc is benched as well thanks to perf) but we may want to ask the teams too. This MCP is mostly about librustc_driver.so, but I'd imagine other teams may be interested as well.
I collected the data for this in https://github.com/rust-lang/rust/pull/119440
Mentors or Reviewers
anyone ought to be brave enough to review this minor config change
Process
The main points of the Major Change Process are as follows:
@rustbot second.-C flag, then full team check-off is required.@rfcbot fcp mergeon either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.