refactor check_{lang,library}_ub: use a single intrinsic #122629

RalfJung · 2024-03-17T09:32:52Z

This enacts the plan I laid out here: use a single intrinsic, called ub_checks (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of debug_assertions (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library.

This makes it easier to do something like #122282 in the future: that just slightly alters the semantics of ub_checks (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time.

The first commit just moves things around; I don't think these macros and functions belong into intrinsics.rs as they are not intrinsics.

r? @saethlin

rustbot · 2024-03-17T09:33:03Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

RalfJung · 2024-03-17T09:36:02Z

compiler/stable_mir/src/mir/body.rs

-pub enum UbKind {
-    LanguageUb,
-    LibraryUb,
+    UbChecks,


I see no choice but to change smir here; the things it exposed were never meant to be very stable to begin with and I don't think we want to keep them.

I've just been breaking smir every time I change MIR. Is there any guidance or enforcement that we don't do that?

No idea. SMIR people get pinged when this happens so I guess they'll complain when it gets too much for them. ;)

It would be nice to avoid these breakages. If something is unstable, we recommend to not expose them, or expose as an Opaque type.

See Coverage statement as an example.

In this specific case, we could just always use the same UbKind value, and mark UbValue as deprecated.

If we leave compatibility shims in stable MIR (not that I think we should do that in this PR), is there a plan for how to remove them eventually?

StableMIR is not yet versioned, but once it is, we would likely go over things that has been marked as deprecated and remove them when a new major is about to be released.

@RalfJung I think it is fine to remove it now.

I do have a question about how to evaluate this operator though. Is the idea here that we are assessing the value of cfg!(debug_assertions) for the crate being compiled? So if the MIR comes from a dependency that was compiled with debug_assertions off, but the current crate has the debug_assertions on, this rvalue will be evaluated to true. Is that correct?

Is it possible to document the answer here?

Yes, exactly. This is what the existing documentation means when it says "the value of debug_assertions at monomorphization time".

I have also extended the intrinsic's docs to make this more clear.

RalfJung · 2024-03-17T10:26:32Z

I should probably ensure that this does not regress perf, since these checks appear in hot code.

@bors try @rust-timer queue

… r=<try> refactor check_{lang,library}_ub: use a single intrinsics This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

RalfJung · 2024-03-17T10:34:43Z

Argh, okay builds with debug assertions should finally work now.

@bors try @rust-timer queue

… r=<try> refactor check_{lang,library}_ub: use a single intrinsics This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

bors · 2024-03-17T10:35:52Z

⌛ Trying commit 69e8455 with merge f00755e...

bors · 2024-03-17T10:52:10Z

💔 Test failed - checks-actions

RalfJung · 2024-03-23T09:04:18Z

library/core/src/ub_checks.rs

+/// The intention is to not do that when running in the interpreter, as that one has its own
+/// language UB checks which generally produce better errors.
+#[rustc_const_unstable(feature = "const_ub_checks", issue = "none")]
+#[inline]


FWIW this is the inline that was missing.

bors · 2024-03-23T16:41:59Z

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

These macros and functions are not intrinsics, after all.

…into library

RalfJung · 2024-03-23T17:47:12Z

@bors r=saethlin

bors · 2024-03-23T17:47:14Z

📌 Commit 6177530 has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-03-23T21:11:03Z

⌛ Testing commit 6177530 with merge 2f090c3...

workingjubilee · 2024-03-23T22:55:54Z

@workingjubilee that is quite poetic but equally mysterious. :) Why should this have p=1?

I simply bumped prio for all rollup=never PRs in the queue at the time so that another PR that was approved but was seen as possibly-not-worth-prioritizing-over-current-nevers could be correctly marked iffy and thus correctly skipped in rollups.

bors · 2024-03-23T23:10:00Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 2f090c3 to master...

matthiaskrgr · 2024-03-23T23:12:01Z

I was wondering about that, since it seemed to directly go against our decision to NOT prioritize rollup=nevers over rollup=iffy in the queue.

rust-timer · 2024-03-24T00:24:47Z

Finished benchmarking commit (2f090c3): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.2%	[2.3%, 12.1%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-6.4%, -0.1%]	3
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	1.8%	[-6.4%, 12.1%]	6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.4%]	32
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	5
Improvements ✅ (primary)	-0.1%	[-0.8%, -0.0%]	12
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.8%, 0.4%]	44

Bootstrap: 668.358s -> 669.644s (0.19%)
Artifact size: 315.00 MiB -> 314.97 MiB (-0.01%)

workingjubilee · 2024-03-24T00:46:34Z

I was wondering about that, since it seemed to directly go against our decision to NOT prioritize rollup=nevers over rollup=iffy in the queue.

Yes, I have some thoughts about that but it seemed fine to just adjust some numbers as a one-off since the effect ran out quickly.

… r=saethlin refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

Eliminate `UbCheck` for non-standard libraries The purpose of this PR is to allow other passes to treat `UbChecks` as constants in MIR for optimization after rust-lang#122629. r? RalfJung

Eliminate `UbChecks` for non-standard libraries The purpose of this PR is to allow other passes to treat `UbChecks` as constants in MIR for optimization after rust-lang#122629. r? RalfJung

… r=saethlin refactor check_{lang,library}_ub: use a single intrinsic This enacts the plan I laid out [here](rust-lang#122282 (comment)): use a single intrinsic, called `ub_checks` (in aniticpation of rust-lang/compiler-team#725), that just exposes the value of `debug_assertions` (consistently implemented in both codegen and the interpreter). Put the language vs library UB logic into the library. This makes it easier to do something like rust-lang#122282 in the future: that just slightly alters the semantics of `ub_checks` (making it more approximating when crates built with different flags are mixed), but it no longer affects whether these checks can happen in Miri or compile-time. The first commit just moves things around; I don't think these macros and functions belong into `intrinsics.rs` as they are not intrinsics. r? `@saethlin`

rustbot assigned saethlin Mar 17, 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 17, 2024

RalfJung marked this pull request as ready for review March 17, 2024 09:33

RalfJung mentioned this pull request Mar 17, 2024

Eliminate UbCheck for non-standard libraries #122282

Closed