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

Constify Box<T, A> methods #91884

Merged
merged 3 commits into from
Jan 4, 2022
Merged

Constify Box<T, A> methods #91884

merged 3 commits into from
Jan 4, 2022

Conversation

lilasta
Copy link
Contributor

@lilasta lilasta commented Dec 14, 2021

Tracking issue: #92521

Most of the methods bounded on ~const. intrinsics::const_eval_select is used for handling an allocation error.

Constified API
impl<T, A: Allocator> Box<T, A> {
    pub const fn new_in(x: T, alloc: A) -> Self
    where
        A: ~const Allocator + ~const Drop;
    pub const fn try_new_in(x: T, alloc: A) -> Result<Self, AllocError>
    where
        T: ~const Drop,
        A: ~const Allocator + ~const Drop;
    pub const fn new_uninit_in(alloc: A) -> Box<mem::MaybeUninit<T>, A>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn try_new_uninit_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn new_zeroed_in(alloc: A) -> Box<mem::MaybeUninit<T>, A>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn try_new_zeroed_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn pin_in(x: T, alloc: A) -> Pin<Self>
    where
        A: 'static,
        A: 'static + ~const Allocator + ~const Drop,
    pub const fn into_boxed_slice(boxed: Self) -> Box<[T], A>;
    pub const fn into_inner(boxed: Self) -> T
    where
        Self: ~const Drop,
}

impl<T, A: Allocator> Box<MaybeUninit<T>, A> {
    pub const unsafe fn assume_init(self) -> Box<T, A>;
    pub const fn write(mut boxed: Self, value: T) -> Box<T, A>;
    pub const unsafe fn from_raw_in(raw: *mut T, alloc: A) -> Self;
    pub const fn into_raw_with_allocator(b: Self) -> (*mut T, A);
    pub const fn into_unique(b: Self) -> (Unique<T>, A);
    pub const fn allocator(b: &Self) -> &A;
    pub const fn leak<'a>(b: Self) -> &'a mut T
    where
        A: 'a;
    pub const fn into_pin(boxed: Self) -> Pin<Self>
    where
        A: 'static;
}

unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> const Drop for Box<T, A>;
impl<T: ?Sized, A: Allocator> const From<Box<T, A>> for Pin<Box<T, A>>
where
    A: 'static;
impl<T: ?Sized, A: Allocator> const Deref for Box<T, A>;
impl<T: ?Sized, A: Allocator> const DerefMut for Box<T, A>;
impl<T: ?Sized, A: Allocator> const Unpin for Box<T, A> where A: 'static;
Example
pub struct ConstAllocator;

unsafe impl const Allocator for ConstAllocator {
    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        unsafe {
            let ptr = core::intrinsics::const_allocate(layout.size(), layout.align());
            Ok(NonNull::new_unchecked(ptr as *mut [u8; 0] as *mut [u8]))
        }
    }

    unsafe fn deallocate(&self, _ptr: NonNull<u8>, _layout: Layout) {
        /* do nothing */
    }

    fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        self.allocate(layout)
    }

    unsafe fn grow(
        &self,
        _ptr: NonNull<u8>,
        _old_layout: Layout,
        _new_layout: Layout,
    ) -> Result<NonNull<[u8]>, AllocError> {
        unimplemented!()
    }

    unsafe fn grow_zeroed(
        &self,
        _ptr: NonNull<u8>,
        _old_layout: Layout,
        _new_layout: Layout,
    ) -> Result<NonNull<[u8]>, AllocError> {
        unimplemented!()
    }

    unsafe fn shrink(
        &self,
        _ptr: NonNull<u8>,
        _old_layout: Layout,
        _new_layout: Layout,
    ) -> Result<NonNull<[u8]>, AllocError> {
        unimplemented!()
    }

    fn by_ref(&self) -> &Self
    where
        Self: Sized,
    {
        self
    }
}

