Skip to content

Fix some Arc allocator leaks #120445

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 30, 2024
Merged

Fix some Arc allocator leaks #120445

merged 1 commit into from
Jan 30, 2024
+80 −29

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jan 28, 2024

This doesn't matter for the stable Global allocator as it is a ZST singleton, but other allocators may rely on all instances being dropped.

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2024

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@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 Jan 28, 2024
Mark-Simulacrum
Mark-Simulacrum requested changes Jan 28, 2024
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum 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 with nit fixed (or if I missed something and it can't be fixed then also fine to approve)

library/alloc/src/sync.rs
@@ -3443,8 +3445,8 @@ impl<T, A: Allocator + Clone, const N: usize> TryFrom<Arc<[T], A>> for Arc<[T; N

fn try_from(boxed_slice: Arc<[T], A>) -> Result<Self, Self::Error> {
if boxed_slice.len() == N {
let alloc = boxed_slice.alloc.clone();
Ok(unsafe { Arc::from_raw_in(Arc::into_raw(boxed_slice) as *mut [T; N], alloc) })
let (ptr, alloc) = boxed_slice.internal_into_inner_with_allocator();
Copy link
Member

Choose a reason for hiding this comment

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

In the cases above you also removed the A: Clone bound -- seems like we ought to do that here too?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2024
@Nemo157
Copy link
Member Author

Nemo157 commented Jan 28, 2024

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jan 28, 2024

📌 Commit daff4e0 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 28, 2024
@Nemo157
Copy link
Member Author

Nemo157 commented Jan 28, 2024

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 28, 2024
@Nemo157
Copy link
Member Author

Nemo157 commented Jan 28, 2024

Whoops, missed the impl that was actually commenting on.

@Nemo157
Fix some Arc allocator leaks
6837b81 
This doesn't matter for the stable `Global` allocator as it is a ZST
singleton, but other allocators may rely on all instances being dropped.
@Nemo157
Copy link
Member Author

Nemo157 commented Jan 28, 2024

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jan 28, 2024

📌 Commit 6837b81 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 28, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 30, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
@bors
Auto merge of rust-lang#120496 - GuillaumeGomez:rollup-fmu9jre, r=Gui…
f3d71c9 
…llaumeGomez

Rollup of 11 pull requests

Successful merges:

 - rust-lang#117906 (Improve display of crate name when hovered)
 - rust-lang#118533 (Suppress unhelpful diagnostics for unresolved top level attributes)
 - rust-lang#120293 (Deduplicate more sized errors on call exprs)
 - rust-lang#120295 (Remove `raw_os_nonzero` feature.)
 - rust-lang#120310 (adapt test for v0 symbol mangling)
 - rust-lang#120342 (Remove various `has_errors` or `err_count` uses)
 - rust-lang#120434 (Revert outdated version of "Add the wasm32-wasi-preview2 target")
 - rust-lang#120445 (Fix some `Arc` allocator leaks)
 - rust-lang#120475 (Improve error message when `cargo build` is used to build the compiler)
 - rust-lang#120476 (Remove some unnecessary check logic for lang items in HIR typeck)
 - rust-lang#120485 (add missing potential_query_instability for keys and values in hashmap)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a5aa355 into rust-lang:master Jan 30, 2024
@rustbot rustbot added this to the 1.77.0 milestone Jan 30, 2024
@Nemo157 Nemo157 deleted the arc-plug branch January 30, 2024 19:21
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
@rust-timer
Unrolled build for rust-lang#120445
1891b62 
Rollup merge of rust-lang#120445 - Nemo157:arc-plug, r=Mark-Simulacrum

Fix some `Arc` allocator leaks

This doesn't matter for the stable `Global` allocator as it is a ZST singleton, but other allocators may rely on all instances being dropped.
@zachs18 zachs18 mentioned this pull request Feb 10, 2024
Closed
@zachs18 zachs18 mentioned this pull request May 10, 2024
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 11, 2024
@GuillaumeGomez
Rollup merge of rust-lang#124981 - zachs18:rc-allocator-generalize-1,…
c3ecdb3 
… r=Mark-Simulacrum

Relax allocator requirements on some Rc/Arc APIs.

Split out from rust-lang#119761

* Remove `A: Clone` bound from `Rc::assume_init`(s), `Rc::downcast`, and `Rc::downcast_unchecked` (`Arc` methods were already relaxed by rust-lang#120445)
* Make `From<Rc<[T; N]>> for Rc<[T]>` allocator-aware (`Arc`'s already is).
* Remove `A: Clone` from `Rc/Arc::unwrap_or_clone`

Internal changes:

* Made `Arc::internal_into_inner_with_allocator` method into `Arc::into_inner_with_allocator` associated fn.
* Add private `Rc::into_inner_with_allocator` (to match Arc), so other fns don't have to juggle `ManuallyDrop`.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 11, 2024
@GuillaumeGomez
Rollup merge of rust-lang#124981 - zachs18:rc-allocator-generalize-1,…
a59ad73 
… r=Mark-Simulacrum

Relax allocator requirements on some Rc/Arc APIs.

Split out from rust-lang#119761

* Remove `A: Clone` bound from `Rc::assume_init`(s), `Rc::downcast`, and `Rc::downcast_unchecked` (`Arc` methods were already relaxed by rust-lang#120445)
* Make `From<Rc<[T; N]>> for Rc<[T]>` allocator-aware (`Arc`'s already is).
* Remove `A: Clone` from `Rc/Arc::unwrap_or_clone`

Internal changes:

* Made `Arc::internal_into_inner_with_allocator` method into `Arc::into_inner_with_allocator` associated fn.
* Add private `Rc::into_inner_with_allocator` (to match Arc), so other fns don't have to juggle `ManuallyDrop`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 11, 2024
@matthiaskrgr
Rollup merge of rust-lang#124981 - zachs18:rc-allocator-generalize-1,…
e3fca20 
… r=Mark-Simulacrum

Relax allocator requirements on some Rc/Arc APIs.

Split out from rust-lang#119761

* Remove `A: Clone` bound from `Rc::assume_init`(s), `Rc::downcast`, and `Rc::downcast_unchecked` (`Arc` methods were already relaxed by rust-lang#120445)
* Make `From<Rc<[T; N]>> for Rc<[T]>` allocator-aware (`Arc`'s already is).
* Remove `A: Clone` from `Rc/Arc::unwrap_or_clone`

Internal changes:

* Made `Arc::internal_into_inner_with_allocator` method into `Arc::into_inner_with_allocator` associated fn.
* Add private `Rc::into_inner_with_allocator` (to match Arc), so other fns don't have to juggle `ManuallyDrop`.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 12, 2024
@rust-timer
Unrolled build for rust-lang#124981
6c1975d 
Rollup merge of rust-lang#124981 - zachs18:rc-allocator-generalize-1, r=Mark-Simulacrum

Relax allocator requirements on some Rc/Arc APIs.

Split out from rust-lang#119761

* Remove `A: Clone` bound from `Rc::assume_init`(s), `Rc::downcast`, and `Rc::downcast_unchecked` (`Arc` methods were already relaxed by rust-lang#120445)
* Make `From<Rc<[T; N]>> for Rc<[T]>` allocator-aware (`Arc`'s already is).
* Remove `A: Clone` from `Rc/Arc::unwrap_or_clone`

Internal changes:

* Made `Arc::internal_into_inner_with_allocator` method into `Arc::into_inner_with_allocator` associated fn.
* Add private `Rc::into_inner_with_allocator` (to match Arc), so other fns don't have to juggle `ManuallyDrop`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum requested changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.77.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Nemo157 @rustbot @bors @Mark-Simulacrum