Investigate why no GlobalAlloc-related symbols are generated

[ojeda]

When compiling the crates as staticlib, the compiler generates the symbols based on GlobalAlloc; but it doesn't when asking for an rlib.

I assume it does it only when it needs to link a "final" product (executable, static library, etc.), but I haven't look how it actually works in rustc yet. I think it is reasonable to generate them in an rlib which overrides the allocator, but maybe they have a reason not to...

[bjorn3]

rust-lang/rust#86844 just landed. With this PR all __rust_* alloc functions are defined directly by the #[global_allocator] expansion, so they no longer have to be defined manually. It is now required to define a static with the name __rust_no_alloc_shim_is_unstable though to indicate that this use case is not yet stable.

[bjorn3]

I've got a patch for the changes in rust-lang/rust#86844. Should I send it as RFC patch given that we don't yet use rustc 1.71.0 and it needs to be included in the patch set to update to rustc 1.71.0?

From 47729005596e395ad5981fb06b30a0f41e6198e0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Roy=20Baron?= <[email protected]>
Date: Thu, 22 Jun 2023 17:00:38 +0200
Subject: [PATCH] Rework global allocator definition for rustc 1.71.0
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Rustc 1.71.0 changed the way the allocator shim works. [1] Instead of
having to define a bunch of methods whose existence is unstable, it is
now only necessary to define the methods on the stable GlobalAlloc trait
impl for KernelAllocator and a single unstable static. In the future it
may be possible that the whole method will be stabilized.

Link: https://github.com/rust-lang/rust/pull/86844 [1]
Signed-off-by: Björn Roy Baron <[email protected]>
---
 rust/kernel/allocator.rs | 65 +++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index 397a3dd57a9b..6f04f8389850 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -21,44 +21,41 @@ unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
             bindings::kfree(ptr as *const core::ffi::c_void);
         }
     }
+
+    unsafe fn realloc(&self, ptr: *mut u8, _layout: Layout, new_size: usize) -> *mut u8 {
+        unsafe {
+            bindings::krealloc(
+                ptr as *const core::ffi::c_void,
+                new_size,
+                bindings::GFP_KERNEL,
+            ) as *mut u8
+        }
+    }
+
+    unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+        unsafe {
+            bindings::krealloc(
+                core::ptr::null(),
+                layout.size(),
+                bindings::GFP_KERNEL | bindings::__GFP_ZERO,
+            ) as *mut u8
+        }
+    }
 }
 
 #[global_allocator]
 static ALLOCATOR: KernelAllocator = KernelAllocator;
 
-// `rustc` only generates these for some crate types. Even then, we would need
-// to extract the object file that has them from the archive. For the moment,
-// let's generate them ourselves instead.
+// For dispatching allocation requests to #[global_allocator] or the default
+// allocator in libstd rust uses a so called allocator shim, which is an
+// object file containing a couple of functions with a fixed name that call
+// either #[global_allocator] or the default allocator in libstd. `rustc`
+// only generates this allocator shim when rustc is invoking the linker.
 //
