rust: generate bindings for helpers

[nbdd0121]

Helper functions are now only compiled if the wrapped function is not
made directly available via bindgen. For the compiled helper functions,
bindings for them are automatically generated via a second invocation
to bindgen, and the corresponding binding will have its rust_helper_
prefix removed, so Rust code can call bindings::foo regardless
whether foo is directly exposed or exposed via a helper.

Details about how this works:

• bindings_generated.rs is first created via bindgen;
• List of functions are extracted from bindings_generated.rs and a
  rust_helper_foo macro is generated for each function directly
  exposed this way;
• helpers.c can then use C preprocessor to conditionally compile
  helpers only if they are not directly exposed.
• bindgen invokes again to create bindings for helpers.c.
• bindgen output is then postprocessed to remove the rust_helper
  prefix and add #[link_name = ""].

Related #352

[TheSven73]

I'm not sure I get what this does... could you provide a layperson description ? :)

If I understand correctly, we'd still have to create rust helpers manually in helpers.c, it's just that through some magic, you can now call them as if they were part of the bindings:: namespace. Is that correct?

[nbdd0121]

If I understand correctly, we'd still have to create rust helpers manually in helpers.c, it's just that through some magic, you can now call them as if they were part of the bindings:: namespace. Is that correct?

Yes, and if the header exposes functions as extern symbols instead of macro or static inline, that externed symbol will be used directly and the helper won't be compiled.

It's not magic :). Here's some detailed example. We know that errname is either an extern function or a static inline function depending some config.

If it is an extern function, then bindings_generated.rs will have something like:

extern "C" {
    fn errname(err: c_types::c_int) -> *const c_types::c_char;
}

which will cause bindings_generated.h to have this line:

#define rust_helper_errname

which means in helpers.c, the following lines are not compiled

#ifndef rust_helper_errname
const char *rust_helper_errname(int err)
{
	return errname(err);
}
EXPORT_SYMBOL_GPL(rust_helper_errname);
#endif

On the other hand, if it's defined as macro or static inline function, bindings_generated.rs contains nothing about errname, so the #define line is not present in bindings_generated.h. So the lines in helpers.c will be compiled, and the second bindgen invocation will produce something like:

extern "C" {
    fn rust_helper_errname(err: c_types::c_int) -> *const c_types::c_char;
}

which I use sed to replace into

extern "C" {
    #[link_name = "rust_helper_errname"]
    fn errname(err: c_types::c_int) -> *const c_types::c_char;
}

So the caller don't need to care about whether an extern symbol exists or a helper is needed; it just calls bindings::errname, and it will transparently be linked directly or to the helper.

[nbdd0121]

v1 -> v2

changed the mechanism used to prevent name conflict

• previous C macros are used to conditional compile helpers, so a C header has to be generated
• now helpers are generated regardless, and glob import is used to prevent name conflict
• this means that some helpers will be dead code, but the logic is simpler and less magical

[ksquirrel]

Review of 453bd738c601:

• ✔️ Commit 453bd73: Looks fine!

[fbq]

I like the idea that boilerplate "extern"s can be removed ;-) But I think at least we want the helpers to have different namespaces with other bindings, consider the following case:

First we implement a helper named:

bool rust_helper_do_something()

and use it as

if unsafe { bindings::do_something() } {
    <xxx>
}

Then some one, who is not aware of our implementation and usage, also implement a do_something in kernel:

bool do_something() 
{
    ...
}

EXPORT_SYM(do_domething);

And it will be preferred by Rust code, so here comes the first problem, what if the C version of do_something works slightly differently? The difference may be not detectable by our existing tests yet still introduce bugs (e.g. break our SAFETY assumptions).

And there is another problem, if both Rust side code and C side code got evolved a bit around do_something, and suddenly C version of do_something got removed, and we switched back to rust_helper_do_something, we might have a hidden bug again because the difference of two functions may not be detectable to tests yet cause bugs.

Thoughts?

[bjorn3]

Helpers are normally named after the macro they call. If a macro and a C function have the same name, that gives problems anyway as the macro shadows the function amd if you include the header with the macro before the header with the function, you get a syntax error or renamed function.

[nbdd0121]

I would consider it very problematic and confusing if a rust_helper_foo is functionally different from foo. rust_helper_foo should only be used to invoke macros or inline functions that are not otherwise exposed via bindgen.

