Put checks that detect UB under their own flag below debug_assertions #123411

saethlin · 2024-04-03T13:22:14Z

Implementation of rust-lang/compiler-team#725

Tracking issue: #123499

rustbot · 2024-04-03T13:22:23Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

saethlin · 2024-04-03T13:22:33Z

tests/codegen/ub-checks.rs

+    // CHECK-NEXT: start:
+    // CHECK-NEXT: icmp ult
+    // CHECK-NEXT: tail call void @llvm.assume
+    // CHECK-NEXT: getelementptr inbounds
+    // CHECK-NEXT: ret ptr


@scottmcm Can you offer some advice on the right way to write this test? I'm not sure how exact I should be matching against the IR.

Maybe use revisions to put tests for the presence and the absence in the same file? For inspiration, maybe look at this file, where it checks the different codegen for OPT0 and OPT3 -- you could do the same kind of thing for UB-checks on and off:

rust/tests/codegen/intrinsics/typed_swap.rs

Lines 1 to 4 in 0accf4e

//@ revisions: OPT0 OPT3

//@ [OPT0] compile-flags: -Copt-level=0

//@ [OPT3] compile-flags: -Copt-level=3

//@ compile-flags: -C no-prepopulate-passes

And if the NOUBCHECK-NOT negative check is right next to the YESUBCHECK positive check for the same thing, it's way more likely that the negative test is actually doing something meaningful.

Maybe you can also check things like that the assume optimizes out when the ub-check is on, because it ends up unneeded as the dominating branch checks the same thing?

src/tools/miri/src/lib.rs

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

compiler/rustc_session/src/options.rs

RalfJung · 2024-04-03T20:08:27Z

@rustbot author

rustbot · 2024-04-03T22:28:06Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

saethlin · 2024-04-03T22:28:23Z

@rustbot ready

RalfJung · 2024-04-04T06:06:54Z

compiler/rustc_session/src/config.rs

@@ -1236,6 +1236,10 @@ fn default_configuration(sess: &Session) -> Cfg {
        ins_none!(sym::overflow_checks);
    }

