Skip to content

Respect stacked borrows when polling#11

Merged
eholk merged 2 commits into
microsoft:mainfrom
eholk:issue-9
Aug 12, 2022
Merged

Respect stacked borrows when polling#11
eholk merged 2 commits into
microsoft:mainfrom
eholk:issue-9

Conversation

@eholk

@eholk eholk commented Aug 12, 2022

Copy link
Copy Markdown
Contributor

Polling the wrapped future previously had several reborrow operations, such as in the use of map_unchecked_mut and in the implementation of as_mut_ptr. This caused issues when running under miri in the stress_drop_sender test.

This change re-implements these problematic cases to avoid reborrows, either through using Pin::get_unchecked_mut or transmuting pointers rather than borrowing and casting.

Fixes #9

eholk added 2 commits August 12, 2022 12:19
Polling the wrapped future previously had several reborrow operations,
such as in the use of `map_unchecked_mut` and in the implementation of
`as_mut_ptr`. This caused issues when running under miri in the
`stress_drop_sender` test.

This change re-implements these problematic cases to avoid reborrows,
either through using `Pin::get_unchecked_mut` or transmuting pointers
rather than borrowing and casting.

Fixes microsoft#9
@eholk eholk requested review from daprilik and wesleywiser August 12, 2022 22:33
@daprilik

Copy link
Copy Markdown
Contributor

As much as I'd love to rubber-stamp this PR, I'm not all too familiar with stacked borrow rules myself, so i'll leave it to Wesley to sign off here.

@yoshuawuyts

yoshuawuyts commented Aug 12, 2022

Copy link
Copy Markdown
Member

I see Miri is enabled on CI, but the Miri docs mention stacked borrows is still experimental — does Miri in CI here validate against the stacked borrows rules, or does that need to be enabled somehow?

@eholk

eholk commented Aug 12, 2022

Copy link
Copy Markdown
Contributor Author

I've just been running Miri in the default settings and was able to repro the bug locally. We're using the defaults in CI too, so we should be good there too.

@yoshuawuyts

Copy link
Copy Markdown
Member

Gotcha! Yay, great!

@wesleywiser wesleywiser left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me!

@eholk eholk merged commit 798df7b into microsoft:main Aug 12, 2022
@eholk eholk deleted the issue-9 branch August 12, 2022 23:52
@wesleywiser

Copy link
Copy Markdown

I see Miri is enabled on CI, but the Miri docs mention stacked borrows is still experimental — does Miri in CI here validate against the stacked borrows rules, or does that need to be enabled somehow?

Stacked borrows is the model used to detect aliasing violations so it's enabled by default. There's actually an option to disable it -Zmiri-disable-stacked-borrows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.map_unchecked_mut() is unsound for self-referential types

4 participants