-// Note that `#[no_mangle]` implies exported too, nowadays.
-#[no_mangle]
-fn __rust_alloc(size: usize, _align: usize) -> *mut u8 {
-    unsafe { bindings::krealloc(core::ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 }
-}
-
-#[no_mangle]
-fn __rust_dealloc(ptr: *mut u8, _size: usize, _align: usize) {
-    unsafe { bindings::kfree(ptr as *const core::ffi::c_void) };
-}
-
-#[no_mangle]
-fn __rust_realloc(ptr: *mut u8, _old_size: usize, _align: usize, new_size: usize) -> *mut u8 {
-    unsafe {
-        bindings::krealloc(
-            ptr as *const core::ffi::c_void,
-            new_size,
-            bindings::GFP_KERNEL,
-        ) as *mut u8
-    }
-}
-
+// Since recently this allocator shim is no longer necessary for
+// #[global_allocator] however. Instead #[global_allocator] directly defines
+// functions with the right names. For now this has not been made a guarantee
+// however just like the old allocator shim wasn't. As such we need to define
+// this static to acknowledge that it may break in the future.
 #[no_mangle]
-fn __rust_alloc_zeroed(size: usize, _align: usize) -> *mut u8 {
-    unsafe {
-        bindings::krealloc(
-            core::ptr::null(),
-            size,
-            bindings::GFP_KERNEL | bindings::__GFP_ZERO,
-        ) as *mut u8
-    }
-}
+static __rust_no_alloc_shim_is_unstable: u8 = 0;
-- 
2.39.2

[ojeda]

Sounds good to me. If you think this should be the actual patch to be (eventually) applied, then I would skip the RFC tag. Instead, what you can do is write after the --- line that this is intended for the Rust 1.71.0 upgrade, e.g. something like:

...
Link: https://github.com/rust-lang/rust/pull/86844 [1]
Signed-off-by: Björn Roy Baron <[email protected]>
---
This patch is meant to be included in the Rust 1.71.0 upgrade.

 rust/kernel/allocator.rs | 65 +++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 34 deletions(-)
...

That text will not be included in the commit message when applied.

By the way, I assume this need to be applied at the same time as the upgrade itself, i.e. otherwise it breaks the build, right? If so (i.e. if I have to do everything at once in a single commit), should I put you as Co-developed-by? (I will copy your commit message too in a section of the patch in that case).

Also, the other day I was trying a Rust 1.72.0 nightly to confirm the fix of the .eh_frame section, and I had to change this too, but I don't recall having to add the __rust_no_alloc_shim_is_unstable static. Is that because it was a nightly, while the static is needed for stable (even with RUSTC_BOOTSTRAP=1)?

Thanks @bjorn3!

[bjorn3]

Yes, it has to be done at the same time as the rustc update and either at the same time as or before the liballoc update. __rust_no_alloc_shim_is_unstable is only necessary when you update liballoc as only the liballoc of rustc 1.71 and later references it. If you kept using the liballoc of rustc 1.70 it would still work fine without.

[bjorn3]

If so (i.e. if I have to do everything at once in a single commit), should I put you as Co-developed-by? (I will copy your commit message too in a section of the patch in that case).

Sure

[nbdd0121]

I think we should apply the realloc/alloc_zeroed part first, and the rest can go with 1.71 update.

[bjorn3]

Makes sense. Will work on a patch for that.

[ojeda]

__rust_no_alloc_shim_is_unstable is only necessary when you update liballoc as only the liballoc of rustc 1.71 and later references it. If you kept using the liballoc of rustc 1.70 it would still work fine without.

Ah, that explains it, thanks!

[bjorn3]

Would this work?

From 2befb15fff5d33f8f0e254007e84aa066b227b8a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Roy=20Baron?= <[email protected]>
Date: Thu, 22 Jun 2023 19:09:04 +0200
Subject: [PATCH] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

While there are default impls for these methods, using the respective C
api's is faster. Currently neither the existing nor these new
GlobalAlloc method implementations are actually called. Instead the
__rust_* function defined below the GlobalAlloc impl are used. With
rustc 1.71 these functions will be gone and all allocation calls will go
through the GlobalAlloc implementation.

Link: https://github.com/Rust-for-Linux/linux/issues/68
Signed-off-by: Björn Roy Baron <[email protected]>
---
 rust/kernel/allocator.rs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index 397a3dd57a9b..e0a27b1326b5 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -21,6 +21,26 @@ unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
             bindings::kfree(ptr as *const core::ffi::c_void);
         }
     }
+
+    unsafe fn realloc(&self, ptr: *mut u8, _layout: Layout, new_size: usize) -> *mut u8 {
+        unsafe {
+            bindings::krealloc(
+                ptr as *const core::ffi::c_void,
+                new_size,
+                bindings::GFP_KERNEL,
+            ) as *mut u8
+        }
+    }
+
+    unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+        unsafe {
+            bindings::krealloc(
+                core::ptr::null(),
+                layout.size(),
+                bindings::GFP_KERNEL | bindings::__GFP_ZERO,
+            ) as *mut u8
+        }
+    }
 }
 
 #[global_allocator]
-- 
2.39.2

[ojeda]

https://lore.kernel.org/rust-for-linux/[email protected]/

[ojeda]

The upgrade to 1.71 removes the functions involved since they are now generated as expected: https://lore.kernel.org/rust-for-linux/[email protected]/#iZ31rust:kernel:allocator.rs.

Thanks @bjorn3 for fixing this in the compiler!

(I will also add a Closes: tag to the commit to this issue when I apply it, by the way.)