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

Stacked borrow implementation of miri may mistreat statics since they aren't places anymore #1122

Closed
oli-obk opened this issue Dec 18, 2019 · 11 comments
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) C-proposal Category: a proposal for something we might want to do, or maybe not; details still being worked out

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Dec 18, 2019

cc @spastorino @RalfJung

The way we represent statics and promoteds in MIR changed from them being an explicit place to them just being a constant that is a reference to the static (rust-lang/rust#67000). Since this was supported before anyway, we remove the special case of statics being referencable both via a Place and via a constant.

This change may have broken how stacked borrows process references to statics. We should run miri on a few examples and see whether the borrows assigned change in a significant way.

related discussion on zulip: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/promoteds.20are.20not.20places

@RalfJung
Copy link
Member

RalfJung commented Dec 22, 2019

Looks like Miri's test suite still works fine...

I think what'll happen is that the embedded const pointers end up with their tag determined by static_base_ptr, which is exactly what we want: they count as "root pointers", and those (for statics) have SharedReadWrite permission.

@RalfJung
Copy link
Member

Wait, rust-lang/rust#67000 didn't land yet, so what is the current status?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 22, 2019

rust-lang/rust#67000 is for promoteds. statics have already landed

@RalfJung
Copy link
Member

Oh I see. Do you have a PR reference for that?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 22, 2019

rust-lang/rust#66587

@RalfJung
Copy link
Member

How can that have landed without touching interpret at all...? There was a case somewhere there for Static places, right?

Oh I guess that still exists for promoteds?

@RalfJung
Copy link
Member

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 22, 2019

Yea all of these are dead code to be removed by rust-lang/rust#67000

@RalfJung
Copy link
Member

Well let's get back to this issue then once rust-lang/rust#67000 lands. Clearly right now everything is a mess.^^

@RalfJung
Copy link
Member

Also I think it might be better to move this to the Miri repo. This is specifically about the Stacked Borrows implementation, which we track there. So I'll move it if I can.

@RalfJung RalfJung transferred this issue from rust-lang/rust Dec 22, 2019
@RalfJung RalfJung added A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) C-proposal Category: a proposal for something we might want to do, or maybe not; details still being worked out labels Dec 22, 2019
@RalfJung
Copy link
Member

rust-lang/rust#67000 has landed, and from all I can see Stacked Borrows for globals behaves exactly as it should. The miri engine uses tag_global_base_ptr to determine the tag when directly accessing a global; if there were untagged accesses to such globals we would notice (as in rust-lang/rust#70479).

So, closing this as everything seems just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) C-proposal Category: a proposal for something we might want to do, or maybe not; details still being worked out
Projects
None yet
Development

No branches or pull requests

2 participants