It's a deliberate choice to use the same namespace, so that the Rust users do not have to care whether the function is directly exposed or exposed via a helper. Some functions like errname can be either a extern function or an static inline function depending on configs.

[fbq]

Helpers are normally named after the macro they call. If a macro and a C function have the same name, that gives problems anyway as the macro shadows the function amd if you include the header with the macro before the header with the function, you get a syntax error or renamed function.

Well, you said "normally", so there are exceptions, and the thing is, we don't have a way to detect the exception (at least I don't see one in this PR). Luckily the only exception seems to be rust_helper_refcount_new currently.

Having an undetectable problem is dangerous, and it's even more dangerous when the problem is normally rare.

[fbq]

I would consider it very problematic and confusing if a rust_helper_foo is functionally different from foo. rust_helper_foo should only be used to invoke macros or inline functions that are not otherwise exposed via bindgen.

As long as you can guarantee that rust_helper_foo always call foo directly, then I don't have any problem with this.

Now think about this, maybe we can try this if rust_helper_foo doesn't call foo directly: we just define foo in rust/helpers.c.

For example, instead of defining rust_helper_refcount_new, we just define refcount_new. This is kinda like taking the ownership of the name, if the refcount maintainers want to use that name later, they will find the conflicts and reach out to us, and we can just give it up and adjust if the semantics of refcount_new changes.

This means in rust/helpers, we either:

• User a C macro to generate rust_helper_foo with calling foo as the function body, or
• Define a foo to take the ownership of the name.

I think it's a simple and practical rule to follow when we add things in rust/helpers.c, and rules out some potential issues because of sharing namespace with normal bindings.

Thoughts?

It's a deliberate choice to use the same namespace, so that the Rust users do not have to care whether the function is directly exposed or exposed via a helper. Some functions like errname can be either a extern function or an static inline function depending on configs.

[nbdd0121]

This means in rust/helpers, we either:

• User a C macro to generate rust_helper_foo with calling foo as the function body, or
• Define a foo to take the ownership of the name.

I think it's a simple and practical rule to follow when we add things in rust/helpers.c, and rules out some potential issues because of sharing namespace with normal bindings.

Thoughts?

Sounds very reasonable to me.

[nbdd0121]

Rebased, and changed rust_helper_refcount_new to rust_helper_REFCOUNT_INIT to address @fbq's concern for now.

[ojeda]

Solving the conflicts and trying this one, we get a warning about a clashing BUG() due to the panic handler being now in the kernel crate.

bindgen does not seem to understand _Noreturn yet, opened: rust-lang/rust-bindgen#2094.

[ojeda]

Perhaps we should have "manual helpers" for bits like this, something like:

diff --git a/rust/helpers.c b/rust/helpers.c
index b74c8991fb9..a6ef4cd02e0 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -13,7 +13,9 @@
 #include <linux/security.h>
 #include <asm/io.h>
 
-void rust_helper_BUG(void)
+// Manual until `bindgen` supports `__noreturn`
+// https://github.com/rust-lang/rust-bindgen/issues/2094
+__noreturn void rust_manual_helper_BUG(void)
 {
        BUG();
 }
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c6bf9043afb..a95c4f57e8c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -233,9 +233,9 @@ macro_rules! container_of {
 #[panic_handler]
 fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
     extern "C" {
-        fn rust_helper_BUG() -> !;
+        fn rust_manual_helper_BUG() -> !;
     }
     pr_emerg!("{}\n", info);
     // SAFETY: FFI call.
-    unsafe { rust_helper_BUG() };
+    unsafe { rust_manual_helper_BUG() };
 }

[nbdd0121]

Rebased and fixed conflicts.

I think given that there aren't many noreturn ffi functions, we can just live with it for now. I've added a loop {} underneath bindings::BUG() call.

[ojeda]

I think given that there aren't many noreturn ffi functions, we can just live with it for now. I've added a loop {} underneath bindings::BUG() call.

I think we crossed messages -- see my previous one.

Please add the __noreturn on the helper and the comment on top. Regarding the loop {} or the manual helper, I think both approaches are fine. The manual helper may generate one less instruction, but...

[nbdd0121]

Ok. I keep the loop{} approach and thus add the comment to the call site instead.

One single additional instruction on the perhaps coldest code path.. I am not worried. It could be removed with unsafe { unreachable_unchecked() } but I think the gain doesn't warrant an additional unsafe...

[nbdd0121]

I think the change exposes a bug in IoMem. readq isn't gated on CONFIG_64BIT.