[PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Jubilee Young]

T-libs-api has consensus for stabilizing some of `feature(new_uninit)`,
but not for `Box<MaybeUninit<T>>::write`. Instead, we can use
`MaybeUninit<T>::write`, so Rust for Linux can drop the feature after
stabilization. That will happen after merging, as the FCP has completed:
https://github.com/rust-lang/rust/issues/63291#issuecomment-2183022955

This is required before stabilization because remaining-unstable API
will be divided into new features. This code doesn't know about those
yet. It can't: they haven't landed, as the relevant PR is blocked on
rustc's CI testing Rust-for-Linux without this patch.
 
Signed-off-by: Jubilee Young <workingjubilee@gmail.com>
---
 rust/kernel/alloc/box_ext.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/alloc/box_ext.rs b/rust/kernel/alloc/box_ext.rs
index b68ade26a42d..6179d893ff3a 100644
--- a/rust/kernel/alloc/box_ext.rs
+++ b/rust/kernel/alloc/box_ext.rs
@@ -37,8 +37,10 @@ pub trait BoxExt<T>: Sized {
 
 impl<T> BoxExt<T> for Box<T> {
     fn new(x: T, flags: Flags) -> Result<Self, AllocError> {
-        let b = <Self as BoxExt<_>>::new_uninit(flags)?;
-        Ok(Box::write(b, x))
+        let mut b = <Self as BoxExt<_>>::new_uninit(flags)?;
+        b.write(x);
+        // SAFETY: We just wrote to it.
+        Ok(unsafe { b.assume_init() })
     }
 
     #[cfg(any(test, testlib))]
-- 
2.45.2

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Trevor Gross]

On Fri, Aug 23, 2024 at 12:04 AM Jubilee Young <workingjubilee@gmail.com> wrote:
>
> T-libs-api has consensus for stabilizing some of `feature(new_uninit)`,
> but not for `Box<MaybeUninit<T>>::write`. Instead, we can use
> `MaybeUninit<T>::write`, so Rust for Linux can drop the feature after
> stabilization. That will happen after merging, as the FCP has completed:
> https://github.com/rust-lang/rust/issues/63291#issuecomment-2183022955
>
> This is required before stabilization because remaining-unstable API
> will be divided into new features. This code doesn't know about those
> yet. It can't: they haven't landed, as the relevant PR is blocked on
> rustc's CI testing Rust-for-Linux without this patch.
>
> Signed-off-by: Jubilee Young <workingjubilee@gmail.com>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Additional context of the CI job is at [1]. It's probably worthwhile
to add a mechanism for patching the kernel in the rust-lang RFL CI
job, so we can do things like this without needing to pick between
blocking rustc and disabling the CI job. (I thought this may have
existed already but I am not seeing it).

Miguel - we probably want this in fixes so we can continue building
the current kernel with upcoming rustc.

[1]: https://github.com/rust-lang/rust/pull/129401

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Miguel Ojeda]

On Fri, Aug 23, 2024 at 7:44 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> Additional context of the CI job is at [1]. It's probably worthwhile
> to add a mechanism for patching the kernel in the rust-lang RFL CI
> job, so we can do things like this without needing to pick between
> blocking rustc and disabling the CI job. (I thought this may have
> existed already but I am not seeing it).

What we discussed was to simply give them a different hash/tag/branch
when this would happen.

I don't think a patching system is needed on their side, since they
don't have a requirement to keep the source fixed.

So I will just give them a new target which can be put in the PR to
re-try. There should be no need to disable the CI nor block rustc for
simple cases like this.

Thanks for reacting quickly in the PR, Trevor!

Cheers,
Miguel

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Trevor Gross]

On Fri, Aug 23, 2024 at 1:11 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> What we discussed was to simply give them a different hash/tag/branch
> when this would happen.
>
> I don't think a patching system is needed on their side, since they
> don't have a requirement to keep the source fixed.
>
> So I will just give them a new target which can be put in the PR to
> re-try. There should be no need to disable the CI nor block rustc for
> simple cases like this.

After [1] I was thinking that staying on a tagged version was the
goal. If this isn't the case then sounds reasonable, thanks for the
clarification!

> Thanks for reacting quickly in the PR, Trevor!

Of course :)

- Trevor

[1]: https://github.com/rust-lang/rust/pull/128322

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Miguel Ojeda]

On Fri, Aug 23, 2024 at 8:25 AM Trevor Gross <tmgross@umich.edu> wrote:
>
> After [1] I was thinking that staying on a tagged version was the
> goal. If this isn't the case then sounds reasonable, thanks for the
> clarification!

Definitely -- if such a tag exists in mainline, we should use that.

Cheers,
Miguel

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Alice Ryhl]

On Fri, Aug 23, 2024 at 7:04 AM Jubilee Young <workingjubilee@gmail.com> wrote:
>
> T-libs-api has consensus for stabilizing some of `feature(new_uninit)`,
> but not for `Box<MaybeUninit<T>>::write`. Instead, we can use
> `MaybeUninit<T>::write`, so Rust for Linux can drop the feature after
> stabilization. That will happen after merging, as the FCP has completed:
> https://github.com/rust-lang/rust/issues/63291#issuecomment-2183022955
>
> This is required before stabilization because remaining-unstable API
> will be divided into new features. This code doesn't know about those
> yet. It can't: they haven't landed, as the relevant PR is blocked on
> rustc's CI testing Rust-for-Linux without this patch.
>
> Signed-off-by: Jubilee Young <workingjubilee@gmail.com>

This looks good to me. It should work both before and after the change in rustc.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

I'm not sure if this should have a Fixes tag? The original code is
from commit 08d3f5492879 ("rust: alloc: introduce the `BoxExt` trait")
which got merged in 6.10.

Alice

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Miguel Ojeda]

On Fri, Aug 23, 2024 at 1:10 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> I'm not sure if this should have a Fixes tag? The original code is
> from commit 08d3f5492879 ("rust: alloc: introduce the `BoxExt` trait")
> which got merged in 6.10.

There was no bug in the original code, so I don't think so.

Cheers,
Miguel

Re: [PATCH] rust: Eschew `Box<MaybeUninit<T>>::write`

[Miguel Ojeda]

On Fri, Aug 23, 2024 at 7:04 AM Jubilee Young <workingjubilee@gmail.com> wrote:
>
> T-libs-api has consensus for stabilizing some of `feature(new_uninit)`,
> but not for `Box<MaybeUninit<T>>::write`. Instead, we can use
> `MaybeUninit<T>::write`, so Rust for Linux can drop the feature after
> stabilization. That will happen after merging, as the FCP has completed:
> https://github.com/rust-lang/rust/issues/63291#issuecomment-2183022955
>
> This is required before stabilization because remaining-unstable API
> will be divided into new features. This code doesn't know about those
> yet. It can't: they haven't landed, as the relevant PR is blocked on
> rustc's CI testing Rust-for-Linux without this patch.
>
> Signed-off-by: Jubilee Young <workingjubilee@gmail.com>

Applied to `rust-fixes` -- thanks everyone!

    [ The PR has landed [2] and will be released in Rust 1.82.0 (expected on
      2024-10-17), so we could conditionally enable the new unstable feature
      (`box_uninit_write` [3]) instead, but just for a single `unsafe` block
      it is probably not worth it. For the time being, I added it to the
      "nice to have" section of our unstable features list. - Miguel ]

    [ Reworded slightly. - Miguel ]

Cheers,
Miguel