-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Promoteds can contain raw pointers, but these must still only point to immutable allocations #67603
Conversation
☔ The latest upstream changes (presumably #67327) made this pull request unmergeable. Please resolve the merge conflicts. |
Friendly ping: what’s next for this PR? Can I help? |
I just haven't gotten to it. Probably Tuesday or some other day next week |
cc9326c
to
1af201c
Compare
I addressed all review comments and marked the issue as a stable to nightly regression |
InternKind::Promoted => { | ||
alloc.mutability = Mutability::Not; | ||
} | ||
InternKind::Constant | InternKind::ConstProp => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was thinking about this branch here when asking about nested allocations in constants. Isn't this unreachable then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is reachable, but I'm entirely unclean on why it is reachable
const RAW_TRAIT_OBJ_CONTENT_INVALID: *const dyn Trait = &unsafe { BoolTransmute { val: 3 }.bl } as *const _;
hits the InternKind::Constant
arm.
See the MIR on https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3bc6c8b165b614bbe6cf5566d30752de there's no StorageDead
for the allocation containing the bool
with bit pattern 3
.
I'm not really sure what's going on there. Technically, since we're inside a constant, shouldn't this trigger promotion and promote the transmuted value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under unleash it's also reached in
const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;
but that triggers the delay_span_bug as expected.
Also under unleash, but without an ICE (just triggering the dynamic checks in interp):
const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;
//~^ WARN: skipping const checks
const MUTATING_BEHIND_RAW: () = {
// Test that `MUTABLE_BEHIND_RAW` is actually immutable, by doing this at const time.
unsafe {
*MUTABLE_BEHIND_RAW = 99 //~ ERROR any use of this value will cause an error
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what's going on there. Technically, since we're inside a constant, shouldn't this trigger promotion and promote the transmuted value?
Ah, this must be the "not-promotion" also described in this document (Ctrl-F "looks like"). That makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I don't understand though is why RAW_TRAIT_OBJ_CONTENT_INVALID
doesn't trigger the ICE. Why is the allocation already immutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The PR doesn't change anything here so this doesn't block landing, but I'd really like to understand this and then see it documented.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would find documentation here immensely helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a holistic view of what happens, it would really help to write that down.
I don't, which is why my answers are so confusing. They are just a brain dump of me discovering what is going on, not me understanding it entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These issues are preexisting, so after this PR is merged I'll open a new one to write docs and actually figure out what is going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't, which is why my answers are so confusing. They are just a brain dump of me discovering what is going on, not me understanding it entirely
Fair enough. :)
☔ The latest upstream changes (presumably #68078) made this pull request unmergeable. Please resolve the merge conflicts. |
1af201c
to
5053f1b
Compare
☔ The latest upstream changes (presumably #67000) made this pull request unmergeable. Please resolve the merge conflicts. |
r=me with the |
…o immutable allocations
Co-Authored-By: Ralf Jung <post@ralfj.de>
e1479dd
to
69ffe7b
Compare
@bors r=RalfJung |
📌 Commit 69ffe7b has been approved by |
@bors p=1 (regression fix, and this has been sitting for a while) |
Promoteds can contain raw pointers, but these must still only point to immutable allocations fixes #67601 r? @RalfJung cc @wesleywiser in order to not change behaviour in this PR, const prop uses the constant rules for interning, but at least there's an explicit mode for it now that we can think about this in the future
☀️ Test successful - checks-azure |
Tested on commit rust-lang/rust@faf45c5. Direct link to PR: <rust-lang/rust#67603> 💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
Confirmed that this fixes #67601 for Servo. Thanks! |
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15) This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15) This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15) This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15) This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15) This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
fixes #67601
r? @RalfJung
cc @wesleywiser in order to not change behaviour in this PR, const prop uses the constant rules for interning, but at least there's an explicit mode for it now that we can think about this in the future