Delay interning errors to after validation #122684

oli-obk · 2024-03-18T12:35:01Z

This improves diagnostics since validation errors are usually more helpful compared with interning errors that just make broad statements about the entire constant

r? @RalfJung

rustbot · 2024-03-18T12:35:11Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung

Thanks for trying this! IMO it looks promising.

it's not much simpler than the other PR, because now we need to handle nested statics generated by interning.

It seems a lot simpler to me, now we can entirely avoid that extra "patch mutability" stage?

I'm still not sure whether validation should truly also be responsible for handling pointers that can't be "seen" by the type system; this needs a full recursive traversal after all -- a traversal that interning already performs. It's also conceptually no longer about "validating a value".

The alternative would be for interning to return a Result<(), DelayedInternError> and for the queries to do something like

let intern_res = intern();
validate()?;
if let Err(intern_err) = intern_res {
  intern_err.emit();
}

DelayedInternError could have a panicking drop to make sure we catch cases where emit is forgotten.

We can still have validation opportunistically check for dangling pointers to get nice errors (in raw pointers, and maybe inside unions by just scanning the provenance map) -- but we'd not have to do a full second recursive traversal of everything reachable by such hidden pointers.

compiler/rustc_const_eval/src/interpret/validity.rs

tests/ui/consts/const-mut-refs/mut_ref_in_final_dynamic_check.stderr

tests/ui/consts/miri_unleashed/mutable_references.rs

RalfJung · 2024-03-19T13:13:29Z

tests/ui/consts/miri_unleashed/static-no-inner-mut.32bit.stderr

-
-error: encountered mutable pointer in final value of static
-  --> $DIR/static-no-inner-mut.rs:13:1
+   | ^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .<deref>.v: encountered `UnsafeCell` in read-only memory


The obvious reaction to this will be "so why is that memory read-only"...

tests/ui/consts/std/cell.stderr

compiler/rustc_const_eval/src/interpret/validity.rs

tests/ui/consts/miri_unleashed/mutable_references.rs

compiler/rustc_const_eval/src/interpret/intern.rs

compiler/rustc_const_eval/src/interpret/validity.rs

oli-obk · 2024-03-19T17:23:33Z

I removed all the untyped validation and will create a separate PR for it

RalfJung · 2024-03-19T17:48:18Z

I removed all the untyped validation and will create a separate PR for it

Ah, good idea, thanks.

compiler/rustc_const_eval/src/const_eval/valtrees.rs

compiler/rustc_const_eval/src/interpret/intern.rs

compiler/rustc_const_eval/src/interpret/validity.rs

RalfJung · 2024-03-22T13:29:13Z

compiler/rustc_const_eval/src/interpret/validity.rs

+                    .type_of(did)
+                    .no_bound_vars()
+                    .expect("statics should not have generic parameters")
+                    .is_freeze(*self.ecx.tcx, ty::ParamEnv::reveal_all()) =>


I know this is pre-existing, but... why is reveal_all okay here? Shouldn't this be ecx.param_env()?

