compile-time evaluation: detect writes through immutable pointers #118324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 The Miri subtree was changed cc @rust-lang/miri Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_gcc cc @antoyo |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 26, 2023
…<try> compile-time evaluation: detect writes through immutable pointers This has two motivations: - it unblocks rust-lang#116745 (and therefore takes a big step towards `const_mut_refs` stabilization), because we can now detect if the memory that we find in `const` can be interned as "immutable" - it would detect the UB that was uncovered in rust-lang#117905, which was caused by accidental stabilization of `copy` functions in `const` that can only be called with UB When UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with [the const-UB RFC](https://rust-lang.github.io/rfcs/3016-const-ub.html), meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without `const_mut_refs` -- the accidentally stabilized `copy` functions are the only way this can happen, so the crates that popped up in rust-lang#117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that uncovered have all been fixed in their respective crates already. I just hope perf works out.
RalfJung
commented
Nov 26, 2023
| /// Returns the `AllocId` of this provenance. | ||
| #[inline(always)] | ||
| pub fn alloc_id(self) -> AllocId { | ||
| AllocId(NonZeroU64::new(self.0.get() & !IMMUTABLE_MASK).unwrap()) |
There was a problem hiding this comment.
We could use new_unchecked here, we have an invariant that the AllocId part of the provenance is always non-null. But let's first see if perf is fine without unsafe code.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
ctfe-read-only-pointers
branch
from
f25c190
to
7a4d651
Compare
rustbot
added
A-testsuite
RalfJung
force-pushed
the
ctfe-read-only-pointers
branch
from
7a4d651
to
37a50ac
Compare
ghost
reviewed
Nov 26, 2023
ghost
reviewed
Nov 26, 2023
compiler/rustc_middle/src/mir/interpret/allocation/provenance_map.rs
Outdated
Show resolved
Hide resolved
RalfJung
commented
Nov 26, 2023
| @@ -8,6 +8,135 @@ use crate::{declare_lint, declare_lint_pass, FutureIncompatibilityReason}; | |||
| use rustc_span::edition::Edition; | |||
| use rustc_span::symbol::sym; | |||
|
|
|||
| declare_lint_pass! { | |||
There was a problem hiding this comment.
I moved this to the top, it was in a random spot in the middle of the file which surely doesn't make a ton of sense.
|
Finished benchmarking commit (b5c17f8): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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 @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.277s -> 673.76s (-0.08%) |
rustbot
added
perf-regression
RalfJung
force-pushed
the
ctfe-read-only-pointers
branch
from
7197e21
to
be1e481
Compare
|
Right, so there is a regression in ctfe-stress. I was worried this might happen. I guess now comes the usual phase of doing random changes and seeing how they affect perf... |
RalfJung
force-pushed
the
ctfe-read-only-pointers
branch
from
be1e481
to
303872f
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
RalfJung
force-pushed
the
ctfe-read-only-pointers
branch
from
4896b97
to
8188bd4
Compare
|
I squashed the fixups but kept the commits that are logical units. Thanks for the review! @bors r=saethlin |
bors
added
S-waiting-on-bors
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Test successful - checks-actions |
This was referenced Dec 7, 2023
|
Finished benchmarking commit (0e7f91b): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.821s -> 675.63s (0.12%) |
celinval
added a commit
to model-checking/kani
that referenced
this pull request
Dec 8, 2023
Updates the Rust toolchain to `nightly-2023-12-08`. The relevant changes are: * rust-lang/rust#118324 * rust-lang/rust#118684 --------- Co-authored-by: Celina G. Val <celinval@amazon.com>
Mark-Simulacrum
added
the
perf-regression-triaged
Agreed. |
bjorn3
pushed a commit
to bjorn3/rust
that referenced
this pull request
Dec 19, 2023
…saethlin compile-time evaluation: detect writes through immutable pointers This has two motivations: - it unblocks rust-lang#116745 (and therefore takes a big step towards `const_mut_refs` stabilization), because we can now detect if the memory that we find in `const` can be interned as "immutable" - it would detect the UB that was uncovered in rust-lang#117905, which was caused by accidental stabilization of `copy` functions in `const` that can only be called with UB When UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with [the const-UB RFC](https://rust-lang.github.io/rfcs/3016-const-ub.html), meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without `const_mut_refs` -- the accidentally stabilized `copy` functions are the only way this can happen, so the crates that popped up in rust-lang#117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that we know about have all been fixed in their respective crates already. The way this is implemented is by making use of the fact that our interpreter is already generic over the notion of provenance. For CTFE we now use the new `CtfeProvenance` type which is conceptually an `AllocId` plus a boolean `immutable` flag (but packed for a more efficient representation). This means we can mark a pointer as immutable when it is created as a shared reference. The flag will be propagated to all pointers derived from this one. We can then check the immutable flag on each write to reject writes through immutable pointers. I just hope perf works out.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 17, 2024
…-obk const-eval interning: get rid of type-driven traversal This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global `tcx` memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for `static` and `const`, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.) We can do this now that rust-lang#118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not. The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for `const` and pomoted and non-interior-mutable `static`; all other allocations are interned as immutable for `static`, `const`, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB: - for promoteds, we rely on the analysis that does promotion to ensure that this is sound. - for `const` and `static`, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via `&<expr>` at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable. Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules. I also extended our type-driven const validity checks to ensure that `&mut T` in the final value of a const points to mutable memory, at least if `T` is not zero-sized. This catches cases of people turning `&i32` into `&mut i32` (which would still be considered a read-only pointer). Similarly, when these checks encounter an `UnsafeCell`, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer. This PR does have the immediate effect of allowing some new code on stable, for instance: ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a `Vec<i32>`), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway. This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.) `@rust-lang/wg-const-eval` I hope you are okay with this plan. :) `@rust-lang/lang` paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 23, 2024
…-obk const-eval interning: get rid of type-driven traversal This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global `tcx` memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for `static` and `const`, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.) We can do this now that rust-lang#118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not. The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for `const` and pomoted and non-interior-mutable `static`; all other allocations are interned as immutable for `static`, `const`, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB: - for promoteds, we rely on the analysis that does promotion to ensure that this is sound. - for `const` and `static`, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via `&<expr>` at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable. Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules. I also extended our type-driven const validity checks to ensure that `&mut T` in the final value of a const points to mutable memory, at least if `T` is not zero-sized. This catches cases of people turning `&i32` into `&mut i32` (which would still be considered a read-only pointer). Similarly, when these checks encounter an `UnsafeCell`, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer. This PR does have the immediate effect of allowing some new code on stable, for instance: ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a `Vec<i32>`), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway. This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.) `@rust-lang/wg-const-eval` I hope you are okay with this plan. :) `@rust-lang/lang` paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Jan 24, 2024
const-eval interning: get rid of type-driven traversal This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global `tcx` memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for `static` and `const`, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.) We can do this now that rust-lang/rust#118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not. The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for `const` and pomoted and non-interior-mutable `static`; all other allocations are interned as immutable for `static`, `const`, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB: - for promoteds, we rely on the analysis that does promotion to ensure that this is sound. - for `const` and `static`, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via `&<expr>` at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable. Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules. I also extended our type-driven const validity checks to ensure that `&mut T` in the final value of a const points to mutable memory, at least if `T` is not zero-sized. This catches cases of people turning `&i32` into `&mut i32` (which would still be considered a read-only pointer). Similarly, when these checks encounter an `UnsafeCell`, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer. This PR does have the immediate effect of allowing some new code on stable, for instance: ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a `Vec<i32>`), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway. This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.) `@rust-lang/wg-const-eval` I hope you are okay with this plan. :) `@rust-lang/lang` paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.
71 tasks
GuillaumeGomez
pushed a commit
to GuillaumeGomez/rust
that referenced
this pull request
Feb 21, 2024
…saethlin compile-time evaluation: detect writes through immutable pointers This has two motivations: - it unblocks rust-lang#116745 (and therefore takes a big step towards `const_mut_refs` stabilization), because we can now detect if the memory that we find in `const` can be interned as "immutable" - it would detect the UB that was uncovered in rust-lang#117905, which was caused by accidental stabilization of `copy` functions in `const` that can only be called with UB When UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with [the const-UB RFC](https://rust-lang.github.io/rfcs/3016-const-ub.html), meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without `const_mut_refs` -- the accidentally stabilized `copy` functions are the only way this can happen, so the crates that popped up in rust-lang#117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that we know about have all been fixed in their respective crates already. The way this is implemented is by making use of the fact that our interpreter is already generic over the notion of provenance. For CTFE we now use the new `CtfeProvenance` type which is conceptually an `AllocId` plus a boolean `immutable` flag (but packed for a more efficient representation). This means we can mark a pointer as immutable when it is created as a shared reference. The flag will be propagated to all pointers derived from this one. We can then check the immutable flag on each write to reject writes through immutable pointers. I just hope perf works out.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This has two motivations:
const_mut_refsstabilization), because we can now detect if the memory that we find inconstcan be interned as "immutable"copyfunctions inconstthat can only be called with UBWhen UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with the const-UB RFC, meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without
const_mut_refs-- the accidentally stabilizedcopyfunctions are the only way this can happen, so the crates that popped up in #117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that we know about have all been fixed in their respective crates already.The way this is implemented is by making use of the fact that our interpreter is already generic over the notion of provenance. For CTFE we now use the new
CtfeProvenancetype which is conceptually anAllocIdplus a booleanimmutableflag (but packed for a more efficient representation). This means we can mark a pointer as immutable when it is created as a shared reference. The flag will be propagated to all pointers derived from this one. We can then check the immutable flag on each write to reject writes through immutable pointers.I just hope perf works out.