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

handle diverging functions forwarding their return place #66827

Merged
merged 2 commits into from
Dec 2, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 27, 2019

Fixes rust-lang/miri#1075: the shim around diverging closures turned into function pointers actually "obtains" a return place inside a diverging function, but just uses it as the return place for a diverging callee. Handle this by using NULL places.

This is kind of a hack as it breaks our invariant that all places are dereferencable, but we'd eventually let raw pointers break that anyway I assume so that seems fine.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 27, 2019
// (even a ZST read/write) needs to error, so let us make this
// a NULL place.
//
// FIXME: Ideally we'd make sure that the place projections also
Copy link
Contributor

Choose a reason for hiding this comment

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

Like just doing a place projection on a null place should error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Place projections should have inbounds rules, basically. We have an implementation of that in Miri, and these days I am actually not ashamed of it any more. ;)

But I am worried about the perf impact of this and I think it is currently actually impossible to violate the inbounds requirement, so I didn't bother working on moving that into the core engine.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2019

📌 Commit 2869aba has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2019
@bors
Copy link
Contributor

bors commented Nov 30, 2019

⌛ Testing commit 2869aba with merge d703ed834af1ea4b1c38e85e7dc220786f876865...

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 30, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2019
@RalfJung
Copy link
Member Author

Chocolatey was down. @bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2019
@bors
Copy link
Contributor

bors commented Nov 30, 2019

⌛ Testing commit 2869aba with merge 1041db3d91d3f1be462da9124ae0b1b7e16d5a14...

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 30, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2019
@Centril
Copy link
Contributor

Centril commented Nov 30, 2019

@bors retry spurious

@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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 1, 2019
…oli-obk

handle diverging functions forwarding their return place

Fixes rust-lang/miri#1075: the shim around diverging closures turned into function pointers actually "obtains" a return place inside a diverging function, but just uses it as the return place for a diverging callee. Handle this by using NULL places.

This is kind of a hack as it breaks our invariant that all places are dereferencable, but we'd eventually let raw pointers break that anyway I assume so that seems fine.

r? @oli-obk
bors added a commit that referenced this pull request Dec 1, 2019
Rollup of 7 pull requests

Successful merges:

 - #66346 (Replace .unwrap() with ? in std::os::unix::net)
 - #66789 (rustc: move mir::SourceScopeLocalData to a field of SourceScopeData.)
 - #66822 (libunwind_panic: adjust miri panic hack)
 - #66827 (handle diverging functions forwarding their return place)
 - #66828 (Less minification)
 - #66850 (rustc: hide HirId's fmt::Debug output from -Z span_free_formats.)
 - #66907 (rustc: don't just show raw DefIndex's in BrNamed's fmt::Debug impl.)

Failed merges:

 - #66874 (Miri engine: proper support for `Assert` MIR terminators)

r? @ghost
bors added a commit that referenced this pull request Dec 1, 2019
Rollup of 7 pull requests

Successful merges:

 - #66346 (Replace .unwrap() with ? in std::os::unix::net)
 - #66789 (rustc: move mir::SourceScopeLocalData to a field of SourceScopeData.)
 - #66822 (libunwind_panic: adjust miri panic hack)
 - #66827 (handle diverging functions forwarding their return place)
 - #66828 (Less minification)
 - #66850 (rustc: hide HirId's fmt::Debug output from -Z span_free_formats.)
 - #66907 (rustc: don't just show raw DefIndex's in BrNamed's fmt::Debug impl.)

Failed merges:

 - #66874 (Miri engine: proper support for `Assert` MIR terminators)

r? @ghost
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 2, 2019
…oli-obk

handle diverging functions forwarding their return place

Fixes rust-lang/miri#1075: the shim around diverging closures turned into function pointers actually "obtains" a return place inside a diverging function, but just uses it as the return place for a diverging callee. Handle this by using NULL places.

This is kind of a hack as it breaks our invariant that all places are dereferencable, but we'd eventually let raw pointers break that anyway I assume so that seems fine.

r? @oli-obk
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 2, 2019
…oli-obk

handle diverging functions forwarding their return place

Fixes rust-lang/miri#1075: the shim around diverging closures turned into function pointers actually "obtains" a return place inside a diverging function, but just uses it as the return place for a diverging callee. Handle this by using NULL places.

This is kind of a hack as it breaks our invariant that all places are dereferencable, but we'd eventually let raw pointers break that anyway I assume so that seems fine.

r? @oli-obk
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 2, 2019
…oli-obk

handle diverging functions forwarding their return place

Fixes rust-lang/miri#1075: the shim around diverging closures turned into function pointers actually "obtains" a return place inside a diverging function, but just uses it as the return place for a diverging callee. Handle this by using NULL places.

This is kind of a hack as it breaks our invariant that all places are dereferencable, but we'd eventually let raw pointers break that anyway I assume so that seems fine.

r? @oli-obk
bors added a commit that referenced this pull request Dec 2, 2019
Rollup of 5 pull requests

Successful merges:

 - #66245 (Conditional compilation for sanitizers)
 - #66654 (Handle const-checks for `&mut` outside of `HasMutInterior`)
 - #66822 (libunwind_panic: adjust miri panic hack)
 - #66827 (handle diverging functions forwarding their return place)
 - #66834 (rustbuild fixes)

Failed merges:

r? @ghost
@bors bors merged commit 2869aba into rust-lang:master Dec 2, 2019
bors added a commit to rust-lang/miri that referenced this pull request Dec 2, 2019
Test diverging closure coercion

Adds a test for #1075. Depends on rust-lang/rust#66827.
@RalfJung RalfJung deleted the miri-missing-ret-place branch December 2, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Running diverging closure coerced to fn ptr fails: "invalid use of NULL pointer"
5 participants