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

Should closures implicitly be wrapped in MaybeDangling? #511

Open
RalfJung opened this issue May 20, 2024 · 4 comments
Open

Should closures implicitly be wrapped in MaybeDangling? #511

RalfJung opened this issue May 20, 2024 · 4 comments

Comments

@RalfJung
Copy link
Member

Closures can have fields that are reference-typed, and then when closures are passed around by-value, those fields behave like struct fields for the aliasing model, so they get retagged and protected. This can lead to subtle and surprising UB. For instance:

Given that the user never sees the reference-typed field here, this seems like quite the footgun. So maybe we should declare that closures behave like MaybeDangling, and not recurse into them when retagging? That would also mean we have to stop emitting noalias dereferenceable for references passed in closure fields.

@CAD97
Copy link

CAD97 commented May 20, 2024

My immediate gut reaction was that it should be fine to retag — the closure captures a lifetime and code should be aware of that when moving the closure object around — but these examples are pretty illustrative; it isn't really about the retag but about the protector.

It's also a bit different if the reference comes from by-ref capture of a local place (this should as much as possible behave as if directly using the local imo) or if it's by-move capture of a ref retagging type (which is moreso what my gut reaction was considering).

The example with fetch_update is a bit like the Arc dealloc case, except instead of just &ControlBlock we have (&ControlBlock, &GuardedData). Only ControlBlock gets the UnsafeCell exception to protectors, and not GuardedData.

Somewhat amusing observation: that example would've been fine if the closure captured last_next, but it actually captured the projected place *last_next instead, IIUC. This is deliberate behavior to allow closure captures to participate in precise partial place borrowing, but resulted in a problematic protector here.

Potentially wild concept — would it be possible to retag but not protect any retagging types captured by closures (or equivalently async)? Although in fairness, IIRC "dereferencable_on_entry" isn't actually all that useful an optimization property.

@RalfJung
Copy link
Member Author

Somewhat amusing observation: that example would've been fine if the closure captured last_next, but it actually captured the projected place *last_next instead, IIUC. This is deliberate behavior to allow closure captures to participate in precise partial place borrowing, but resulted in a problematic protector here.

Good point! I had not realized this.

Potentially wild concept — would it be possible to retag but not protect any retagging types captured by closures (or equivalently async)? Although in fairness, IIRC "dereferencable_on_entry" isn't actually all that useful an optimization property.

Without a protector, we can't even emit noalias, as LLVM considers that to be function-scoped.

@chorman0773
Copy link
Contributor

I actually separately noted this, because I want to turn a macro like boxed!($expr) into (E2021 rules) construct_in_place(ptr, FnOnce::call_once, (|| $expr,)), but then you get a bunch of retags that wouldn't happen in if expr was evaluated directly.

Although in fairness, IIRC "dereferencable_on_entry" isn't actually all that useful an optimization property.

I personally have plans to make use of a dereferenceable that means dereferenceable_on_entry. The optimizations require different tracing, but should still be useful.

@RalfJung
Copy link
Member Author

Here's another case of someone hitting this problem.

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

No branches or pull requests

3 participants