Validate before interning #122432

oli-obk · 2024-03-13T12:45:10Z

There are more cleanups that can be done afterwards, but I think they may be unnecessary to make this PR useful on its own

rustbot · 2024-03-13T12:45:20Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

oli-obk · 2024-03-13T12:47:12Z

compiler/rustc_const_eval/src/interpret/validity.rs

+        {
+            for (_, prov) in alloc.provenance().ptrs().iter() {
+                if let AllocKind::Dead = self.get_alloc_info(prov.alloc_id()).2 {
+                    throw_validation_failure!(
+                        path,
+                        DanglingPtrUseAfterFree { ptr_kind: PointerKind::Ref(Mutability::Not) }
+                    )
+                }
+            }
+        }
+        Ok(())


There are more improvements to do here (like check unions) to get better paths, but that's harder

bors · 2024-03-14T02:58:12Z

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

RalfJung · 2024-03-18T07:01:31Z

compiler/rustc_const_eval/src/interpret/intern.rs

+        };
+        // We always intern with `inner_mutability`, and furthermore we ensured above that if
+        // that is "immutable", then there are *no* mutable pointers anywhere in the newly
+        // interned memory -- justifying that we can indeed intern immutably. However this also


Hm, we're not really ensuring this any more, are we?

That check is now done by intern_const_alloc_recursive. And that function has to re-compute inner_mutability just for that purpose.

Maybe patch_mutability_of_allocs should still do the check, but just return whether there was a problem or not, then we run validation, and then if validation didn't error and there was a problem during patch_mutability_of_allocs, then we show the other error? Thinking about it, if we do that we can probably still do interning before validation... 🙈

Yea, there were more follow-up cleanups to do, but I didn't want to change too much in one go

If we follow the strategy of "do interning first, record if there was an error, but delay reporting that until after validation" -- that requires way fewer changes than this PR, doesn't it?

I'll try it out, I should have noted down what I thought I could improve/clean up if we did this

Oh I remember now: we can make interning completely infallible by moving the CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE into validation

Ok, I implemented this in #122684

I think we should still go with this PR, as it allows making individual things simpler, even if it means validation needs to handle allocations that haven't been interned yet. Most of the time that should be resolveable by using general interpreter methods instead of using dedicated global_alloc code paths

Oh I remember now: we can make interning completely infallible by moving the CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE into validation

I don't think that works. Validation is inherently a type-based traversal, so data stored in unions (or padding) is completely ignored. (And that's arguably by design.) However we need to check all pointers to ensure none of them are mutable, including those stored in unions (and padding).

I changed that in this PR. After a place has been validated, we also do some checks on all relocations of that place

Hm, to me that doesn't fit with the intention of validation as a type-driven traversal... I'll have to think about this.

tests/ui/consts/dangling-alloc-id-ice.stderr

RalfJung · 2024-03-18T07:10:24Z

tests/ui/consts/dangling-alloc-id-ice.stderr