#[test]
fn const_box() {
    const VALUE: u32 = {
        let mut boxed = Box::new_in(1u32, ConstAllocator);
        assert!(*boxed == 1);

        *boxed = 42;
        assert!(*boxed == 42);

        *boxed
    };

    assert!(VALUE == 42);
}

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 14, 2021
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 15, 2021

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

@rust-log-analyzer

This comment has been minimized.

library/alloc/src/boxed.rs Outdated Show resolved Hide resolved
_old_layout: Layout,
_new_layout: Layout,
) -> Result<NonNull<[u8]>, AllocError> {
unimplemented!()
Copy link
Member

Choose a reason for hiding this comment

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

Please implement these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a type for testing, so I omitted some implementations. Should I still implement these?

Copy link
Member

Choose a reason for hiding this comment

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

It is fine to leave them unimplemented for now, but I think they should be implemented before merging. You may be able to copy the default implementation of these methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented these, but the methods aren't called in the testcases, we can't check if they are implemented correctly. Also we may need to implement const_deallocate to the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Const deallocation is simply leaking the thing. The mir interpreter cleans up all dangling allocations

Copy link
Member

Choose a reason for hiding this comment

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

Also we may need to implement const_deallocate to the compiler.

I just saw you open a PR to add this, let me emphasize: This PR cannot contain any use of that intrinsic. The nature of ConstAlloc/CtfeAlloc is to leak when dropping, which means you can't realistically use it in this test allocator impl.

Copy link
Member

Choose a reason for hiding this comment

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

The nature of ConstAlloc/CtfeAlloc is to leak when dropping

And why is that?
I don't think this should be the case, at least for allocations that are created and then deallocated within the same const-evalaution. (Of course, at runtime deallocation is a NOP, since static memory cannot be deallocated and it is shared among all the copies of the constant anyway.)

Copy link
Member

Choose a reason for hiding this comment

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

Well, looks like const_eval_select can resolve this problem faily easily.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that is enough -- if one const works on something that was allocated in a different const, deallocation should still be a NOP.

@rust-log-analyzer

This comment has been minimized.

library/alloc/src/alloc.rs Outdated Show resolved Hide resolved
@lilasta lilasta marked this pull request as ready for review December 23, 2021 04:10
@fee1-dead
Copy link
Member

fee1-dead commented Dec 23, 2021

cc @rust-lang/wg-const-eval

Should we do this before offering ConstAlloc as an implementation and figure out what the best approach to const alloc is?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 23, 2021

Yea, I would prefer a ConstAlloc to play with first, too. But I'll have a look at the impl, maybe we could merge it and have an arena allocator test to show it works without allowing GlobalAlloc ?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 23, 2021

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned yaahc Dec 23, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Dec 23, 2021

Oh hold up. I should've read the PR first. I think this is fine as it is? Or am I missing sth? Even if we go a magic route, we'll want this, right?

match layout.size() {
0 => { /* do nothing */ }
_ => { /* do nothing too */ }
}
Copy link
Member

Choose a reason for hiding this comment

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

I think if we have something like const_allocate dynamically creating allocations during const-eval, we should also have a corresponding const_deallocate to get rid of them again. This will improve UB detection and reduce memory consumption during CTFE.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 23, 2021

@woppopo how motivated are you feeling right now 😄? If a lot, could you create a ui test showing how to use a Box with an array-backed arena allocator?

library/alloc/src/boxed.rs Outdated Show resolved Hide resolved
library/alloc/src/boxed.rs Show resolved Hide resolved
@lilasta
Copy link
Contributor Author

lilasta commented Dec 25, 2021

@woppopo how motivated are you feeling right now 😄? If a lot, could you create a ui test showing how to use a Box with an array-backed arena allocator?

@oli-obk
I encountered an ICE... 😢
#92272 (duplicate)
#78459

@oli-obk
Copy link
Contributor

oli-obk commented Dec 25, 2021

Oh no 😄 I guess we'll have to wait for that bug to be fixed or for the same constify work to be done for vec

So... a tracking issue with a reference to this problem is all that's needed for this PR

@lilasta
Copy link
Contributor Author

lilasta commented Jan 3, 2022

Oh no 😄 I guess we'll have to wait for that bug to be fixed or for the same constify work to be done for vec
So... a tracking issue with a reference to this problem is all that's needed for this PR.

ConstAllocator used for the test is a zero sized type, this PR probably isn't affected by the problem.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 3, 2022

Oh yea, definitely

The PR just needs a tracking issue and the feature gates should reference that

@oli-obk
Copy link
Contributor

oli-obk commented Jan 3, 2022

@bors r+ rollup

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 3, 2022
@lilasta
Copy link
Contributor Author

lilasta commented Jan 3, 2022

I've added tracking issues. By the way, I just realized that we can test Box::new_in with array-backed allocator by using static variables internally...
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=f1ed3eb26a714e269439c7136b6043dd

@lilasta
Copy link
Contributor Author

lilasta commented Jan 3, 2022

I made a mistake in previous commit, re-pushed. Sorry, please re-approve...

@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2022

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 3, 2022

📌 Commit c9d2d3c has been approved by oli-obk

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2022

@bors r- delegate+
please do bors r=oli-obk when CI is green :)

