Skip to content

Add tests for moving data across await point #2784

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

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

bryangarza
Copy link
Contributor

This patch adds a few tests to assert the current behavior when passing data across an await point. This will help to test out an upcoming fix for the issue of arguments in async functions growing in size because of the generator upvar that is generated when we desugar the async function.

See rust-lang/rust#62958

Also relates to rust-lang/rust#107500

FYI @oli-obk @pnkfelix

@RalfJung
Copy link
Member

RalfJung commented Feb 11, 2023

Thanks! These are interesting tests.

However, I don't think we are guaranteeing that this works. Specifically, the backing store for x could be deallocated earlier. Would be good to at least have comments explicitly explaining that.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2023

However, I don't think we are guaranteeing that this works.

In fact, these tests are only for checking the current behaviour so that when we inevitably (as re-using these memory slots is being worked on) break these tests, that does not happen silently. We really want to break these tests, as they can cause immense wastes of space if enough async function calls are nested.

@RalfJung
Copy link
Member

If we want to break them we have to first make them UB...

But yeah that's what I thought. So there should be a comment explaining this.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2023

If we want to break them we have to first make them UB...

well... the equivalent non-async code is UB already.

But yeah that's what I thought. So there should be a comment explaining this.

agreed

@RalfJung
Copy link
Member

well... the equivalent non-async code is UB already.

That doesn't seem true? This PR adds a non-async version as well and it is a 'pass' test.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 16, 2023

To be honest, the kinds of corner cases that I think I'm worried about for rust-lang/rust#62958 are more things that can only be expressed either in MIR directly or via things like:

fn manual_async_fn(param: MaybeBig) -> impl Future {
    async {
        let local = param;
        let borrow_the_upvar = &param as *const _;
        // uh oh, it is probably a bad idea to have `local` occupy same storage as`param` *now*
    }
}

I do not think you'll be able to express cases like the above via a normal async fn, but they are the cases I am worried about mishandling in my analysis and transformation.

(Furthermore, I do not think the generator upvar optimizations I am writing will actually attempt to be so aggressive as to do the kind of coalescing that is described in the test written in this Pull Request. But then again, I haven't tried my prototype out on the example yet, so maybe I will end up surprising myself.)

This patch adds a few tests to assert the current behavior when passing
data across an await point. This will help to test out an upcoming fix
for the issue of arguments in async functions growing in size because of
the generator upvar that is generated when we desugar the async
function.

See rust-lang/rust#62958

Also relates to rust-lang/rust#107500

Co-authored-by: Ralf Jung <[email protected]>
@bryangarza
Copy link
Contributor Author

@pnkfelix

To be honest, the kinds of corner cases that I think I'm worried about for rust-lang/rust#62958 are more things that can only be expressed either in MIR directly or via things like:

fn manual_async_fn(param: MaybeBig) -> impl Future {
    async {
        let local = param;
        let borrow_the_upvar = &param as *const _;
        // uh oh, it is probably a bad idea to have `local` occupy same storage as`param` *now*
    }
}

I do not think you'll be able to express cases like the above via a normal async fn, but they are the cases I am worried about mishandling in my analysis and transformation.

Gotcha -- we can do a separate PR for the kind of tests you described above.

(Furthermore, I do not think the generator upvar optimizations I am writing will actually attempt to be so aggressive as to do the kind of coalescing that is described in the test written in this Pull Request. But then again, I haven't tried my prototype out on the example yet, so maybe I will end up surprising myself.)

FYI, I ran these new miri tests on your branch of rustc, and they pass as you expected 🙂

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2023

📌 Commit dc6be57 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 18, 2023

⌛ Testing commit dc6be57 with merge d14500b...

@bors
Copy link
Contributor

bors commented Feb 18, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing d14500b to master...

@bors bors merged commit d14500b into rust-lang:master Feb 18, 2023
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.

6 participants