Add config.toml options for enabling overflow checks in rustc and std #87784

hkratz · 2021-08-05T05:35:48Z

The names are overflow-checks and overflow-checks-std and they work similar to debug-assertions and debug-assertions-std. Once added we can measure how big the performance impact actually is and maybe enable them for CI tests.

Enabling them already makes two ui tests fail:

failures:
    [ui] ui/parser/item-free-const-no-body-semantic-fail.rs
    [ui] ui/parser/item-free-static-no-body-semantic-fail.rs

(See #84219 and #87761.)

rust-highfive · 2021-08-05T05:35:51Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

hkratz · 2021-08-05T08:35:04Z

Performance impact for running the full stage 1 testsuite locally is less than I expected:

# With fix from #87761 for the failing tests.
./x.py clean; time ./x.py test --stage 1

# no checks:
real	23m19.492s
user	176m22.251s
sys	20m28.837s

# w/ overflow checks in everything but std:
real	23m26.973s
user	176m50.005s
sys	20m32.393s

# w/ overflow checks in everything:
real	23m57.156s
user	181m20.771s
sys	21m25.673s

Mark-Simulacrum · 2021-08-05T11:41:52Z

Could you change them to default to true in this PR temporarily, so we can run perf on this PR? I think if we can get away with not exposing this as an option (and instead always enabling them) we should.

hkratz · 2021-08-05T13:40:39Z

Could you change them to default to true in this PR temporarily, so we can run perf on this PR? I think if we can get away with not exposing this as an option (and instead always enabling them) we should.

@Mark-Simulacrum Done. This should be ready for a perf test.

Mark-Simulacrum · 2021-08-05T13:54:48Z

@bors try @rust-timer queue

rust-timer · 2021-08-05T13:54:50Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-05T13:54:56Z

⌛ Trying commit 72ada176db4853c7fb922e4b3efbdcb2b7df655c with merge f9bbcb152f7bd5dba5923c3c42999ee3c78aae98...

bors · 2021-08-05T15:08:01Z

☀️ Try build successful - checks-actions
Build commit: f9bbcb152f7bd5dba5923c3c42999ee3c78aae98 (f9bbcb152f7bd5dba5923c3c42999ee3c78aae98)

rust-timer · 2021-08-05T15:08:03Z

Queued f9bbcb152f7bd5dba5923c3c42999ee3c78aae98 with parent e21e1d6, future comparison URL.

rust-timer · 2021-08-05T17:20:23Z

Finished benchmarking try commit (f9bbcb152f7bd5dba5923c3c42999ee3c78aae98): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 23.8% on incr-unchanged builds of ctfe-stress-4-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

hkratz · 2021-08-05T18:22:27Z

Hmm, at least now we know.

(Sorry the Close and Reopen was an accident on my mobile)

Mark-Simulacrum · 2021-08-05T18:53:05Z

Could you drop the enablement for std and we can rerun benchmarks? I'd be interested in just rustc enablement impact.

I expect it'd be interesting to work out if there's just a few adds or whatever we can replace with explicit wrapping adds (with comments about performance or so). But that's a larger project, I just want numbers for now on the rustc-only overflow checks and we can likely merge after that.

hkratz · 2021-08-05T21:10:43Z

@Mark-Simulacrum Ready for perf testing.

Mark-Simulacrum · 2021-08-05T23:15:22Z

@bors try @rust-timer queue

rust-timer · 2021-08-05T23:15:24Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-08-05T23:15:30Z

⌛ Trying commit 1ee9083f12cf38e66e4c7bf2fc18a2d1c062f5ee with merge d570e77cc2384b466568c2ebc072def445db0f1b...

bors · 2021-08-06T00:00:50Z

☀️ Try build successful - checks-actions
Build commit: d570e77cc2384b466568c2ebc072def445db0f1b (d570e77cc2384b466568c2ebc072def445db0f1b)

rust-timer · 2021-08-06T00:00:52Z

Queued d570e77cc2384b466568c2ebc072def445db0f1b with parent 2f07ae4, future comparison URL.

rust-timer · 2021-08-06T01:42:25Z

Finished benchmarking try commit (d570e77cc2384b466568c2ebc072def445db0f1b): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 21.4% on incr-unchanged builds of ctfe-stress-4-check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Mark-Simulacrum · 2021-08-06T02:46:07Z

Alright, thanks! Looks like neither is an option for now as default, so if you can drop the testing commits we can probably merge (I'll give another glance over the code but don't think there's likely to be more work before merging).

src/bootstrap/CHANGELOG.md

Mark-Simulacrum · 2021-08-06T11:36:42Z

r=me after last nit is fixed

The options are `overflow-checks` and `overflow-checks-std` defaulting to false.

Mark-Simulacrum · 2021-08-06T12:11:24Z

@bors r+

Thanks!

bors · 2021-08-06T12:11:25Z

📌 Commit 2f70953 has been approved by Mark-Simulacrum

bors · 2021-08-06T15:35:05Z

⌛ Testing commit 2f70953 with merge 24380a6...

bors · 2021-08-06T18:23:08Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 24380a6 to master...

jyn514 · 2021-10-12T16:47:00Z

src/bootstrap/config.rs

@@ -968,6 +976,8 @@ impl Config {
        config.rust_debug_assertions = debug_assertions.unwrap_or(default);
        config.rust_debug_assertions_std =
            debug_assertions_std.unwrap_or(config.rust_debug_assertions);
+        config.rust_overflow_checks = overflow_checks.unwrap_or(default);
+        config.rust_overflow_checks_std = overflow_checks_std.unwrap_or(default);


Shouldn't this be .unwrap_or(rust_overflow_checks)? Otherwise overflow_checks = true won't actually enable it for the standard library.

I just noticed that too while working on #89776. It would be more in line with how rust_debug_assertions and rust_debug_assertions_std behave.

I will put that and the stuff missing from config.toml.example and configure.py into a followup PR.

…take-two, r=Mark-Simulacrum Fix config.toml overflow-checks options This a follow-up PR to rust-lang#87784. Improvements: * Add missing entries for overflow-checks to config.toml.example. * Add --enable-overflow-checks-std option to configure script. * Make rust.overflow-checks-stdoption default to rust.overflow-checks. Also adds the missing `--enable-debug-assertions-std `option to configure script. r? `@Mark-Simulacrum` cc `@jyn514`

…take-two, r=Mark-Simulacrum Fix config.toml overflow-checks options This a follow-up PR to rust-lang#87784. Improvements: * Add missing entries for overflow-checks to config.toml.example. * Add --enable-overflow-checks-std option to configure script. * Make rust.overflow-checks-stdoption default to rust.overflow-checks. Also adds the missing `--enable-debug-assertions-std `option to configure script. r? ``@Mark-Simulacrum`` cc ``@jyn514``

…take-two, r=Mark-Simulacrum Fix config.toml overflow-checks options This a follow-up PR to rust-lang#87784. Improvements: * Add missing entries for overflow-checks to config.toml.example. * Add --enable-overflow-checks-std option to configure script. * Make rust.overflow-checks-stdoption default to rust.overflow-checks. Also adds the missing `--enable-debug-assertions-std `option to configure script. r? ```@Mark-Simulacrum``` cc ```@jyn514```

rust-highfive assigned Mark-Simulacrum Aug 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2021

hkratz changed the title ~~Add options for enabling overflow checks in rustc and std in config.toml~~ Add config.toml options for enabling overflow checks in rustc and std Aug 5, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 5, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 5, 2021

hkratz closed this Aug 5, 2021

hkratz reopened this Aug 5, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 5, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 6, 2021

hkratz force-pushed the bootstrap_config_overflow_checks branch from 1ee9083 to 7f34b96 Compare August 6, 2021 05:26

Mark-Simulacrum reviewed Aug 6, 2021

View reviewed changes

src/bootstrap/CHANGELOG.md Outdated Show resolved Hide resolved

Mark-Simulacrum removed the perf-regression Performance regression. label Aug 6, 2021

Add options for enabling overflow checks in rustc and std.

2f70953

The options are `overflow-checks` and `overflow-checks-std` defaulting to false.

hkratz force-pushed the bootstrap_config_overflow_checks branch from 7f34b96 to 2f70953 Compare August 6, 2021 12:04

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2021

bors merged commit 24380a6 into rust-lang:master Aug 6, 2021

rustbot added this to the 1.56.0 milestone Aug 6, 2021

hkratz deleted the bootstrap_config_overflow_checks branch August 6, 2021 18:24

jyn514 reviewed Oct 12, 2021

View reviewed changes

This was referenced Oct 12, 2021

Fix config.toml overflow-checks options #89828

Merged

CI: Enable overflow checks for test (non-dist) builds #89776

Merged

dtolnay mentioned this pull request Sep 7, 2024

ICE: symbol-mangling-version=v0 attempt to subtract with overflow #130011

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config.toml options for enabling overflow checks in rustc and std #87784

Add config.toml options for enabling overflow checks in rustc and std #87784

hkratz commented Aug 5, 2021

rust-highfive commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

rust-timer commented Aug 5, 2021

bors commented Aug 5, 2021

bors commented Aug 5, 2021

rust-timer commented Aug 5, 2021

rust-timer commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

rust-timer commented Aug 5, 2021

bors commented Aug 5, 2021

bors commented Aug 6, 2021

rust-timer commented Aug 6, 2021

rust-timer commented Aug 6, 2021

Mark-Simulacrum commented Aug 6, 2021

Mark-Simulacrum commented Aug 6, 2021

Mark-Simulacrum commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

jyn514 Oct 12, 2021

hkratz Oct 12, 2021

hkratz Oct 12, 2021

Add config.toml options for enabling overflow checks in rustc and std #87784

Add config.toml options for enabling overflow checks in rustc and std #87784

Conversation

hkratz commented Aug 5, 2021

rust-highfive commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

rust-timer commented Aug 5, 2021

bors commented Aug 5, 2021

bors commented Aug 5, 2021

rust-timer commented Aug 5, 2021

rust-timer commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

hkratz commented Aug 5, 2021

Mark-Simulacrum commented Aug 5, 2021

rust-timer commented Aug 5, 2021

bors commented Aug 5, 2021

bors commented Aug 6, 2021

rust-timer commented Aug 6, 2021

rust-timer commented Aug 6, 2021

Mark-Simulacrum commented Aug 6, 2021

Mark-Simulacrum commented Aug 6, 2021

Mark-Simulacrum commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

bors commented Aug 6, 2021

jyn514 Oct 12, 2021

Choose a reason for hiding this comment

hkratz Oct 12, 2021

Choose a reason for hiding this comment

hkratz Oct 12, 2021

Choose a reason for hiding this comment