@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 3, 2022
@bors
Copy link
Contributor

bors commented Jan 3, 2022

✌️ @woppopo can now approve this pull request

@lilasta
Copy link
Contributor Author

lilasta commented Jan 3, 2022

I'm so sorry to bother you again🙇...

@lilasta
Copy link
Contributor Author

lilasta commented Jan 3, 2022

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 3, 2022

📌 Commit 51e4291 has been approved by oli-obk

@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 3, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 3, 2022
Constify `Box<T, A>` methods

Tracking issue: none yet

Most of the methods bounded on `~const`. `intrinsics::const_eval_select` is used for handling an allocation error.

<details><summary>Constified API</summary>

```rust
impl<T, A: Allocator> Box<T, A> {
    pub const fn new_in(x: T, alloc: A) -> Self
    where
        A: ~const Allocator + ~const Drop;
    pub const fn try_new_in(x: T, alloc: A) -> Result<Self, AllocError>
    where
        T: ~const Drop,
        A: ~const Allocator + ~const Drop;
    pub const fn new_uninit_in(alloc: A) -> Box<mem::MaybeUninit<T>, A>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn try_new_uninit_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn new_zeroed_in(alloc: A) -> Box<mem::MaybeUninit<T>, A>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn try_new_zeroed_in(alloc: A) -> Result<Box<mem::MaybeUninit<T>, A>, AllocError>
    where
        A: ~const Allocator + ~const Drop;
    pub const fn pin_in(x: T, alloc: A) -> Pin<Self>
    where
        A: 'static,
        A: 'static + ~const Allocator + ~const Drop,
    pub const fn into_boxed_slice(boxed: Self) -> Box<[T], A>;
    pub const fn into_inner(boxed: Self) -> T
    where
        Self: ~const Drop,
}

impl<T, A: Allocator> Box<MaybeUninit<T>, A> {
    pub const unsafe fn assume_init(self) -> Box<T, A>;
    pub const fn write(mut boxed: Self, value: T) -> Box<T, A>;
    pub const unsafe fn from_raw_in(raw: *mut T, alloc: A) -> Self;
    pub const fn into_raw_with_allocator(b: Self) -> (*mut T, A);
    pub const fn into_unique(b: Self) -> (Unique<T>, A);
    pub const fn allocator(b: &Self) -> &A;
    pub const fn leak<'a>(b: Self) -> &'a mut T
    where
        A: 'a;
    pub const fn into_pin(boxed: Self) -> Pin<Self>
    where
        A: 'static;
}

unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> const Drop for Box<T, A>;
impl<T: ?Sized, A: Allocator> const From<Box<T, A>> for Pin<Box<T, A>>
where
    A: 'static;
impl<T: ?Sized, A: Allocator> const Deref for Box<T, A>;
impl<T: ?Sized, A: Allocator> const DerefMut for Box<T, A>;
impl<T: ?Sized, A: Allocator> const Unpin for Box<T, A> where A: 'static;
```

