Skip to content
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

Merged
merged 8 commits into from
Jan 15, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 25, 2019

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2019
@bors
Copy link
Contributor

bors commented Dec 26, 2019

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

@RalfJung RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2020
@SimonSapin
Copy link
Contributor

Friendly ping: what’s next for this PR? Can I help?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 3, 2020

I just haven't gotten to it. Probably Tuesday or some other day next week

@oli-obk oli-obk force-pushed the no_mut_static_ref_from_const branch from cc9326c to 1af201c Compare January 8, 2020 09:17
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 8, 2020

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 => {
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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
    }
};

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.)

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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. :)

@bors
Copy link
Contributor

bors commented Jan 10, 2020

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

@oli-obk oli-obk force-pushed the no_mut_static_ref_from_const branch from 1af201c to 5053f1b Compare January 10, 2020 08:12
@bors
Copy link
Contributor

bors commented Jan 11, 2020

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

@RalfJung
Copy link
Member

r=me with the if let Some(_) = ecx.tcx.alloc_map.lock().get(alloc_id) branch removed.

@oli-obk oli-obk force-pushed the no_mut_static_ref_from_const branch from e1479dd to 69ffe7b Compare January 15, 2020 10:50
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 15, 2020

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Jan 15, 2020

📌 Commit 69ffe7b has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2020
@RalfJung
Copy link
Member

@bors p=1 (regression fix, and this has been sitting for a while)

@bors
Copy link
Contributor

bors commented Jan 15, 2020

⌛ Testing commit 69ffe7b with merge faf45c5...

bors added a commit that referenced this pull request Jan 15, 2020
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
@bors
Copy link
Contributor

bors commented Jan 15, 2020

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing faf45c5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 15, 2020
@bors bors merged commit 69ffe7b into rust-lang:master Jan 15, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #67603!

Tested on commit faf45c5.
Direct link to PR: #67603

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 15, 2020
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).
@SimonSapin
Copy link
Contributor

Confirmed that this fixes #67601 for Servo. Thanks!

bors-servo pushed a commit to servo/servo that referenced this pull request Jan 16, 2020
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15)

This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
bors-servo pushed a commit to servo/servo that referenced this pull request Jan 16, 2020
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15)

This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
bors-servo pushed a commit to servo/servo that referenced this pull request Jan 16, 2020
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15)

This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
bors-servo pushed a commit to servo/servo that referenced this pull request Jan 16, 2020
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15)

This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
bors-servo pushed a commit to servo/servo that referenced this pull request Jan 16, 2020
Upgrade to rustc 1.42.0-nightly (3291ae339 2020-01-15)

This was unblocked by rust-lang/rust#67603 fixing rust-lang/rust#67601.
@oli-obk oli-obk deleted the no_mut_static_ref_from_const branch March 16, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in Servo: "internal compiler error: mutable allocation in constant"
7 participants