Introduce the unsafe_define_aref_type macro

[bjorn3]

This macro generates a Rust wrapper for a C struct that is always reference counted and implements AlwaysRefCounted for it. Additionally it makes sure that UnsafeCell is used to avoid UB and adds relevant documentation.

Signed-off-by: Björn Roy Baron [email protected]

[bjorn3]

My goal with this PR is twofold: To make it easier to define rust wrappers in a sound way and to make it a bit easier to allow generating the rust wrapper using an annotation in the C header file if we want that.

[ojeda]

On one hand it may make it a bit less readable at first glance to new readers, but the macro looks straightforward enough, and one can always use the generated docs or an IDE to get the "expanded" version again.

to make it a bit easier to allow generating the rust wrapper using an annotation in the C header file if we want that.

That is an interesting suggestion, are you thinking about something like...

/*
 * ...
 * same context as task->real_cred.
 */
__rust_aref_type(...)
struct cred {
    atomic_t usage;
    ...
};

? I imagine they would all go into a private module, and then we would use a re-export in the right place of the kernel crate to place it, right? Or do you have something else in mind?

I like that it would make both C and Rust sides more integrated, though it depends on how much we end up needing to put in the (...) (the less, the better -- in particular, if we need to write Rust syntax like pub struct there, instead of e.g. just the get_cred, put_cred symbols, it may look a bit too invasive).

[bjorn3]

That is an interesting suggestion, are you thinking about something like...

/*
 * ...
 * same context as task->real_cred.
 */
__rust_aref_type(...)
struct cred {
    atomic_t usage;
    ...
};

? I imagine they would all go into a private module, and then we would use a re-export in the right place of the kernel crate to place it, right? Or do you have something else in mind?

Pretty much.

I like that it would make both C and Rust sides more integrated, though it depends on how much we end up needing to put in the (...) (the less, the better -- in particular, if we need to write Rust syntax like pub struct there, instead of e.g. just the get_cred, put_cred symbols, it may look a bit too invasive).

I think we only need __rust_aref_type(Credential, get_cred, put_cred). We may even be able to skip the rust type name and derive it by changing the type name from snake case to camel case automatically or even using the C type name, but renaming it in the re-export. The pub struct Credential(cred) is not necessary. I just used it in this macro to allow specifying the visibility and to make it clear that it is a wrapper. The get and put function names are necessary though as the kernel doesn't use a consistent naming scheme for them.

[ojeda]

I think we only need __rust_aref_type(Credential, get_cred, put_cred). We may even be able to skip the rust type name and derive it by changing the type name from snake case to camel case automatically or even using the C type name, but renaming it in the re-export. The pub struct Credential(cred) is not necessary. I just used it in this macro to allow specifying the visibility and to make it clear that it is a wrapper. The get and put function names are necessary though as the kernel doesn't use a consistent naming scheme for them.

Yeah, that matches exactly what I was thinking: by automatically deducing the type, we could get (get_cred, put_cred) which looks very clean; but on the other hand, (Credential, get_cred, put_cred) gives readers and possibly tools a clear way to find out the Rust type, i.e. the "reverse direction" to what we have in the Rust docs "Wraps the kernel's struct cred". So I think the type name may not hurt, and we could even teach kernel-doc to generate the link (somehow figuring the path of the Rust re-export).

I also wondered about linking the get/set from the declaration of each C symbol by using the type name, but while in a small example like below it may look nice and makes each annotation smaller, in practice I think it will be worse since things tend to be sprinkled around (i.e. rather than close), it takes more lines and extra annotations, more complexity on the tooling probably, etc.:

__rust_aref_type(Credential)
struct cred { ... };

__rust_aref_get(Credential)
static inline const struct cred *get_cred(const struct cred *cred) { ... };

__rust_aref_put(Credential)
static inline void put_cred(const struct cred *_cred) { ... };

[ojeda]

LGTM, by the way, though let's see what others think about the macro approach. Also, maybe later on when AlwaysRefCounted lands in rust-next eventually you can use it as your first patches to the list using the b4 web submission endpoint (or it could go together with AlwaysRefCounted in the same batch).

[wedsonaf]

I really like the idea of having a macro for this.

One concern that I have with this implementation is that it introduces another pattern for us to "search" for when looking for missing safety annotations. Without this patch, I can search for "unsafe {" or "unsafe impl", but with this patch I have some ad-hoc name.

Could we try to find a reasonable way to introduce either "unsafe {" or "unsafe impl" in the macro call sites so that we can continue to straightforwardly find the spots where SAFETY comments are needed?

[bjorn3]

Prefixing the macro name with unsafe_ is a common pattern. For example:

• https://github.com/rust-lang/rust/blob/442f997f98ac9f16f60ba3a7109f884dbf8d370c/library/core/src/intrinsics.rs#L2214
• https://github.com/gimli-rs/object/blob/b3096ce8d1ca6ac920cc1663cb6d67e00c377dde/src/pod.rs#L149

I can add an unsafe keyword to the macro invocation body though.

[fbq]

maybe

inc = get_cred;
dec = put_cred;

instead? For two reasons:

• the methods of AlwaysRefCounted are inc_ref and dec_ref, using inc and dec is more consistent
• get in Rust has a lightly different meaning, e.g. UnsafeCell::get

[bjorn3]

Rebased and changed base branch from rust to rust-next, previous version is at 793aa0b. The only AlwaysRefCounted type on the current rust-next branch is Task, so I had to discard the changes for the other types this PR used to change.