</details>

<details><summary>Example</summary>

```rust
pub struct ConstAllocator;

unsafe impl const Allocator for ConstAllocator {
    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        unsafe {
            let ptr = core::intrinsics::const_allocate(layout.size(), layout.align());
            Ok(NonNull::new_unchecked(ptr as *mut [u8; 0] as *mut [u8]))
        }
    }

    unsafe fn deallocate(&self, _ptr: NonNull<u8>, _layout: Layout) {
        /* do nothing */
    }

    fn allocate_zeroed(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
        self.allocate(layout)
    }

    unsafe fn grow(
        &self,
        _ptr: NonNull<u8>,
        _old_layout: Layout,
        _new_layout: Layout,
    ) -> Result<NonNull<[u8]>, AllocError> {
        unimplemented!()
    }

    unsafe fn grow_zeroed(
        &self,
        _ptr: NonNull<u8>,
        _old_layout: Layout,
        _new_layout: Layout,
    ) -> Result<NonNull<[u8]>, AllocError> {
        unimplemented!()
    }

    unsafe fn shrink(
        &self,
        _ptr: NonNull<u8>,
        _old_layout: Layout,
        _new_layout: Layout,
    ) -> Result<NonNull<[u8]>, AllocError> {
        unimplemented!()
    }

    fn by_ref(&self) -> &Self
    where
        Self: Sized,
    {
        self
    }
}

#[test]
fn const_box() {
    const VALUE: u32 = {
        let mut boxed = Box::new_in(1u32, ConstAllocator);
        assert!(*boxed == 1);

        *boxed = 42;
        assert!(*boxed == 42);

        *boxed
    };

    assert!(VALUE == 42);
}
```

</details>
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91754 (Modifications to `std::io::Stdin` on Windows so that there is no longer a 4-byte buffer minimum in read().)
 - rust-lang#91884 (Constify `Box<T, A>` methods)
 - rust-lang#92107 (Actually set IMAGE_SCN_LNK_REMOVE for .rmeta)
 - rust-lang#92456 (Make the documentation of builtin macro attributes accessible)
 - rust-lang#92507 (Suggest single quotes when char expected, str provided)
 - rust-lang#92525 (intra-doc: Make `Receiver::into_iter` into a clickable link)
 - rust-lang#92532 (revert rust-lang#92254 "Bump gsgdt to 0.1.3")

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c7125ba into rust-lang:master Jan 4, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 4, 2022
@lilasta lilasta deleted the const_box branch January 5, 2022 01:30
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 29, 2022
Add `intrinsics::const_deallocate`

Tracking issue: rust-lang#79597
Related: rust-lang#91884

This allows deallocation of a memory allocated by `intrinsics::const_allocate`. At the moment, this can be only used to reduce memory usage, but in the future this may be useful to detect memory leaks (If an allocated memory remains after evaluation, raise an error...?).
@steffahn
Copy link
Member

steffahn commented Mar 10, 2022

FYI, y'all missed #94804 during review.

@@ -1908,7 +1951,8 @@ impl<T: ?Sized, A: Allocator> AsMut<T> for Box<T, A> {
* could have a method to project a Pin<T> from it.
*/
#[stable(feature = "pin", since = "1.33.0")]
impl<T: ?Sized, A: Allocator> Unpin for Box<T, A> where A: 'static {}
#[rustc_const_unstable(feature = "const_box", issue = "92521")]
impl<T: ?Sized, A: Allocator> const Unpin for Box<T, A> where A: 'static {}
Copy link
Member

Choose a reason for hiding this comment

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

What is const Unpin supposed to mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make sense to implement a marker trait as const, but I made it const with the intention of indicating that Box can be unpinned even at compile-time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.