-
Notifications
You must be signed in to change notification settings - Fork 431
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
Switch to CoercePointee macro, with examples #1130
base: rust-next
Are you sure you want to change the base?
Switch to CoercePointee macro, with examples #1130
Conversation
rust/kernel/list/arc.rs
Outdated
// This is to allow coercion from `ListArc<T>` to `ListArc<U>` if `T` can be converted to the | ||
// dynamically-sized type (DST) `U`. | ||
impl<T, U, const ID: u64> core::ops::CoerceUnsized<ListArc<U, ID>> for ListArc<T, ID> | ||
where | ||
T: ListArcSafe<ID> + Unsize<U> + ?Sized, | ||
U: ListArcSafe<ID> + ?Sized, | ||
{ | ||
} | ||
|
||
// This is to allow `ListArc<U>` to be dispatched on when `ListArc<T>` can be coerced into | ||
// `ListArc<U>`. | ||
impl<T, U, const ID: u64> core::ops::DispatchFromDyn<ListArc<U, ID>> for ListArc<T, ID> | ||
where | ||
T: ListArcSafe<ID> + Unsize<U> + ?Sized, | ||
U: ListArcSafe<ID> + ?Sized, | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't delete these due to support for older rustc releases. Please see this doc for info on conditional compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, our current minimum version is 1.78.0, so you can use that to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will revert those chunks and wrap it with a conditional
27e8103
to
36e9396
Compare
From now I am going to squash the commits a bit and attach proper commit messages. I will run the checks over the commit as per instructions. |
Sounds great, thanks for doing that -- please also take a look at: https://rust-for-linux.com/contributing#submit-checklist-addendum (i.e. our own list, mostly about running By the way, for linking to kernel docs, I recommend docs.kernel.org, e.g. the latest version would be at: https://docs.kernel.org/process/submitting-patches.html |
rust/kernel/sync/arc.rs
Outdated
@@ -471,13 +479,16 @@ impl<T: ?Sized> From<Pin<UniqueArc<T>>> for Arc<T> { | |||
/// obj.as_arc_borrow().use_reference(); | |||
/// # Ok::<(), Error>(()) | |||
/// ``` | |||
#[cfg_attr(CONFIG_RUST_COERCE_POINTEE, repr(transparent))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be unconditional
samples/rust/rust_print.rs
Outdated
#[cfg(not(CONFIG_RUST_COERCE_POINTEE))] | ||
pr_info!( | ||
"The demonstration for dynamic dispatching through Arc is skipped. " | ||
"To see the print-out from this example, please upgrade your Rust compiler to 1.83.0 or higher.\n" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this example limited. Does the logic we have pre-1.83 not work for the sample?
rust/kernel/list/arc.rs
Outdated
/// `ListArc` is well-behaved so that its dereferencing operation does not mutate. | ||
#[cfg(CONFIG_RUST_COERCE_POINTEE)] | ||
unsafe impl<T: ?Sized + ListArcSafe<ID>, const ID: u64> core::pin::PinCoerceUnsized | ||
for ListArc<T, ID> | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need PinCoerceUnsized
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Dropped.
rust/kernel/lib.rs
Outdated
#![cfg_attr( | ||
CONFIG_RUST_COERCE_POINTEE, | ||
feature(derive_coerce_pointee, pin_coerce_unsized_trait) | ||
)] | ||
#![cfg_attr( | ||
not(CONFIG_RUST_COERCE_POINTEE), | ||
feature(coerce_unsized, dispatch_from_dyn, unsize) | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this format better if we split each feature out on its own line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have splitted them in the final patch. I will push the results shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good and ready for submission. Just one nit:
rust/kernel/sync/arc.rs
Outdated
#[cfg(CONFIG_RUST_COERCE_POINTEE)] | ||
use core::marker::CoercePointee; | ||
#[cfg(not(CONFIG_RUST_COERCE_POINTEE))] | ||
use core::marker::Unsize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use full paths for these instead of the imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fully qualified paths looks much better and easier to reason given all the cfg
conditional.
Since Rust 1.84.0 the macro `CoercePointee` has been made public on Nightly, so that it answers the some usability questions. If one wants to equip generic types with the ability to weaken itself and to work with unsized types with dynamic dispatching. This feature is useful such that Rust code are enabled to work with a family of types satisfying the same protocol or Rust traits, while the same safety guarantees are still uphold [1]. Examples of this weakening include those from *[u8; 8]* to *[u8]*, eliding the concrete size of the array; and a concrete type *T* to *dyn Trait* where *T* implements the trait or traits *Trait*. As of date, the exact language features to enable this type weakening is still under stabilization effort. Nevertheless, Alice Ryhl has proposed [2] a very valuable combination of them such that a user can enable this feature via a procedural macro `CoercePointee` without much verbosity and without declaring dependence on the relevant unstable language features. Alice has previously filed a patch [3] to demonstrate the capability of this macro. This patch provides further updates to incorporate recent changes to the proposal in [2] and paves the way for the final stabilization of the feature in the Rust language. A minimal demostration code is added to the *samples/rust/rust_print_main.rs* module. The use of the macro is now gated behind the available Rust version *1.83.0*. The *kernel* crate will still be as functional on the prior Rust toolchains. Link: https://doc.rust-lang.org/stable/nomicon/exotic-sizes.html?highlight=dynamic#dynamically-sized-types-dsts [1] Link: https://rust-lang.github.io/rfcs/3621-derive-smart-pointer.html [2] Link: https://lore.kernel.org/all/[email protected]/ [3] Signed-off-by: Xiangfei Ding <[email protected]>
507c541
to
06e4e20
Compare
Target audience: first timers to build kernel with Rust support
This patch demonstrates the new capability from the current Rust Nightly, which now supports the
#[derive(CoercePointee)]
macro to derive the key trait implementations, in place of use of unstable language features such asUnsize
andDispatchFromDyn
.An illustrative example is attached for more details in the Rust sample kernel module.
How to test this
As usual, you shall follow the guide to set up the toolchain: https://docs.kernel.org/rust/quick-start.html. This demonstration can be executed through virtualization. In this case, during configuration through make LLVM=1 menuconfig, be sure to select Enable Paravirtualization code and KVM Guest support under Processor type and features/Linux guest support; also select Printing Macros as modularised under Kernel hacking/Sample kernel code/Rust samples. Initiate build with make LLVM=1 optionally with -j for parallelism.
Meanwhile, you will need busybox from https://www.busybox.net/downloads/binaries/1.35.0-x86_64-linux-musl/busybox. I am running a x86_64 box, so the demo uses qemu-system-x86_64 as the emulator. I will use the usr/gen_init_cpio script for generating the initramfs image initram.img. Here is the description I used.
I used this command to launch the emulation and it dropped to a shell.
In the shell I executed
insmod rust_print.ko
.What would you see
Upon successful launch, a quick
insmod rust_print.ko
will generate some kernel log. You may watch out for the following log stanza, especially the lines containing the textArc<dyn Display>
.TODO(@wieDasDing): work on the patch epilogue a bit more