+    if sess.ub_checks() {
+        ins_none!(sym::ub_checks);


This is for cfg!(ub_checks) I assume. There's no test for that though that I can see?

Also I wonder if we need to feature-gate this. Some cfg have been feature-gated in the past.

We need some way to prevent use on stable, yeah. In the MCP you mention that cfg(overflow_checks) isn't stabilized; can we use the same mechanism (possibly even the same feature gate)?

True, that's tracked here: #111466.

Same feature gate seems strange though, then we'll have to split it up if we want to stabilize cfg(overflow_checks) before -Cub-checks.

fair, different feature gate then

RalfJung · 2024-04-04T06:07:47Z

LGTM except for the missing test, but I'll hand over to a compiler person as some of this is a bit outside my comfort zone -- in particular regarding the new cfg flag.

r? compiler

Urgau · 2024-04-05T17:49:36Z

compiler/rustc_session/src/config.rs

@@ -1236,6 +1236,10 @@ fn default_configuration(sess: &Session) -> Cfg {
        ins_none!(sym::overflow_checks);
    }

+    if sess.ub_checks() {
+        ins_none!(sym::ub_checks);


This is missing it's counter-part below in CheckCfg::fill_well_known:

rust/compiler/rustc_session/src/config.rs

Line 1392 in 6cf853c

pub fn fill_well_known(&mut self, current_target: &Target) {

Something like this should do it:

ins!(sym::ub_checks, no_values);

Please also follow the guidelines,

rust/compiler/rustc_session/src/config.rs

Lines 1415 to 1426 in 6cf853c

// Symbols are inserted in alphabetical order as much as possible.

// The exceptions are where control flow forces things out of order.

//

// NOTE: This should be kept in sync with `default_configuration`.

// Note that symbols inserted conditionally in `default_configuration`

// are inserted unconditionally here.

//

// When adding a new config here you should also update

// `tests/ui/check-cfg/well-known-values.rs`.

//

// Don't forget to update `src/doc/unstable-book/src/compiler-flags/check-cfg.md`

// in the unstable book as well!

It sounds like you told me that the previous state here is buggy. Why did the tests pass?

We don't currently sanity check that default_configuration and CheckCfg::fill_well_known are synced; we should probably do that, but it's non-trivial.

We rely on them being (manually) synced.

We also don't currently automatically enable check-cfg in compiletest, that's why CI doesn't fail. (I will send a PR to enable it by default)

Btw I don't see the cfg added in tests/ui/check-cfg/well-known-values.rs nor do I see the documentation being updated at src/doc/unstable-book/src/compiler-flags/check-cfg.md.

You will also need to bless the tests/ui/check-cfg/ directory.

Thanks. All the check-cfg changes look good.

Thanks for maintaining check-cfg. It's a great feature.

rustbot · 2024-04-05T19:37:15Z

Some changes occurred in tests/ui/check-cfg

cc @Urgau

rustbot · 2024-04-05T19:43:44Z

Some changes occurred in src/doc/unstable-book/src/compiler-flags/check-cfg.md

cc @Urgau

…ements, r=wesleywiser Improve cfg and check-cfg configuration This PR improves cfg and check-cfg configuration by: 1. Extracting both logic under a common module (to improve the connection between the two) 2. Adding more documentation, in particular some steps when adding a new cfg I also added my-self as mention in our triagebot conf for the new module. Inspired by rust-lang#123411 (comment)

Rollup merge of rust-lang#123519 - Urgau:session-cfg-check-cfg-improvements, r=wesleywiser Improve cfg and check-cfg configuration This PR improves cfg and check-cfg configuration by: 1. Extracting both logic under a common module (to improve the connection between the two) 2. Adding more documentation, in particular some steps when adding a new cfg I also added my-self as mention in our triagebot conf for the new module. Inspired by rust-lang#123411 (comment)

bors · 2024-04-06T11:30:17Z

☔ The latest upstream changes (presumably #123540) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-04-06T15:27:12Z

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

saethlin · 2024-04-06T16:30:06Z

@bors r=Urgau,RalfJung

bors · 2024-04-06T16:30:09Z

📌 Commit a7912cb has been approved by Urgau,RalfJung

It is now in the queue for this repository.

Put checks that detect UB under their own flag below debug_assertions Implementation of rust-lang/compiler-team#725

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#119224 (Drop panic hook after running tests) - rust-lang#123411 (Put checks that detect UB under their own flag below debug_assertions) - rust-lang#123446 (Fix incorrect 'llvm_target' value used on watchOS target) - rust-lang#123516 (Do not ICE on field access check on expr with `ty::Error`) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#119224 (Drop panic hook after running tests) - rust-lang#123411 (Put checks that detect UB under their own flag below debug_assertions) - rust-lang#123516 (Do not ICE on field access check on expr with `ty::Error`) - rust-lang#123522 (Stabilize const Atomic*::into_inner) - rust-lang#123559 (Add a debug asserts call to match_projection_projections to ensure invariant) - rust-lang#123563 (Rewrite `version` test run-make as an UI test) Failed merges: - rust-lang#123569 (Move some tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123411 - saethlin:ub-checks, r=Urgau,RalfJung Put checks that detect UB under their own flag below debug_assertions Implementation of rust-lang/compiler-team#725

rustbot assigned RalfJung Apr 3, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 3, 2024

saethlin commented Apr 3, 2024

View reviewed changes

RalfJung reviewed Apr 3, 2024

View reviewed changes

src/tools/miri/src/lib.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 3, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/rvalue.rs Show resolved Hide resolved

RalfJung reviewed Apr 3, 2024

View reviewed changes

compiler/rustc_session/src/options.rs Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2024

saethlin force-pushed the ub-checks branch from 294d980 to 0ce678c Compare April 3, 2024 22:28

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2024

RalfJung reviewed Apr 4, 2024

View reviewed changes

rustbot assigned Nadrieril and unassigned RalfJung Apr 4, 2024

saethlin mentioned this pull request Apr 5, 2024

Tracking Issue for feature(ub_checks) and -Zub-checks #123499

Open

3 tasks

saethlin force-pushed the ub-checks branch from 0ce678c to 6cf853c Compare April 5, 2024 15:59

Urgau requested changes Apr 5, 2024

View reviewed changes

saethlin force-pushed the ub-checks branch 2 times, most recently from 420414f to 6edde24 Compare April 5, 2024 18:25

This comment has been minimized.

Sign in to view

saethlin force-pushed the ub-checks branch from 6edde24 to 33576b3 Compare April 5, 2024 19:37

saethlin force-pushed the ub-checks branch from 33576b3 to a5bdd36 Compare April 5, 2024 19:43

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 5, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2024

Put checks that detect UB under their own flag below debug_assertions

a7912cb

saethlin force-pushed the ub-checks branch from 0ae1f30 to a7912cb Compare April 6, 2024 15:27

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2024

GuillaumeGomez mentioned this pull request Apr 6, 2024

Rollup of 4 pull requests #123565

Closed

matthiaskrgr mentioned this pull request Apr 6, 2024

Rollup of 6 pull requests #123576

Merged

bors merged commit 84dca15 into rust-lang:master Apr 7, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Apr 7, 2024

saethlin deleted the ub-checks branch April 7, 2024 02:14

saethlin mentioned this pull request Apr 7, 2024

Move alignment checks to codegen #117473

Draft

RalfJung mentioned this pull request Apr 7, 2024

Severe perf regression in optimized debug builds due to extra UB checks #121245

Closed

ojeda mentioned this pull request Apr 7, 2024

Rust unstable features needed for the kernel Rust-for-Linux/linux#2

Open

83 tasks

saethlin linked an issue Apr 7, 2024 that may be closed by this pull request

Put "checks that detect UB" under their own flag below debug_assertions #122678

Closed

saethlin mentioned this pull request Apr 7, 2024

Put "checks that detect UB" under their own flag below debug_assertions #122678

Closed

fmease mentioned this pull request Apr 10, 2024

compiletest silently fails on a name-value directive with a known name but missing colon but does not report an error #123760

Open

saethlin mentioned this pull request Apr 12, 2024

Totally-not-a-tracking-issue for UB-detecting debug assertions in the standard library #120848

Open

9 tasks

ojeda mentioned this pull request May 5, 2024

Rust wanted features Rust-for-Linux/linux#354

Open

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put checks that detect UB under their own flag below debug_assertions #123411

Put checks that detect UB under their own flag below debug_assertions #123411

saethlin commented Apr 3, 2024 •

edited by RalfJung

Loading

rustbot commented Apr 3, 2024

saethlin Apr 3, 2024 •

edited

Loading

scottmcm Apr 4, 2024

RalfJung commented Apr 3, 2024

rustbot commented Apr 3, 2024

saethlin commented Apr 3, 2024

RalfJung Apr 4, 2024

Nadrieril Apr 4, 2024

RalfJung Apr 4, 2024

Nadrieril Apr 4, 2024

RalfJung commented Apr 4, 2024

Urgau Apr 5, 2024

saethlin Apr 5, 2024

Urgau Apr 5, 2024 •

edited

Loading

Urgau Apr 5, 2024

saethlin Apr 5, 2024

This comment has been minimized.

rustbot commented Apr 5, 2024

rustbot commented Apr 5, 2024

bors commented Apr 6, 2024

rustbot commented Apr 6, 2024

saethlin commented Apr 6, 2024

bors commented Apr 6, 2024

	//@ revisions: OPT0 OPT3
	//@ [OPT0] compile-flags: -Copt-level=0
	//@ [OPT3] compile-flags: -Copt-level=3
	//@ compile-flags: -C no-prepopulate-passes

	// Symbols are inserted in alphabetical order as much as possible.
	// The exceptions are where control flow forces things out of order.
	//
	// NOTE: This should be kept in sync with `default_configuration`.
	// Note that symbols inserted conditionally in `default_configuration`
	// are inserted unconditionally here.
	//
	// When adding a new config here you should also update
	// `tests/ui/check-cfg/well-known-values.rs`.
	//
	// Don't forget to update `src/doc/unstable-book/src/compiler-flags/check-cfg.md`
	// in the unstable book as well!

Put checks that detect UB under their own flag below debug_assertions #123411

Put checks that detect UB under their own flag below debug_assertions #123411

Conversation

saethlin commented Apr 3, 2024 • edited by RalfJung Loading

rustbot commented Apr 3, 2024

saethlin Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Apr 3, 2024

rustbot commented Apr 3, 2024

saethlin commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urgau Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Apr 5, 2024

rustbot commented Apr 5, 2024

bors commented Apr 6, 2024

rustbot commented Apr 6, 2024

saethlin commented Apr 6, 2024

bors commented Apr 6, 2024

saethlin commented Apr 3, 2024 •

edited by RalfJung

Loading

saethlin Apr 3, 2024 •

edited

Loading

Urgau Apr 5, 2024 •

edited

Loading