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

Support custom allocators in Box #77187

Merged
merged 5 commits into from
Oct 26, 2020
Merged

Support custom allocators in Box #77187

merged 5 commits into from
Oct 26, 2020

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Sep 25, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2020
@TimDiekmann
Copy link
Member Author

@nikomatsakis wrote in #71873 (comment)

The chalk error is legit in the sense that we haven't defined what fundamental means for a type with multiple type parameters. I guess we will ignore all but the first, but we're going to have to spend at least some time thinking about how to use this to break coherence :)

What is the state of this?

@jackh726
Copy link
Member

@nikomatsakis wrote in #71873 (comment)

The chalk error is legit in the sense that we haven't defined what fundamental means for a type with multiple type parameters. I guess we will ignore all but the first, but we're going to have to spend at least some time thinking about how to use this to break coherence :)

What is the state of this?

No change. But I'll be sure to bring this up in the wg-traits meeting on tuesday.

@TimDiekmann
Copy link
Member Author

That would be great, thank you!

@TimDiekmann
Copy link
Member Author

Rebased onto master to include #77118

library/alloc/src/boxed.rs Outdated Show resolved Hide resolved
library/alloc/src/boxed.rs Outdated Show resolved Hide resolved
where
T: 'a, // Technically not needed, but kept to be explicit.
A: 'a,
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test, which ensures, that the allocator must not be moved while the leaked data is in use.

@jackh726
Copy link
Member

Just wanted to check in to say we did talk about this today at our @rust-lang/wg-traits meeting. We worked out a design that we're hoping to get into Chalk by next release on Saturday. Then it'll just be a Chalk upgrade and this should be unblocked from that.

@TimDiekmann
Copy link
Member Author

That's awesome, thank you!

@TimDiekmann
Copy link
Member Author

Rebased onto master to include #77289

@bors
Copy link
Contributor

bors commented Oct 2, 2020

☔ The latest upstream changes (presumably #77436) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@jyn514 jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. A-allocators Area: Custom and system allocators T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2020
library/alloc/src/boxed.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Oct 5, 2020

☔ The latest upstream changes (presumably #77466) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Remove `Box::leak_with_alloc`


Add leak-test for box with allocator


Rename `AllocErr` to `AllocError` in leak-test


Add `Box::alloc` and adjust examples to use the new API
@TimDiekmann
Copy link
Member Author

Rebased onto master to include #77515.

@rustbot modify labels: +S-waiting-on-review -S-blocked

@rustbot rustbot removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 7, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #77187!

Tested on commit fd54259.
Direct link to PR: #77187

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

@bors bors merged commit fd54259 into rust-lang:master Oct 26, 2020
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Oct 26, 2020
Tested on commit rust-lang/rust@fd54259.
Direct link to PR: <rust-lang/rust#77187>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
@rustbot rustbot added this to the 1.49.0 milestone Oct 26, 2020
@TimDiekmann TimDiekmann deleted the box-alloc branch October 26, 2020 23:23

// The box is unitiliazed later when moving out the allocator. The pointer is stored
// beforehand.
let ptr = b.0;
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid this code broke Stacked Borrows. The code before was very careful to go through a mutable reference, as the comment explains. That comment was retained but the mutable reference is no longer used, so this code no longer works.

I'll submit a PR to fix this. In the future, when you find comments like this and are not sure what to do about them, feel free to ping me. :)

bors added a commit to rust-lang/miri that referenced this pull request Oct 27, 2020
test Box::into_raw aliasing

Directly test aliasing problems caused by `Box::into_raw` issues (like we have them again right now due to rust-lang/rust#77187, but the pinned rustc is older than that so this should still be able to land).
@RalfJung RalfJung mentioned this pull request Oct 27, 2020
bors added a commit to rust-lang/miri that referenced this pull request Oct 27, 2020
test Box::into_raw aliasing

Directly test aliasing problems caused by `Box::into_raw` issues (like we have them again right now due to rust-lang/rust#77187, but the pinned rustc is older than that so this should still be able to land).
bors added a commit to rust-lang/miri that referenced this pull request Oct 27, 2020
test Box::into_raw aliasing

Directly test aliasing problems caused by `Box::into_raw` issues (like we have them again right now due to rust-lang/rust#77187, but the pinned rustc is older than that so this should still be able to land).
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2020
fix Box::into_unique

rust-lang#77187 broke Stacked Borrows pointer tagging around `Box::into_unique` (this is caused by `Box` being a special case in the type system, which box-internal code needs to account for). This PR fixes that.

r? `@Amanieu` Cc `@TimDiekmann`

Fixes rust-lang#78419.
@H2CO3
Copy link

H2CO3 commented Nov 28, 2020

I would also like to raise awareness about the issue of this breaking ABI here, more in my comment over at the related PR for Vec.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jan 15, 2021

Is there a write-up available on this design? I've been reading through the custom allocator PRs, and don't have a good sense why for example #58457 was abandoned in favor of the current design. The designs appear to differ, and I'd like to better understand why that choice was made.

edit: I guess the right place to ask would be in #32838 which tracks this work. I didn't see the issue linked from here, so I missed it. Apologies!

edit: The allocator WG appears to be tracking the methods introduced in this PR here: rust-lang/wg-allocators#7. This is the exact resource I was looking for.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 15, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 15, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2022
@RalfJung
Copy link
Member

FWIW this PR caused a lot of pain in the compiler, a long stream of ICEs, accidental UB -- altogether causing a lot of work for a lot of people. See #95453 for more details.

This should never have landed without a discussion with the compiler team on how to support Box with an extra field. Box is a language-primitive type, its definition cannot change without adjusting many parts of the compiler and even the language spec.

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 merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.