Miri: unsized locals and by-value dyn traits #59780
Merged
Conversation
This file contains hidden or 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
rust-highfive
added
the
S-waiting-on-review
This was referenced Apr 7, 2019
bjorn3
reviewed
Apr 8, 2019
oli-obk
reviewed
Apr 8, 2019
oli-obk
approved these changes
Apr 8, 2019
typos Co-Authored-By: RalfJung <post@ralfj.de>
|
@bors r+ |
|
📌 Commit 4d79d39 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Apr 8, 2019
Miri: unsized locals and by-value dyn traits r? @oli-obk Cc @eddyb Fixes rust-lang/miri#449
|
@bors p=1 needed to make Miri work again Miri's toolstate is green but that is a bug. |
bors
added a commit
that referenced
this pull request
Apr 11, 2019
Miri: unsized locals and by-value dyn traits r? @oli-obk Cc @eddyb Fixes rust-lang/miri#449
eddyb
reviewed
Apr 11, 2019
| // We have to implement all "object safe receivers". Currently we | ||
| // support built-in pointers (&, &mut, Box) as well as unsized-self. We do | ||
| // not yet support custom self types. | ||
| // Also see librustc_codegen_llvm/abi.rs and librustc_codegen_llvm/mir/block.rs. |
|
☀️ Test successful - checks-travis, status-appveyor |
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
Apr 11, 2019
Tested on commit rust-lang/rust@3de0106. Direct link to PR: <rust-lang/rust#59780> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 6, 2022
…i-obk interpret: remove support for unsized_locals I added support for unsized_locals in rust-lang#59780 but the current implementation is a crude hack and IMO definitely not the right way to have unsized locals in MIR. It also [causes problems](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Missing.20Layout.20Check.20in.20.60interpret.2Foperand.2Ers.60.3F). and what codegen does is unsound and has been for years since clearly nobody cares (so I hope nobody actually relies on that implementation and I'll be happy if Miri ensures they do not). I think if we want to have unsized locals in Miri/MIR we should add them properly, either by having a `StorageLive` that takes metadata or by having an `alloca` that returns a pointer (making the ptr indirection explicit) or something like that. So, this PR removes the `LocalValue::Unallocated` hack. It adds `Immediate::Uninit`, for several reasons: - This lets us still do fairly little work in `push_stack_frame`, in particular we do not actually have to create any allocations. - If/when I remove `ScalarMaybeUninit`, we will need something like this to have an "optimized" representation of uninitialized locals. Without this we'd have to put uninitialized integers into the heap! - const-prop needs some way to indicate "I don't know the value of this local'; it used to use `LocalValue::Unallocated` for that, now it can use `Immediate::Uninit`. There is still a fundamental difference between `LocalValue::Unallocated` and `Immediate::Uninit`: the latter is considered a regular local that you can read from and write to, it just has a more optimized representation when compared with an actual `Allocation` that is fully uninit. In contrast, `LocalValue::Unallocated` had this really odd behavior where you would write to it but not read from it. (This is in fact what caused the problems mentioned above.) While at it I also did two drive-by cleanups/improvements: - In `pop_stack_frame`, do the return value copying and local deallocation while the frame is still on the stack. This leads to better error locations being reported. The old errors were [sometimes rather confusing](https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Cron.20Job.20Failure.202022-06-24/near/287445522). - Deduplicate `copy_op` and `copy_op_transmute`. r? `@oli-obk`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
r? @oli-obk
Cc @eddyb
Fixes rust-lang/miri#449