+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
+   = note: the raw bytes of the constant (size: 8, align: 8) {
+               ╾ALLOC0<imm>╼                         │ ╾──────╼


You probably want to add the usual HEX_DUMP normalization rules to avoid having to make this stderr file per-bitwidth.

tests/ui/consts/miri_unleashed/mutable_references.stderr

…icit. Before it was hard to see that in the other cases we just preserve the static's mutability

oli-obk · 2024-03-18T12:37:06Z

@bors try @rust-timer queue

bors · 2024-03-18T12:38:16Z

⌛ Trying commit c99636f with merge abaca54...

…=<try> Validate before interning based on rust-lang#122397 fixes rust-lang#122398 r? `@RalfJung` There are more cleanups that can be done afterwards, but I think they may be unnecessary to make this PR useful on its own

bors · 2024-03-18T14:09:11Z

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

oli-obk · 2024-03-18T14:48:54Z

tests/ui/consts/dangling_raw_ptr.rs

+const BAR: Union = { //~ ERROR it is undefined behavior
+    let x = 42;
+    Union { ptr: &x }
+};


Before this PR we had no tests actually excercising this code path

rust-timer · 2024-03-18T15:47:41Z

Finished benchmarking commit (abaca54): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try 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-perf +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)	1.5%	[1.2%, 1.8%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-	-	0

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

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

Binary size

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

Bootstrap: 668.532s -> 668.042s (-0.07%)
Artifact size: 312.74 MiB -> 312.80 MiB (0.02%)

RalfJung · 2024-03-19T13:45:59Z

So, to me the alternative at #122684 seems conceptually simpler. What are the benefits of the approach in this PR?

oli-obk · 2024-03-19T15:53:10Z

What are the benefits of the approach in this PR?

you get a path to where things are wrong, even if they are in padding or unions.

And it does some separation of concerns, giving us simpler code for the mutability changes and the actual interning, instead of doing both at the same time.

RalfJung · 2024-03-19T16:07:20Z

you get a path to where things are wrong, even if they are in padding or unions.

How is that tied to having the separate "patch allocation mutability" pass? It seems to me that the exact same code you wrote here would also achieve this for the other PR.

(Over in that PR I am arguing maybe we want the interner to handle those errors, but that's a separate discussion from whether validation happens before or after interning. My only concern for these errors in the interner is having extra complexity in the validator which doesn't ever do anything visible without miri-unleashed.)

And it does some separation of concerns, giving us simpler code for the mutability changes and the actual interning, instead of doing both at the same time.

I would say it splits what feels like a single job into two. There's even some code duplication between the alloc-mutbl-patch pass and the interner.

oli-obk · 2024-03-19T16:14:48Z

My next step was to get rid of found_bad_mutable_pointer and instead make that check part of validation, so we also get paths for it. At that point interning is just a loop over all allocations to intern them.

I guess we'll have two of those loops then. Yea the benefits are not very large.

I'll move the latest commits from here over to the other PR then

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

oli-obk commented Mar 13, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung reviewed Mar 18, 2024

View reviewed changes

oli-obk added 5 commits March 18, 2024 10:29

Move validation before interning

f658a96

Make mutability taken from Freezeness of the main static alloc expl…

2e446cc

…icit. Before it was hard to see that in the other cases we just preserve the static's mutability

Also check typed raw pointers in validation for dangliness

e05aae8

Add regression test

b1d4cbc

Check all dangling pointers in validation instead of in interning

c99636f

oli-obk force-pushed the validate_before_interning branch from 722aabc to c99636f Compare March 18, 2024 12:33

oli-obk mentioned this pull request Mar 18, 2024

Delay interning errors to after validation #122684

Merged

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 18, 2024

This comment has been minimized.

Sign in to view

oli-obk commented Mar 18, 2024

View reviewed changes

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 18, 2024

oli-obk added 2 commits March 19, 2024 10:32

Make some tests bitsize independent

2d86ed1

Test and implement detecting nested dead allocations

80a003f

oli-obk added 3 commits March 19, 2024 11:51

Show offsets and derefs in padding, too

60329f7

More tests

672b35c

Make some diagnostics behave consistently

d1aee85

oli-obk force-pushed the validate_before_interning branch from 38b7f81 to d1aee85 Compare March 19, 2024 12:24

oli-obk closed this Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate before interning #122432

Validate before interning #122432

oli-obk commented Mar 13, 2024

rustbot commented Mar 13, 2024

oli-obk Mar 13, 2024

This comment has been minimized.

bors commented Mar 14, 2024

RalfJung Mar 18, 2024 •

edited

Loading

RalfJung Mar 18, 2024

oli-obk Mar 18, 2024

RalfJung Mar 18, 2024

oli-obk Mar 18, 2024

oli-obk Mar 18, 2024

oli-obk Mar 18, 2024

RalfJung Mar 18, 2024 •

edited

Loading

oli-obk Mar 18, 2024

RalfJung Mar 18, 2024

RalfJung Mar 18, 2024

oli-obk commented Mar 18, 2024

This comment has been minimized.

bors commented Mar 18, 2024

This comment has been minimized.

bors commented Mar 18, 2024

This comment has been minimized.

oli-obk Mar 18, 2024

rust-timer commented Mar 18, 2024

RalfJung commented Mar 19, 2024

oli-obk commented Mar 19, 2024 •

edited

Loading

RalfJung commented Mar 19, 2024 •

edited

Loading

oli-obk commented Mar 19, 2024

Validate before interning #122432

Validate before interning #122432

Conversation

oli-obk commented Mar 13, 2024

rustbot commented Mar 13, 2024

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Mar 14, 2024

RalfJung Mar 18, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Mar 18, 2024 • edited Loading

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

oli-obk commented Mar 18, 2024

This comment has been minimized.

bors commented Mar 18, 2024

This comment has been minimized.

bors commented Mar 18, 2024

This comment has been minimized.

Choose a reason for hiding this comment

rust-timer commented Mar 18, 2024

Overall result: ❌ regressions - ACTION NEEDED

RalfJung commented Mar 19, 2024

oli-obk commented Mar 19, 2024 • edited Loading

RalfJung commented Mar 19, 2024 • edited Loading

oli-obk commented Mar 19, 2024

RalfJung Mar 18, 2024 •

edited

Loading

RalfJung Mar 18, 2024 •

edited

Loading

oli-obk commented Mar 19, 2024 •

edited

Loading

RalfJung commented Mar 19, 2024 •

edited

Loading