Skip to content

Change {Box,Arc,Rc,Weak}::into_raw to only work with A = Global #141219

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented May 18, 2025

Also applies to Vec::into_raw_parts.

The expectation is that you can round-trip these methods with from_raw, but this is only true when using the global allocator. With custom allocators you should instead be using into_raw_with_allocator and from_raw_in.

The implementation of Box::leak is changed to use Box::into_raw_with_allocator and explicitly leak the allocator (which was already the existing behavior). This is because, for leak to be safe, the allocator must not free its underlying backing store. The Allocator trait only guarantees that allocated memory remains valid until the allocator is dropped.

@rustbot
Copy link
Collaborator

rustbot commented May 18, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 18, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Also applies to `Vec::into_raw_parts`.

The expectation is that you can round-trip these methods with
`from_raw`, but this is only true when using the global allocator. With
custom allocators you should instead be using
`into_raw_with_allocator` and `from_raw_in`.

The implementation of `Box::leak` is changed to use
`Box::into_raw_with_allocator` and explicitly leak the allocator (which
was already the existing behavior). This is because, for `leak` to be
safe, the allocator must not free its underlying backing store. The
`Allocator` trait only guarantees that allocated memory remains valid
until the allocator is dropped.
@Amanieu
Copy link
Member Author

Amanieu commented May 18, 2025

I don't know why this is failing on Miri... I only moved the methods and didn't change any implementation (except Box::leak).

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member Author

Amanieu commented May 19, 2025

@RalfJung Do you have any idea why these miri tests would fail? The only relevant change is moving Box::into_raw from Box<T, A> to Box<T, Global>.

@RalfJung
Copy link
Member

Yeah it has to do with this comment. This is part of the general issue that adding allocators to Box was done without considering the fact that Box is a primitive type that has special treatment in the operational semantics (and all over the compiler).

@Amanieu
Copy link
Member Author

Amanieu commented May 19, 2025

Sure, but the implementation of Box::into_raw hasn't changed at all in this PR: https://github.com/Amanieu/rust/blob/ac178fc93c2e3ee1a7e53a14db69551a3207fb66/library/alloc/src/boxed.rs#L1152

I don't see why changing the bounds on that methods from Box<T, A> to Box<T, Global> makes miri tests now fail.

@RalfJung
Copy link
Member

Miri has no choice but use some terrible hacks to work around the poor state that Box is in due to custom allocators at the moment. Those hacks are sensitive to whether code is generic or monomorphic. I'll try to take a look and see if we can update those hacks to be compatible with this PR.

@RalfJung
Copy link
Member

I pushed a commit that should hopefully fix CI.

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

r=me pending approval from the rest of libs-api

@safinaskar
Copy link
Contributor

@rustbot label +A-box +A-allocators +A-MIR +A-miri

@rustbot rustbot added A-allocators Area: Custom and system allocators A-box Area: Our favorite opsem complication A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels May 25, 2025
@rustbot rustbot added the A-miri Area: The miri tool label May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators A-box Area: Our favorite opsem complication A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-miri Area: The miri tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants