-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
CI: Enable overflow checks for test (non-dist) builds #89776
Conversation
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@rustbot label -S-waiting-on-review +S-waiting-on-author |
This comment has been minimized.
This comment has been minimized.
Currently fails due to #89639. |
@rustbot label -S-waiting-on-author +S-blocked |
FWIW I think you're going to find that
#72549 fails as well.
…On Mon, Oct 11, 2021 at 2:28 PM Hans Kratz ***@***.***> wrote:
rustbot label -S-waiting-on-author +S-blocked
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#89776 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBAKSOGSWVC5HG4DGK3UGMUDBANCNFSM5FYXUERA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
All that is necessary for evil to succeed is for good people to do nothing.
|
aade54c
to
d4a6675
Compare
This comment has been minimized.
This comment has been minimized.
d4a6675
to
b95d77b
Compare
This comment has been minimized.
This comment has been minimized.
Do we have a recent-ish measurement of how much this would slow down the compiler if enabled by default? I feel like there was a measurement a few months (years?) back... Even if we don't enable the overflow checks within std, it seems reasonable to enable them for the compiler only, if we can afford it. |
@Mark-Simulacrum We have measured this as part of #87784 in August. Perf results: The slowdown is significant so to distributing rustc/std with overflow checks enabled is not a good idea. But I think it makes sense to enable the overflow checks for non-Apple non-dist CI builds. Debug assertions are enabled for those builds already and the overall impact on the CI (at least on The slowest CI builds are the Apple ones anyway and for those we would keep it disabled (just as with debug assertions). |
Thanks. Yeah, I recall those measurements now. Agreed we likely can't accept that hit just yet. I'm fine with us merging this and we can evaluate the CI time impact if it becomes a problem, it seems reasonable to have our test builders run with more assertions where we can afford it. |
b95d77b
to
d0387bc
Compare
will also fail the tools builder due to #89280 which breaks clippy tests |
383348a
to
5ef1550
Compare
5ef1550
to
5c8fca5
Compare
@bors r=Mark-Simulacrum rollup=iffy |
📌 Commit 5c8fca5 has been approved by |
⌛ Testing commit 5c8fca5 with merge 8ed99dd012f1305de11e33ffcb82e7f193b2feee... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors retry - missing search.js in rustdoc-js (?) tests |
☀️ Test successful - checks-actions |
Finished benchmarking commit (a99c9d6): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
It seems that enabling this uncovers non-deterministic overflows in rustc-rayon-core causing the build failures of #90042 and #90222:
Overflow location: This code is not in upstream rayon but just in our fork, introduced here: |
. rust-lang#89776 enabled overflow checks in CI but these lead to two failures already: rust-lang#90042 (comment) rust-lang#90222 (comment) The (first?) problem has been identified: rust-lang#90227 This PR temporarily disables the overflow checks again so we don't have to deal with the "spurious" CI failures until rustc-rayon is fixed. Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/ci.20failed.20with.20.20.22attempt.20to.20subtract.20with.20overflow.22
They stay disabled for Apple builds though, which take the most time already due to running on slow hw.