Implement MaybeDangling compiler support#150447
Implement MaybeDangling compiler support#150447WaffleLapkin wants to merge 6 commits intorust-lang:mainfrom
MaybeDangling compiler support#150447Conversation
aebfd56 to
63e07cf
Compare
This comment has been minimized.
This comment has been minimized.
63e07cf to
c40ca7d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6d96bdb to
d71787d
Compare
This comment has been minimized.
This comment has been minimized.
f6fafff to
baa9118
Compare
This comment has been minimized.
This comment has been minimized.
baa9118 to
c135ac1
Compare
This comment has been minimized.
This comment has been minimized.
c135ac1 to
f399322
Compare
8bbf80d to
a72a3f3
Compare
This comment has been minimized.
This comment has been minimized.
a72a3f3 to
42ce338
Compare
This comment has been minimized.
This comment has been minimized.
51a5c77 to
efc351d
Compare
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@RalfJung this is finally ready for review :) Let me know if you'd prefer the |
This comment has been minimized.
This comment has been minimized.
| if offset.bytes() == 0 | ||
| && let Some(pointee) = this.ty.boxed_ty() => | ||
| { | ||
| let optimize = tcx.sess.opts.optimize != OptLevel::No; |
There was a problem hiding this comment.
let optimize looks like it could be moved out of the match.
There was a problem hiding this comment.
The let optimize = ... line itself is still there, now shadowing the one you put outside the match.
efc351d to
8102f65
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
This PR can result in stable code using ManuallyDrop getting less optimization, right? |
|
@theemathas yes. Importantly also less UB ^^' |
|
Indeed, that is the intent of this PR. ;) Note that this PR does not guarantee the absence of those optimizations on stable. It only guarantees it for code that uses |
Make `size`/`align` always correct rather than conditionally on the `safe` field. This makes it less error prone and easier to work with for `MaybeDangling` / potential future pointer kinds like `Aligned<_>`.
Instead of defaulting to `None` it now defaults to `Align::ONE` i.e. no alignment restriction. Codegen test changes are due to us now skipping `align 1` annotations (they are useless; not skipping them makes all the raw pointers gain an `align 1` annotation which doesn't seem any good)
395a073 to
62cbdee
Compare
| // Handle safe Rust thin and wide pointers. | ||
| /// Returns argument attributes for a scalar argument. | ||
| /// | ||
| /// `drop_target_pointee` is used to special-case the argument of `ptr::drop_in_place`, |
There was a problem hiding this comment.
| /// `drop_target_pointee` is used to special-case the argument of `ptr::drop_in_place`, | |
| /// `drop_target_pointee`, if set, causes the scalar, if it is a pointer, to be treated as a mutable | |
| /// reference to the given type. This is used to special-case the argument of `ptr::drop_in_place`, |
This is unlike before where that treatment only happened if the scalar was by itself not already a safe pointer -- right?
There was a problem hiding this comment.
if it is a pointer
that is not quite correct, we have an assertion that it's a pointer, not a check.
This is unlike before where that treatment only happened if the scalar was by itself not already a
safepointer -- right?
Yes, before it happened if the scalar was a pointer, and specifically a raw one.
There was a problem hiding this comment.
Okay, please make the docs whatever the correct thing is then. :)
There was a problem hiding this comment.
Please add a test (or extend an existing one) ensuring that MaybeDangling<&T> is not marked with captures or readonly, with a comment pointing this out as a relevant thing to test.
I wonder if it would make sense to make the tests like tests/codegen-llvm/function-arguments.rs where we actually list all the attributes so that we can easily notice when they change? The fact that I didn't realize that we are setting readdonly captures on these references concerns me.
There was a problem hiding this comment.
The reason I made the test like I did is that the differences between local and ci llvm were driving me insane trying to write a test that works on both of them ^^'
I'll look at function-argument.rs
There was a problem hiding this comment.
Are you sure that MaybeDangling<&T> actually need to strip captures and readonly? Those do not imply any validity invariants, and effectively only cause restrictions on any &Ts that get extracted out of the MaybeDangling and how they can be used.
That being said, I do not know what the miri paragraph in the RFC is saying because I don't know off the top of my head what retagging is, so... sorry if that's related to the answer to my question (though, even then I feel like this should be spelled out more explicitly).
There was a problem hiding this comment.
Are you sure that MaybeDangling<&T> actually need to strip captures and readonly?
For the model implemented in #150446, yes. So I'd say let's implement that model for now because it's the one we have and know how to implement; I have added this as an open question in the tracking issue.
Those do not imply any validity invariants
They do. They have to come from somewhere, after all.
| safe: None, | ||
| // Make sure we don't assert dereferenceability of the pointer. | ||
| size: Size::ZERO, | ||
| ..info |
There was a problem hiding this comment.
| ..info | |
| // Preserve the alignment assertion! That is required even inside `MaybeDangling`. | |
| ..info |
There was a problem hiding this comment.
Also, why does this test ManuallyDrop? The actually relevant type is MaybeDangling.
There was a problem hiding this comment.
(it's because that's the currently stable type that users care about)
Will change it though, no real reason to test ManuallyDrop
| /// Indicates whether the type is `FieldRepresentingType`. | ||
| const IS_FIELD_REPRESENTING_TYPE = 1 << 13; | ||
| /// Indicates whether the type is `MaybeDangling<_>`. | ||
| const IS_MAYBE_DANGLING = 1 << 14; |
There was a problem hiding this comment.
Most of these are mutually exclusive so it is a huge waste of bits to have a separate bit for each of them... 🤷 that's pre-existing though, not for this PR to fix.
Tracking issue: #118166
cc @RalfJung