-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Relax Allocator
bounds into pin-safe trait
#94114
Conversation
r? @m-ou-se (rust-highfive has picked a reviewer for you, use r? to override) |
r? @yaahc Relevant discussion:
@TimDiekmann and @CAD97 for wg-allocators visibility. |
This comment has been minimized.
This comment has been minimized.
875742b
to
95d44e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Box::leak
also needs to be bound by A: PinSafeAllocator
. Otherwise, this matches what I was looking for with extracting the pin safe guarantee into its own extension trait.
library/core/src/alloc/mod.rs
Outdated
@@ -87,7 +87,7 @@ impl fmt::Display for AllocError { | |||
/// # Safety | |||
/// | |||
/// * Memory blocks returned from an allocator must point to valid memory and retain their validity | |||
/// until the instance and all of its clones are dropped, | |||
/// until the instance and all of its clones become unreachable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is, of course, the hard part of this PR.
I'm thinking of the guarantee in a similar way to that of references, in that it's a "while the handle exists." Of course, under the Stacked Borrows model (and, to an extent, NLL), it's not when the borrow exists, but until when the borrow is last used.
So there're two ways I currently see to explain the weaker (not pin safe) guarantee, borrowing language from references:
- The memory regions are valid for the lifetime of the allocator. This is complicated by the written
'static
lifetime not actually being "the lifetime of the allocator" in this fashion, but an upper bound, so probably not a good way to explain it. - The memory regions are valid until the point when the last clone is last used.
Effectively, what we're looking for is the dual of the pinning guarantee.
So I think how I'd draft this is
Memory blocks must retain their validity until the allocator instance and all of its clones are dropped, forgotten, leaked, or otherwise never accessed again. (In other words, when a lifetime held by the allocator instance is allowed to expire.)
(And now writing that I'm interested in how the #[may_dangle]
borrowck eyepatch interacts with the guarantee, since that (very roughly) allows a lifetime to expire before the container. IIUC it doesn't, since the eyepatch refers to the lifetime within a generic argument, but I'm never certain w.r.t. may_dangle.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the wording to be more explicit about these conditions. Does that work?
95d44e2
to
32fd054
Compare
This comment has been minimized.
This comment has been minimized.
Looks like changing |
7f367f3
to
7591909
Compare
I pushed through some changes to |
7591909
to
4f0bd6a
Compare
4f0bd6a
to
688c6a5
Compare
☔ The latest upstream changes (presumably #94901) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment was marked as resolved.
This comment was marked as resolved.
cc @rust-lang/wg-allocators |
688c6a5
to
40d905c
Compare
Another possibility would be to change the safety condition for
(This could probably be worded better...) This moves the ball back into I'm hesitant to add more complexity to the |
I think it would be sufficient to say:
As a few examples:
I've been prototyping out what this would look like in a crate and it seems like As an aside, I've also been trying out changing |
Personally I think that extracting the pinning guarantees to a separate trait simplifies the main API w.r.t. pinning rather than complicates it for pinning, but if a simple single-trait solution works, then that's better. Personally, I think I'd word it something along the lines of
(This is sort of circular, as the purpose of this clarification is to make
This sounds sketchy, though, because how do you keep the moral equivalent of the following from blowing up? (Off topic for this thread, feel free to ping me on Zulip) let mut future: impl Future;
'a: {
Pin::new(&'a mut future).poll();
}
// 'a is over, future "isn't" pinned
// caveat emptor: yes it is, per current pin documentation
forget(future);
// reuse future's backing memory
// if the async runtime stored a pointer to the future, 💣 |
☔ The latest upstream changes (presumably #95678) made this pull request unmergeable. Please resolve the merge conflicts. |
r? rust-lang/libs-api |
Ping from triage: |
Because `Allocator` requires that returned memory blocks remain valid until the instance and all clones are dropped, some types of allocators cannot fulfill the safety requirements for the trait. This change relaxes the safety requirements of `Allocator` to blocks remaining valid until the allocator becomes unreachable and moves the bound into a separate `PinSafeAllocator` trait. It also relaxes some overly cautious bounds on the `Unpin` impls for `Box` since unpinning a box doesn't require any special consideration.
1dc151e
to
5cae951
Compare
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
5cae951
to
f14ac69
Compare
This comment has been minimized.
This comment has been minimized.
The `BTreeMap` implementation was updated with support for custom allocators, but many of the methods that take allocators do not require that the provided allocator is the same allocator that is used for the owning `BTreeMap`. This could lead to situations where nodes are created with a different allocator, which would violate the safety conditions of `deallocate` on drop. This was discovered because the changes to `Box::leak` make it invalid to call without upgrading the allocator to `A: PinSafeAllocator`. This also updates `Box::pin_in` with the new `PinSafeAllocator` trait.
f14ac69
to
483c8a3
Compare
@JohnCSimon PR has been fixed up. |
☔ The latest upstream changes (presumably #100982) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author |
Closing this as inactive |
Because
Allocator
requires that returned memory blocks remain validuntil the instance and all clones are dropped, some types of allocators
cannot fulfill the safety requirements for the trait. This change
relaxes the safety requirements of
Allocator
to blocks remaining validuntil the allocator becomes unreachable and moves the bound into a
separate
PinSafeAllocator
trait.It also relaxes some overly cautious bounds on the
Unpin
impls forBox
since unpinning a box doesn't require any special consideration.