We're asking for information about a static item's type. If anything we should get the param env of the static (but we know that is just ParamEnv::reveal_all() anyway.

compiler/rustc_const_eval/src/interpret/validity.rs

tests/ui/consts/miri_unleashed/mutable_references.rs

bors · 2024-03-26T02:52:22Z

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

RalfJung · 2024-03-26T06:44:50Z

@rustbot author

oli-obk · 2024-03-26T09:47:52Z

The new state is effectively a full rewrite. So I rebased, split it up into two commits and force pushed.

compiler/rustc_const_eval/src/interpret/intern.rs

compiler/rustc_const_eval/src/interpret/validity.rs

RalfJung · 2024-04-01T09:11:16Z

@rustbot author

oli-obk · 2024-04-02T15:17:12Z

@rustbot ready

bors · 2024-04-03T00:06:18Z

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

compiler/rustc_const_eval/src/interpret/validity.rs

RalfJung · 2024-04-07T08:30:55Z

@rustbot author
(sorry for the slow reviews, I mostly only have time on week-ends for stuff like this and it's been extra busy the last few weeks... though that does seem to become the new normal...)

oli-obk · 2024-04-08T10:58:20Z

No worries. I got enough stuff to do 😆

compiler/rustc_const_eval/src/interpret/validity.rs

RalfJung · 2024-04-16T17:06:43Z

All my comments are resolved, r=me with or without some commit squashing.

…_memory`

validation produces much higher quality errors and already handles most of the cases

oli-obk · 2024-04-17T09:51:57Z

@bors r=RalfJung

bors · 2024-04-17T09:51:59Z

📌 Commit 126dcc6 has been approved by RalfJung

It is now in the queue for this repository.

…after_validaiton, r=RalfJung Delay interning errors to after validation fixes rust-lang#122398 fixes rust-lang#122548 This improves diagnostics since validation errors are usually more helpful compared with interning errors that just make broad statements about the entire constant r? `@RalfJung`

bors · 2024-04-18T02:34:07Z

⌛ Testing commit 126dcc6 with merge 5260893...

bors · 2024-04-18T04:34:22Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 5260893 to master...

rust-timer · 2024-04-18T06:52:14Z

Finished benchmarking commit (5260893): 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
Regressions ❌ (secondary)	0.9%	[0.2%, 4.2%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

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
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 676.169s -> 679.408s (0.48%)
Artifact size: 316.07 MiB -> 315.39 MiB (-0.21%)

rustbot assigned RalfJung Mar 18, 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 Mar 18, 2024

oli-obk mentioned this pull request Mar 18, 2024

Validate before interning #122432

Closed

RalfJung reviewed Mar 19, 2024

View reviewed changes

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch from c7b86c3 to 8f21f22 Compare March 19, 2024 17:20

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch 2 times, most recently from 592f674 to 7517beb Compare March 19, 2024 17:41

RalfJung reviewed Mar 22, 2024

View reviewed changes

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 Mar 26, 2024

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch from 7517beb to 6290e5f Compare March 26, 2024 09:47

oli-obk 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 Mar 26, 2024

RalfJung reviewed Apr 1, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/intern.rs Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/validity.rs Outdated Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/validity.rs Outdated 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 1, 2024

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 2, 2024

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch from 35bea5a to 2d936b4 Compare April 4, 2024 07:37

RalfJung reviewed Apr 7, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/validity.rs Outdated 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 7, 2024

RalfJung reviewed Apr 16, 2024

View reviewed changes

compiler/rustc_const_eval/src/interpret/validity.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch from 586145a to acb817a Compare April 16, 2024 15:15

This comment has been minimized.

Sign in to view

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch from acb817a to a3d987f Compare April 16, 2024 16:19

oli-obk added 6 commits April 17, 2024 09:50

Validate nested static items

8c9cba2

Run the "is this static mutable" logic the same way as in `in_mutable…

d87e963

…_memory`

Deduplicate logic for checking the mutability of allocations

140c9e1

Simplify alloc id mutability check

8b2a4f8

Validate before reporting interning errors.

77fe9f0

validation produces much higher quality errors and already handles most of the cases

Use less fragile error handling

126dcc6

oli-obk force-pushed the delay_interning_errors_to_after_validaiton branch from a3d987f to 126dcc6 Compare April 17, 2024 09:51

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 17, 2024

GuillaumeGomez mentioned this pull request Apr 17, 2024

Rollup of 6 pull requests #124063

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 18, 2024

bors merged commit 5260893 into rust-lang:master Apr 18, 2024
13 checks passed

rustbot added this to the 1.79.0 milestone Apr 18, 2024

cushionbadak mentioned this pull request Apr 19, 2024

ICE: assertion left == right failed at rustc_const_eval/src/interpret/validity.rs #124164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay interning errors to after validation #122684

Delay interning errors to after validation #122684

oli-obk commented Mar 18, 2024 •

edited

rustbot commented Mar 18, 2024

RalfJung left a comment

RalfJung Mar 19, 2024

oli-obk commented Mar 19, 2024

RalfJung commented Mar 19, 2024

RalfJung Mar 22, 2024

oli-obk Mar 26, 2024

bors commented Mar 26, 2024

RalfJung commented Mar 26, 2024

oli-obk commented Mar 26, 2024

RalfJung commented Apr 1, 2024

oli-obk commented Apr 2, 2024

bors commented Apr 3, 2024

RalfJung commented Apr 7, 2024

oli-obk commented Apr 8, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Apr 16, 2024

oli-obk commented Apr 17, 2024

bors commented Apr 17, 2024

bors commented Apr 18, 2024

bors commented Apr 18, 2024

rust-timer commented Apr 18, 2024

Delay interning errors to after validation #122684

Delay interning errors to after validation #122684

Conversation

oli-obk commented Mar 18, 2024 • edited

rustbot commented Mar 18, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Mar 19, 2024

Choose a reason for hiding this comment

oli-obk commented Mar 19, 2024

RalfJung commented Mar 19, 2024

RalfJung Mar 22, 2024

Choose a reason for hiding this comment

oli-obk Mar 26, 2024

Choose a reason for hiding this comment

bors commented Mar 26, 2024

RalfJung commented Mar 26, 2024

oli-obk commented Mar 26, 2024

RalfJung commented Apr 1, 2024

oli-obk commented Apr 2, 2024

bors commented Apr 3, 2024

RalfJung commented Apr 7, 2024

oli-obk commented Apr 8, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Apr 16, 2024

oli-obk commented Apr 17, 2024

bors commented Apr 17, 2024

bors commented Apr 18, 2024

bors commented Apr 18, 2024

rust-timer commented Apr 18, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

oli-obk commented Mar 18, 2024 •

edited