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

Test and assert that we do not reveal a static item's memory AllocId #116571

Closed
wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 9, 2023

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 9, 2023
@RalfJung
Copy link
Member

There's one more thing we could do but I don't know how hard it is... we could make sure to not call set_alloc_id_memory on the "inner, hidden" AllocId of a static. That means we'll get dangling ptr errors if it ever gets used anywhere else, which is better than silently doing the wrong thing.

@@ -212,6 +212,10 @@ pub fn eval_to_const_value_raw_provider<'tcx>(
tcx: TyCtxt<'tcx>,
key: ty::ParamEnvAnd<'tcx, GlobalId<'tcx>>,
) -> ::rustc_middle::mir::interpret::EvalToConstValueResult<'tcx> {
assert!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert!(
// This shouldn't be used for statics, since statics are conceptually places,
// not values -- so what we do here could break pointer identity.
assert!(

@RalfJung
Copy link
Member

This is related to the changes in #116835, right? Though there you said that we cannot yet assert that we don't use the query on statics, and here actually that passes CI, including Miri CI? I am confused.

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 22, 2023

The "eval to const value" path (where I added the assert) invokes the "eval to alloc" code path where we cannot, because that is also used by eval_static_initializer right now

@RalfJung
Copy link
Member

Ah, okay.

So what's your plan with this PR? I'm fine with landing it after adding the comment suggested above, but you also have other PRs touching the same area and changing how we handle alloc IDs around statics.

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 23, 2023

Yea this becomes obsolte if we manage to land the other PRs. I'll make sure to carry over your comment change to the asserts I'm adding elsewhere

@oli-obk oli-obk closed this Oct 23, 2023
@oli-obk oli-obk deleted the test_static_dup branch October 23, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants