From 04b8a0cb3e1e133b01f3c3cd29f2605dc9d23163 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Tue, 26 Nov 2024 16:18:01 +0000 Subject: [PATCH 1/5] lisa._assets.kmodules.lisa: Improve Rust bindings --- lisa/_assets/kmodules/lisa/Makefile | 2 +- .../lisa/rust/lisakmod-macros/Cargo.lock | 7 + .../lisa/rust/lisakmod-macros/Cargo.toml | 1 + .../lisa/rust/lisakmod-macros/src/inlinec.rs | 371 ++++++++++++------ .../rust/lisakmod-macros/src/private/mod.rs | 1 + .../kmodules/lisa/rust/lisakmod/Cargo.lock | 7 + .../lisa/rust/lisakmod/src/prelude.rs | 2 +- .../lisa/rust/lisakmod/src/runtime/alloc.rs | 3 +- .../lisa/rust/lisakmod/src/runtime/sysfs.rs | 206 ++++++---- .../kmodules/lisa/rust/lisakmod/src/tests.rs | 12 +- lisa/_kmod.py | 2 +- 11 files changed, 421 insertions(+), 193 deletions(-) diff --git a/lisa/_assets/kmodules/lisa/Makefile b/lisa/_assets/kmodules/lisa/Makefile index 5d53e9d94..a59eb5b98 100644 --- a/lisa/_assets/kmodules/lisa/Makefile +++ b/lisa/_assets/kmodules/lisa/Makefile @@ -69,7 +69,7 @@ $(GENERATED): clean-files := $(GENERATED) RUST_VERSION ?= nightly -RUSTFLAGS := -Clinker=rust-lld -Clink-self-contained=y -Clto=n -Crelocation-model=static -Cno-redzone=y -Csoft-float=y -Ctarget-cpu=generic -Cforce-frame-pointers=y -Ccodegen-units=1 -Zfunction-sections=y +RUSTFLAGS := -Clinker=rust-lld -Clink-self-contained=y -Clto=n -Crelocation-model=static -Cno-redzone=y -Ctarget-cpu=generic -Cforce-frame-pointers=y -Ccodegen-units=1 -Zfunction-sections=y LISA_EVAL_C := $(MODULE_SRC)/lisa-eval-c.sh diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.lock b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.lock index 23e920bb5..952426828 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.lock +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.lock @@ -30,6 +30,7 @@ name = "lisakmod_macros" version = "0.1.0" dependencies = [ "lisakmod_macros_proc", + "paste", ] [[package]] @@ -42,6 +43,12 @@ dependencies = [ "syn", ] +[[package]] +name = "paste" +version = "1.0.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" + [[package]] name = "proc-macro2" version = "1.0.92" diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.toml b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.toml index 106b02dab..9989feeae 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.toml +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/Cargo.toml @@ -5,3 +5,4 @@ edition = "2021" [dependencies] lisakmod_macros_proc = { path = "./macros" } +paste = "1.0" diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/inlinec.rs b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/inlinec.rs index 5ede5f376..0cbc51239 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/inlinec.rs +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/inlinec.rs @@ -36,6 +36,78 @@ pub trait IntoFfi: FfiType { fn into_ffi(self) -> Self::FfiType; } +pub trait IntoPtr { + fn into_ptr(self) -> Ptr; +} + +pub trait FromPtr { + fn from_ptr(ptr: Ptr) -> Self; +} + +macro_rules! impl_ptr { + ($ptr:ty, $ptr2:ty, $ref:ty) => { + impl IntoPtr<$ptr> for $ptr { + #[inline] + fn into_ptr(self) -> $ptr { + self + } + } + + impl<'a, T: ?Sized> IntoPtr<*const $ref> for *const $ptr { + #[inline] + fn into_ptr(self) -> *const $ref { + self as _ + } + } + + impl<'a, T: ?Sized> IntoPtr<*mut $ref> for *mut $ptr { + #[inline] + fn into_ptr(self) -> *mut $ref { + self as _ + } + } + + impl FromPtr<$ptr> for $ptr { + #[inline] + fn from_ptr(ptr: $ptr) -> Self { + ptr + } + } + + impl<'a, T: ?Sized> FromPtr<*const $ref> for *const $ptr + { + #[inline] + fn from_ptr(ptr: *const $ref) -> Self { + ptr as _ + } + } + + impl<'a, T: ?Sized> FromPtr<*mut $ref> for *mut $ptr { + #[inline] + fn from_ptr(ptr: *mut $ref) -> Self { + ptr as _ + } + } + + impl_ptr!(@nested, ConstPtr<$ptr2>, ConstPtr<$ref>); + impl_ptr!(@nested, MutPtr<$ptr2>, MutPtr<$ref>); + + }; + (@nested, $nested_ptr:ty, $nested_ref:ty) => { + impl<'a, T> FfiType for $nested_ref + where + // We only implement for Sized types, so that unsized types can encode their metadata in custom + // ways, like [T] + T: Sized, + $nested_ptr: FfiType, + { + const C_DECL: &'static str = <$nested_ptr as FfiType>::C_DECL; + type FfiType = <$nested_ptr as FfiType>::FfiType; + } + + } +} + /// [*const T] newtype. /// /// [FfiType], [FromFfi] and [IntoFfi] implementations on [ConstPtr] are expected to be provided @@ -45,6 +117,7 @@ pub trait IntoFfi: FfiType { /// * [&'a T] #[fundamental] pub struct ConstPtr(*const T); +impl_ptr!(*const T, ConstPtr, &'a T); impl From> for *const T { #[inline] @@ -53,23 +126,29 @@ impl From> for *const T { } } -impl FromFfi for ConstPtr +impl FromFfi for ConstPtr where - ConstPtr: FfiType, + // Only implement for Sized so that dynamically sized types (DST) like [T] can have their own + // representation that can encode that size + T: Sized, + ConstPtr: FfiType>, { #[inline] unsafe fn from_ffi(x: Self::FfiType) -> Self { - ConstPtr(x) + ConstPtr(x.into_ptr()) } } -impl IntoFfi for ConstPtr +impl IntoFfi for ConstPtr where - ConstPtr: FfiType, + // Only implement for Sized so that dynamically sized types (DST) like [T] can have their own + // representation that can encode that size + T: Sized, + ConstPtr: FfiType>, { #[inline] fn into_ffi(self) -> Self::FfiType { - self.0 + >::from_ptr(self.0) } } @@ -84,6 +163,7 @@ where /// * [Option>] #[fundamental] pub struct MutPtr(*mut T); +impl_ptr!(*mut T, MutPtr, &'a mut T); impl From> for *mut T { #[inline] @@ -94,31 +174,34 @@ impl From> for *mut T { impl FromFfi for MutPtr where - T: ?Sized, - MutPtr: FfiType, + // Only implement for Sized so that dynamically sized types (DST) like [T] can have their own + // representation that can encode that size + T: Sized, + MutPtr: FfiType>, { #[inline] unsafe fn from_ffi(x: Self::FfiType) -> Self { - MutPtr(x) + MutPtr(x.into_ptr()) } } impl IntoFfi for MutPtr where - T: ?Sized, - MutPtr: FfiType, + // Only implement for Sized so that dynamically sized types (DST) like [T] can have their own + // representation that can encode that size + T: Sized, + MutPtr: FfiType>, { #[inline] fn into_ffi(self) -> Self::FfiType { - self.0 + >::from_ptr(self.0) } } -// The user-provided "root" implementation is for &T and &mut T. Everything is provided from there. -// This is because &T and &mut T are fundamental (as in #[fundamental]), meaning that even if the -// "reference of" generic type is not provided by the crate (since it's a primitive), the user is -// free to implement traits for it despite the regular orphan rule. *T, *mut T, NonNull etc do -// not enjoy such treatment, making it impossible to provide implementation for foreign traits. +// Since it is not possible for the user to write implementations for *const T and *mut T directly, +// they provide impl for ConstPtr and MutPtr newtypes and we just have a blanket +// implementation for the real pointer type. + impl FfiType for *const T where T: ?Sized, @@ -151,28 +234,6 @@ where } } -// These implementations allow arbitrary nesting of pointers, as they provide the definition for a -// pointer to pointer. Unfortunately, we cannot provide a valid generic C_DECL for that, so we -// currently cannot express this. - -// impl FfiType for ConstPtr> -// where -// T: ?Sized, -// ConstPtr: FfiType, -// { -// const C_DECL: &'static str = as FfiType>::C_DECL; -// type FfiType = *const as FfiType>::FfiType; -// } - -// impl FfiType for ConstPtr> -// where -// T: ?Sized, -// MutPtr: FfiType, -// { -// const C_DECL: &'static str = as FfiType>::C_DECL; -// type FfiType = *const as FfiType>::FfiType; -// } - impl FfiType for ConstPtr<*const T> where T: ?Sized, @@ -212,6 +273,17 @@ where } } +impl IntoFfi for *mut T +where + T: ?Sized, + MutPtr: FfiType + IntoFfi, +{ + #[inline] + fn into_ffi(self) -> Self::FfiType { + MutPtr(self).into_ffi() + } +} + impl FfiType for MutPtr<*const T> where T: ?Sized, @@ -230,17 +302,6 @@ where type FfiType = > as FfiType>::FfiType; } -impl IntoFfi for *mut T -where - T: ?Sized, - MutPtr: FfiType + IntoFfi, -{ - #[inline] - fn into_ffi(self) -> Self::FfiType { - MutPtr(self).into_ffi() - } -} - trait PtrToMaybeSized { #[inline] fn is_aligned(&self) -> Option { @@ -272,20 +333,21 @@ impl PtrToMaybeSized for *mut T { impl FfiType for &T where T: ?Sized, - *const T: FfiType + PtrToMaybeSized, + *const T: PtrToMaybeSized, + ConstPtr: FfiType, { - const C_DECL: &'static str = <*const T as FfiType>::C_DECL; - type FfiType = <*const T as FfiType>::FfiType; + const C_DECL: &'static str = as FfiType>::C_DECL; + type FfiType = as FfiType>::FfiType; } impl FromFfi for &T where T: ?Sized, - *const T: FfiType + FromFfi, + ConstPtr: FfiType + FromFfi, { #[inline] unsafe fn from_ffi(x: Self::FfiType) -> Self { - let ptr = <*const T as FromFfi>::from_ffi(x); + let ptr = as FromFfi>::from_ffi(x).0; assert!(PtrToMaybeSized::is_aligned(&ptr).unwrap_or(true)); ptr.as_ref().expect("Unexpected NULL pointer") } @@ -294,7 +356,7 @@ where impl IntoFfi for &T where T: ?Sized, - *const T: FfiType + IntoFfi, + ConstPtr: FfiType + IntoFfi, { #[inline] fn into_ffi(self) -> Self::FfiType { @@ -305,20 +367,21 @@ where impl FfiType for &mut T where T: ?Sized, - *mut T: FfiType, + MutPtr: FfiType, { - const C_DECL: &'static str = <*mut T as FfiType>::C_DECL; - type FfiType = <*mut T as FfiType>::FfiType; + const C_DECL: &'static str = as FfiType>::C_DECL; + type FfiType = as FfiType>::FfiType; } impl FromFfi for &mut T where T: ?Sized, - *mut T: FfiType + FromFfi + PtrToMaybeSized, + *mut T: PtrToMaybeSized, + MutPtr: FfiType + FromFfi, { #[inline] unsafe fn from_ffi(x: Self::FfiType) -> Self { - let ptr = <*mut T as FromFfi>::from_ffi(x); + let ptr = as FromFfi>::from_ffi(x).0; assert!(PtrToMaybeSized::is_aligned(&ptr).unwrap_or(true)); ptr.as_mut().expect("Unexpected NULL pointer") } @@ -327,7 +390,7 @@ where impl IntoFfi for &mut T where T: ?Sized, - *mut T: FfiType + IntoFfi, + MutPtr: FfiType + IntoFfi, { #[inline] fn into_ffi(self) -> Self::FfiType { @@ -338,12 +401,12 @@ where impl FfiType for ConstPtr> where T: ?Sized, - *const T: FfiType, - *mut T: FfiType, + ConstPtr: FfiType, + MutPtr: FfiType, { // Expose as a mutable pointer for C code, since the whole point of UnsafeCell is to allow // mutation of T from a &UnsafeCell. - const C_DECL: &'static str = <*mut T as FfiType>::C_DECL; + const C_DECL: &'static str = as FfiType>::C_DECL; // Expose the pointer as *const for the FFI functions so that the signatures are compatible // with the other blanket implementations. This will effectively transmute the *const // UnsafeCell into *mut T in the IntoFfi implementation at the FFI boundary. @@ -353,20 +416,42 @@ where impl FfiType for MutPtr> where T: ?Sized, - *mut T: FfiType, + MutPtr: FfiType, { - const C_DECL: &'static str = <*mut T as FfiType>::C_DECL; + const C_DECL: &'static str = as FfiType>::C_DECL; type FfiType = *mut UnsafeCell; } -impl FfiType for Pin -where - T: FfiType, -{ - const C_DECL: &'static str = ::C_DECL; - type FfiType = ::FfiType; +macro_rules! impl_transparent_wrapper { + ($wrapper:ty) => { + impl FfiType for $wrapper + where + T: FfiType, + { + const C_DECL: &'static str = ::C_DECL; + type FfiType = ::FfiType; + } + + impl FfiType for ConstPtr<$wrapper> + where + ConstPtr: FfiType, + { + const C_DECL: &'static str = as FfiType>::C_DECL; + type FfiType = *const $wrapper; + } + + impl FfiType for MutPtr<$wrapper> + where + MutPtr: FfiType, + { + const C_DECL: &'static str = as FfiType>::C_DECL; + type FfiType = *mut $wrapper; + } + }; } +impl_transparent_wrapper!(Pin); + impl FromFfi for Pin where T: FromFfi + Deref, @@ -394,26 +479,10 @@ where } } -impl FfiType for ConstPtr> -where - *const T: FfiType, -{ - const C_DECL: &'static str = <*const T as FfiType>::C_DECL; - type FfiType = *const Pin; -} - -impl FfiType for MutPtr> -where - *mut T: FfiType, -{ - const C_DECL: &'static str = <*mut T as FfiType>::C_DECL; - type FfiType = *mut Pin; -} - #[macro_export] -macro_rules! __internal_ptr_cffi { +macro_rules! __impl_primitive_ptr { ($pointee:ty, $c_pointee:literal) => { - $crate::inlinec::__internal_ptr_cffi!( + $crate::inlinec::__impl_primitive_ptr!( @impl, $pointee, $pointee, $c_pointee, "", "" ); @@ -422,14 +491,14 @@ macro_rules! __internal_ptr_cffi { // implementation for ConstPtr>, because we cannot express the resulting C_DECL // (lack of const function in traits). We therefore unroll 2 level of pointers, since we // don't really need more in practice. - $crate::inlinec::__internal_ptr_cffi!( + $crate::inlinec::__impl_primitive_ptr!( @impl, $crate::inlinec::ConstPtr<$pointee>, <$crate::inlinec::ConstPtr<$pointee> as $crate::inlinec::FfiType>::FfiType, $c_pointee, "CONST_TY_DECL(PTR_TY_DECL(", "))" ); - $crate::inlinec::__internal_ptr_cffi!( + $crate::inlinec::__impl_primitive_ptr!( @impl, $crate::inlinec::MutPtr<$pointee>, <$crate::inlinec::MutPtr<$pointee> as $crate::inlinec::FfiType>::FfiType, @@ -453,9 +522,9 @@ macro_rules! __internal_ptr_cffi { } } } -pub use crate::__internal_ptr_cffi; +pub use crate::__impl_primitive_ptr; -macro_rules! transparent_cffi { +macro_rules! impl_primitive { ($ty:ty, $c_name:literal) => { impl FfiType for $ty { const C_DECL: &'static str = @@ -477,23 +546,23 @@ macro_rules! transparent_cffi { } } - __internal_ptr_cffi!($ty, $c_name); + __impl_primitive_ptr!($ty, $c_name); }; } -transparent_cffi!(u8, "uint8_t"); -transparent_cffi!(u16, "uint16_t"); -transparent_cffi!(u32, "uint32_t"); -transparent_cffi!(u64, "uint64_t"); -transparent_cffi!(usize, "size_t"); +impl_primitive!(u8, "uint8_t"); +impl_primitive!(u16, "uint16_t"); +impl_primitive!(u32, "uint32_t"); +impl_primitive!(u64, "uint64_t"); +impl_primitive!(usize, "size_t"); -transparent_cffi!(i8, "int8_t"); -transparent_cffi!(i16, "int16_t"); -transparent_cffi!(i32, "int32_t"); -transparent_cffi!(i64, "int64_t"); -transparent_cffi!(isize, "ssize_t"); +impl_primitive!(i8, "int8_t"); +impl_primitive!(i16, "int16_t"); +impl_primitive!(i32, "int32_t"); +impl_primitive!(i64, "int64_t"); +impl_primitive!(isize, "ssize_t"); -transparent_cffi!(bool, "_Bool"); +impl_primitive!(bool, "_Bool"); // This is used for function returning void exclusively. We never pass a void parameter to a // function. @@ -519,7 +588,7 @@ impl FfiType for c_void { type FfiType = c_void; } // Only implement FromFfi/IntoFfi for pointers to c_void, never for c_void itself -__internal_ptr_cffi!(c_void, "void"); +__impl_primitive_ptr!(c_void, "void"); pub trait NullPtr { fn null_mut() -> *mut Self; @@ -554,7 +623,7 @@ where impl IntoFfi for Option> where - T: NullPtr, + T: ?Sized + NullPtr, MutPtr: FfiType + IntoFfi, { #[inline] @@ -722,16 +791,24 @@ where len: usize, } -impl FfiType for ConstPtr<[u8]> { - const C_DECL: &'static str = "BUILTIN_TY_DECL(struct slice_const_u8, DECLARATOR)"; - type FfiType = FfiSlice<*const u8>; -} +macro_rules! impl_slice { + ($ty:ty, $c_name_const:literal, $c_name_mut:literal) => { + impl FfiType for ConstPtr<[$ty]> { + const C_DECL: &'static str = + $crate::misc::concatcp!("BUILTIN_TY_DECL(", $c_name_const, ", DECLARATOR)"); + type FfiType = FfiSlice<*const $ty>; + } -impl FfiType for MutPtr<[u8]> { - const C_DECL: &'static str = "BUILTIN_TY_DECL(struct slice_u8, DECLARATOR)"; - type FfiType = FfiSlice<*mut u8>; + impl FfiType for MutPtr<[$ty]> { + const C_DECL: &'static str = + $crate::misc::concatcp!("BUILTIN_TY_DECL(", $c_name_mut, ", DECLARATOR)"); + type FfiType = FfiSlice<*mut $ty>; + } + }; } +impl_slice!(u8, "struct slice_const_u8", "struct slice_u8"); + impl IntoFfi for ConstPtr<[T]> where ConstPtr<[T]>: FfiType>, @@ -906,7 +983,7 @@ pub trait Opaque { #[macro_export] macro_rules! __internal_opaque_type { - ($vis:vis struct $name:ident, $c_name:literal, $c_header:literal) => { + ($vis:vis struct $name:ident, $c_name:literal, $c_header:literal $(, $($opt_name:ident {$($opt:tt)*}),* $(,)?)?) => { // Model opaque types as recommended in the Rustonomicon: // https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs // On top of that recipe, we add: @@ -945,6 +1022,10 @@ macro_rules! __internal_opaque_type { _marker: ::core::marker::PhantomData<(*mut u8, ::core::marker::PhantomPinned)>, } + $($( + $crate::inlinec::opaque_type!(@opt $opt_name, $name, $c_header, $($opt)*); + )*)? + // Double check that the we did not fumble the Rust struct layout somehow. const _:() = { const fn member_layout &B>(f: F) -> (usize, usize) { @@ -967,7 +1048,7 @@ macro_rules! __internal_opaque_type { ); }; - $crate::inlinec::__internal_ptr_cffi!($name, $c_name); + $crate::inlinec::__impl_primitive_ptr!($name, $c_name); use $crate::inlinec::Opaque as _; impl $crate::inlinec::Opaque for $name {} @@ -989,6 +1070,60 @@ macro_rules! __internal_opaque_type { } } }; + (@opt attr_by_ref, $name:ident, $c_header:expr, fn $attr:ident(&self) -> &$attr_ty:ty) => { + impl $name { + #[inline] + fn $attr(&self) -> &$attr_ty { + #[$crate::inlinec::cfunc] + fn get(this: &$name) -> &$attr_ty { + $crate::misc::concatcp!( + "#include <", $c_header, ">\n" + ); + + $crate::misc::concatcp!( + "return &this->", ::core::stringify!($attr), ";" + ) + } + get(self) + } + } + }; + (@opt attr_by_mut, $name:ident, $c_header:expr, fn $attr:ident(&mut self) -> &mut $attr_ty:ty) => { + impl $name { + #[inline] + fn $attr(&mut self) -> &mut $attr_ty { + #[$crate::inlinec::cfunc] + fn get(this: &mut $name) -> &mut $attr_ty { + $crate::misc::concatcp!( + "#include <", $c_header, ">\n" + ); + + $crate::misc::concatcp!( + "return &this->", ::core::stringify!($attr), ";" + ) + } + get(self) + } + } + }; + (@opt attr_by_value, $name:ident, $c_header:expr, fn $attr:ident(&self) -> $attr_ty:ty) => { + impl $name { + #[inline] + fn $attr(&self) -> $attr_ty { + #[$crate::inlinec::cfunc] + fn get(this: &$name) -> $attr_ty { + $crate::misc::concatcp!( + "#include <", $c_header, ">\n" + ); + + $crate::misc::concatcp!( + "return this->", ::core::stringify!($attr), ";" + ) + } + get(self) + } + } + }; } // Since the macro is tagged with #[macro_export], it will be exposed in the crate namespace // directly for public use. We then re-export it from here under its pretty name, so that it is diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/private/mod.rs b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/private/mod.rs index c19387e1d..fef04ff1e 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/private/mod.rs +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod-macros/src/private/mod.rs @@ -1,2 +1,3 @@ /* SPDX-License-Identifier: Apache-2.0 */ pub mod misc; +pub use paste; diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod/Cargo.lock b/lisa/_assets/kmodules/lisa/rust/lisakmod/Cargo.lock index 9f12d666d..08322245e 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod/Cargo.lock +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod/Cargo.lock @@ -37,6 +37,7 @@ name = "lisakmod_macros" version = "0.1.0" dependencies = [ "lisakmod_macros_proc", + "paste", ] [[package]] @@ -49,6 +50,12 @@ dependencies = [ "syn", ] +[[package]] +name = "paste" +version = "1.0.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" + [[package]] name = "proc-macro2" version = "1.0.92" diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/prelude.rs b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/prelude.rs index 058c2b1d8..f7ae663e5 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/prelude.rs +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/prelude.rs @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0 */ pub use crate::{ - inlinec::{cexport, cfunc, cstatic, cconstant}, + inlinec::{cconstant, cexport, cfunc, cstatic}, runtime::kbox::KBox, }; diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/alloc.rs b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/alloc.rs index bdd142e55..c053c57c6 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/alloc.rs +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/alloc.rs @@ -15,7 +15,8 @@ struct KernelAllocator; #[inline] fn with_size *mut u8>(layout: Layout, f: F) -> *mut u8 { - let minalign: usize = cconstant!("#include ", "ARCH_KMALLOC_MINALIGN").unwrap_or(1); + let minalign: usize = + cconstant!("#include ", "ARCH_KMALLOC_MINALIGN").unwrap_or(1); let size = layout.size(); let align = layout.align(); diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/sysfs.rs b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/sysfs.rs index 79ec73f10..ec8eb8dde 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/sysfs.rs +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/runtime/sysfs.rs @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 */ -use alloc::{boxed::Box, sync::Arc}; +use alloc::{boxed::Box, string::String, sync::Arc}; use core::{ cell::UnsafeCell, convert::Infallible, @@ -15,7 +15,13 @@ use crate::{ {container_of, mut_container_of}, }; -opaque_type!(struct CKObj, "struct kobject", "linux/kobject.h"); +opaque_type!( + struct CKObj, + "struct kobject", + "linux/kobject.h", + attr_by_value {fn state_in_sysfs(&self) -> bool}, + attr_by_value {fn ktype(&self) -> &CKObjType}, +); opaque_type!(struct CKObjType, "struct kobj_type", "linux/kobject.h"); // SAFETY: CKObjType is a plain-old-data struct, there is nothing in there that can't be shared @@ -83,10 +89,12 @@ impl KObjType { } } +// SAFETY: repr(transparent) is relied on to convert a *const CKObj into a *const KObjectInner. #[repr(transparent)] struct KObjectInner { c_kobj: UnsafeCell, - // CKObj contains a pointer to CKObjType, so we reflect that dependency here. + // CKObj contains a pointer to CKObjType, so we reflect that dependency here. This is probably + // not very useful. _phantom: PhantomData, } @@ -95,16 +103,6 @@ struct KObjectInner { unsafe impl Send for KObjectInner {} unsafe impl Sync for KObjectInner {} -const _: () = { - // This is relied on in order to be able transmute a CKObj pointer to a KObjectInner pointer so - // we can manipulate foreign kobjects, such as the sysfs root folder for the module in - // /sys/module/lisa/ - // - // If that assertion is not true anymore because we need to store metadata in KObjectInner, we - // would need to add a level of indirection. - assert!(core::mem::size_of::() == core::mem::size_of::()); -}; - impl KObjectInner { pub fn new(kobj_type: Arc) -> AllocatedKObjectInner { #[cfunc] @@ -145,7 +143,31 @@ impl KObjectInner { Box::into_pin(unsafe { new.assume_init() }) } + #[inline] + fn with_c_kobj(&self, f: F) -> T + where + F: for<'a> FnOnce(&'a CKObj) -> T, + { + // SAFETY: The reference passed to f() cannot be leaked by f() since it has a rank-2 type + // (HRTB). The closure has to work for _any_ lifetime, so it cannot make any assumption + // about that lifetime, and in particular, it cannot claim it returns a value with that + // lifetime since the lifetime in question is unknown at the point the closure is defined + // (it is only known inside here). + let c_kobj = unsafe { &*self.c_kobj.get() }; + f(c_kobj) + } + fn from_c_kobj(c_kobj: *mut CKObj) -> *mut KObjectInner { + const { + // This is relied on in order to be able transmute a CKObj pointer to a KObjectInner + // pointer so we can manipulate foreign kobjects, such as the sysfs root folder for the + // module in /sys/module/lisa/ + // + // If that assertion was not true anymore because we needed to store metadata in + // KObjectInner, we would need to add a level of indirection. + assert!(core::mem::size_of::() == core::mem::size_of::()); + }; + // SAFETY: it is safe to do that since we check that KObjectInner is exactly the same size // as CKObj and has repr(transparent) c_kobj as *mut KObjectInner @@ -165,6 +187,11 @@ impl KObjectInner { kobj_refcount(self.c_kobj.get()).into() } + #[inline] + fn is_in_sysfs(&self) -> bool { + self.with_c_kobj(|c_kobj| c_kobj.state_in_sysfs()) + } + unsafe fn update_refcount(&self, increase: bool) { #[cfunc] fn kobj_update_refcount(kobj: *mut CKObj, increase: bool) -> bool { @@ -199,17 +226,8 @@ impl KObjectInner { } fn kobj_type(&self) -> &KObjType { - #[cfunc] - fn kobj_kobj_type(kobj: *const CKObj) -> *const CKObjType { - r#" - #include - "#; + let c_kobj_type = self.with_c_kobj(|c_kobj| c_kobj.ktype() as *const CKObjType); - r#" - return kobj->ktype; - "# - } - let c_kobj_type = kobj_kobj_type(self.c_kobj.get()); // SAFETY: All the CKObjType we manipulate are part of a KObjType, so it is safe to cast // back. unsafe { @@ -229,7 +247,33 @@ impl Drop for KObjectInner { } } -pub struct KObject { +mod private { + pub trait Sealed {} +} +pub trait KObjectState: private::Sealed {} + +struct SysfsSpec { + name: String, + parent: Option>, +} + +#[derive(Default)] +pub struct Init { + sysfs: Option, +} +impl private::Sealed for Init {} +impl KObjectState for Init {} + +#[derive(Default)] +pub struct Finalized {} +impl private::Sealed for Finalized {} +impl KObjectState for Finalized {} + +#[derive(Debug)] +#[non_exhaustive] +pub enum Error {} + +pub struct KObject { // Use a pointer, so Rust does not make any assumption on the validity of the pointee. This // simplifies the drop flow. // @@ -237,14 +281,19 @@ pub struct KObject { // from an AllocatedKObjectInner and re-materalized as such when de-allocating). However, // Pin requires Ptr: Deref for anything interesting so we can't actually use it here. inner: *const KObjectInner, + state: State, } -// SAFETY: KObject is just a reference to a KObjectInner, so it is safe to pass it around threads. -unsafe impl Send for KObject {} +// SAFETY: Nothing in KObject cares about what thread it is on, so it can be sent around without +// issues. +unsafe impl Send for KObject {} +// SAFETY: In the Finalized state, all APIs are as thread-safe as KObjectInner is. In the Init +// state, we are not thread safe as there is some builder state maintained in the KObject +unsafe impl Sync for KObject {} -impl KObject { +impl KObject { #[inline] - pub fn new(kobj_type: Arc) -> KObject { + pub fn new(kobj_type: Arc) -> Self { let inner = KObjectInner::new(kobj_type); // SAFETY: Nothing in the public KObject API allows moving out the KObjectInner pointed at, // since that KObjectInner could be shared by any number of KObject instances, just like @@ -254,24 +303,74 @@ impl KObject { let inner = unsafe { Pin::into_inner_unchecked(inner) }; KObject { inner: Box::into_raw(inner), + state: Default::default(), } } + pub fn add<'a, 'parent, 'name>( + &'a mut self, + parent: Option<&'parent KObject>, + name: &'name str, + ) where + 'parent: 'a, + { + // FIXME: should we return an error or panic ? + if let Some(parent) = parent { + assert!( + parent.inner().is_in_sysfs(), + "The parent of KObject \"{name}\" was not added to sysfs" + ); + } + self.state.sysfs = Some(SysfsSpec { + name: name.into(), + parent: parent.cloned(), + }); + } + + pub fn finalize(self) -> Result, Error> { + #[cfunc] + unsafe fn kobj_add(kobj: *mut CKObj, parent: *mut CKObj, name: &[u8]) -> Result<(), c_int> { + r#" + #include + #include + "#; + + r#" + return kobject_add(kobj, parent, "%.*s", (int)name.len, name.data); + "# + } + if let Some(spec) = &self.state.sysfs { + let parent = match &spec.parent { + None => core::ptr::null_mut(), + Some(parent) => parent.c_kobj(), + }; + unsafe { kobj_add(self.c_kobj(), parent, spec.name.as_bytes()) } + .expect("Call to kobject_add() failed"); + } + Ok(KObject::from_inner(self.inner())) + } +} + +impl KObject { + fn from_inner(inner: &KObjectInner) -> Self { + inner.incref(); + KObject { + inner, + state: Default::default(), + } + } #[inline] - pub fn module_root() -> KObject { + pub fn sysfs_module_root() -> Self { let c_kobj = c_eval!("linux/kobject.h", "&THIS_MODULE->mkobj.kobj", *mut CKObj); let inner = KObjectInner::from_c_kobj(c_kobj); // SAFETY: We assume that the pointer we got is valid as it is comming from a well-known // kernel API. let inner = unsafe { inner.as_ref().unwrap() }; - KObject::from_inner(inner) - } - - fn from_inner(inner: &KObjectInner) -> Self { - inner.incref(); - KObject { inner } + Self::from_inner(inner) } +} +impl KObject { fn inner(&self) -> &KObjectInner { // SAFETY: Since we hold a kref reference, we know we are pointing at valid memory. unsafe { self.inner.as_ref().unwrap() } @@ -280,40 +379,9 @@ impl KObject { fn c_kobj(&self) -> *mut CKObj { self.inner().c_kobj.get() } - - // FIXME: We need to prevent the following sequence: - // a=new() - // b=new() - // a.add(parent=Some(a), name=bar) - // b.add(parent=None, name=foo) - // - // This is because we can apparently only add children under a parent that has been added - // already, we cannot build the hierarchy "virtually" before attaching it via a top-level add. - pub fn add<'a, 'parent, 'name>(&'a self, parent: Option<&'parent Self>, name: &'name str) - where - 'parent: 'a, - { - #[cfunc] - unsafe fn kobj_add(kobj: *mut CKObj, parent: *mut CKObj, name: &[u8]) -> Result<(), c_int> { - r#" - #include - #include - "#; - - r#" - return kobject_add(kobj, parent, "%.*s", (int)name.len, name.data); - "# - } - let parent: *mut CKObj = match parent { - Some(parent) => parent.c_kobj(), - None => core::ptr::null_mut(), - }; - unsafe { kobj_add(self.c_kobj(), parent, name.as_bytes()) } - .expect("Failed to call kobject_add()"); - } } -impl Drop for KObject { +impl Drop for KObject { fn drop(&mut self) { // SAFETY: We increased the refcount when created, so we decrease it when dropped. The // release implementation of KObjType will take care of freeing the memory if needed. @@ -325,7 +393,9 @@ impl Drop for KObject { } } -impl Clone for KObject { +// Only implement clone for the Finalized state so that we do not accidentally call kobject_add() +// twice on the same underlying "struct kobject". +impl Clone for KObject { fn clone(&self) -> Self { // SAFETY: self.inner is always valid as long as a KObject points to it. let inner = unsafe { self.inner.as_ref().unwrap() }; diff --git a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/tests.rs b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/tests.rs index 2c67a6ac5..9dda47fd2 100644 --- a/lisa/_assets/kmodules/lisa/rust/lisakmod/src/tests.rs +++ b/lisa/_assets/kmodules/lisa/rust/lisakmod/src/tests.rs @@ -248,11 +248,17 @@ fn test_8() { pr_info!("Rust: test_8"); use crate::runtime::sysfs::{KObjType, KObject}; - let root = KObject::module_root(); + let root = KObject::sysfs_module_root(); let kobj_type = Arc::new(KObjType::new()); - let kobject = KObject::new(kobj_type.clone()); - let kobject2 = KObject::new(kobj_type.clone()); + let mut kobject = KObject::new(kobj_type.clone()); + let mut kobject2 = KObject::new(kobj_type.clone()); + kobject.add(Some(&root), "foo"); + let kobject = kobject.finalize().expect("Could not finalize kobject"); + kobject2.add(Some(&kobject), "bar"); + let kobject2 = kobject2.finalize().expect("Could not finalize kobject"); + + drop(kobject2); } diff --git a/lisa/_kmod.py b/lisa/_kmod.py index 7f94604b7..3eb09f838 100644 --- a/lisa/_kmod.py +++ b/lisa/_kmod.py @@ -3079,7 +3079,7 @@ class FtraceDynamicKmod(DynamicKmod): class _LISADynamicKmodSrc(KmodSrc): _RUST_SPEC = dict( - version='1.82.0', + version='1.83.0', components=[ # rust-src for -Zbuild-std 'rust-src', -- GitLab From c9c2fea0e966efcd10cc6aa82ece6ae303312fab Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Mon, 2 Dec 2024 12:49:59 +0000 Subject: [PATCH 2/5] lisa._kmod: Cache build of Rust code FEATURE Use a persistent CARGO_TARGET_DIR located in the cache so that we re-use existing artifacts when building the Rust code multiple times, leaving the cache invalidation to cargo when needed. --- lisa/_assets/kmodules/lisa/Makefile | 12 +-- lisa/_kmod.py | 153 ++++++++++++++++++---------- 2 files changed, 107 insertions(+), 58 deletions(-) diff --git a/lisa/_assets/kmodules/lisa/Makefile b/lisa/_assets/kmodules/lisa/Makefile index a59eb5b98..d591294cf 100644 --- a/lisa/_assets/kmodules/lisa/Makefile +++ b/lisa/_assets/kmodules/lisa/Makefile @@ -85,11 +85,11 @@ RUST_C_SHIMS_DIR := $(RUST_GENERATED)/rust_c_shims RUSTUP_HOME ?= $(HOME)/.rustup CARGO_HOME ?= $(HOME)/.cargo +CARGO_TARGET_DIR ?= $(RUST_BUILD_DIR)/target RUST_BUILD_DIR := $(RUST_GENERATED)/build RUST_SYMBOLS := $(RUST_BUILD_DIR)/exported.list RUST_SYMBOLS_CLI := $(RUST_BUILD_DIR)/exported.cli -RUST_TARGET_DIR := $(RUST_BUILD_DIR)/target RUST_CBINDGEN_BIN := $(CARGO_HOME)/bin/cbindgen # Export c_flags so that lisa-eval-c.sh can compile a C source with the @@ -105,7 +105,7 @@ export LISA_EVAL_C_CFLAGS=$(shell echo $(c_flags)) # Enable RUSTC_BOOTSTRAP=1 so we can use nightly features on any toolchain, # including stable ones. This allows using the more tested stable binaries. -rust_cmd = chmod +x '$(LISA_EVAL_C)' && export RUSTC_BOOTSTRAP=1 PATH="$(CARGO_HOME)/bin:$$PATH" 'RUSTUP_HOME=$(RUSTUP_HOME)' 'CARGO_HOME=$(CARGO_HOME)' 'LISA_EVAL_C=$(LISA_EVAL_C)' && $(1) +rust_cmd = chmod +x '$(LISA_EVAL_C)' && export RUSTC_BOOTSTRAP=1 PATH="$(CARGO_HOME)/bin:$$PATH" 'RUSTUP_HOME=$(RUSTUP_HOME)' 'CARGO_HOME=$(CARGO_HOME)' 'CARGO_TARGET_DIR=$(CARGO_TARGET_DIR)' 'LISA_EVAL_C=$(LISA_EVAL_C)' && $(1) cargo_cmd = $(call rust_cmd,cargo +$(RUST_VERSION) $(1)) $(RUST_GENERATED): $(GENERATED) @@ -114,18 +114,18 @@ $(RUST_GENERATED): $(GENERATED) $(RUST_BUILD_DIR): $(RUST_GENERATED) mkdir -p "$@" -$(RUST_TARGET_DIR): $(RUST_BUILD_DIR) +$(CARGO_TARGET_DIR): mkdir -p "$@" # Build the rust code into a static archive, then prelink it into an object # file and filter the symbols to only keep as GLOBAL the exported pub # #[no_mangle] ones. This avoids having tons of GLOBAL symbols from core lib. -$(RUST_OBJECT) $(RUST_C_SHIMS_H) $(RUST_C_SHIMS_C): $(RUST_TARGET_DIR) +$(RUST_OBJECT) $(RUST_C_SHIMS_H) $(RUST_C_SHIMS_C): $(RUST_BUILD_DIR) $(CARGO_TARGET_DIR) # Build the crate into a static library - cd $(RUST_SRC) && export RUSTFLAGS="$(RUSTFLAGS)" && $(call cargo_cmd,build --target-dir $(RUST_TARGET_DIR) --release --target=$(RUST_SRC)/rustc_targets/$(ARCH)/target.json -Zbuild-std=core$(comma)alloc) + cd $(RUST_SRC) && export RUSTFLAGS="$(RUSTFLAGS)" && $(call cargo_cmd,build --release --target=$(RUST_SRC)/rustc_targets/$(ARCH)/target.json -Zbuild-std=core$(comma)alloc) # Prelink the archive into a single object file. - $(LD) $(KBUILD_LDFLAGS) -nostdlib -r -o $(RUST_OBJECT) --whole-archive $(RUST_TARGET_DIR)/target/release/liblisakmod.a + $(LD) $(KBUILD_LDFLAGS) -nostdlib -r -o $(RUST_OBJECT) --whole-archive $(CARGO_TARGET_DIR)/target/release/liblisakmod.a # Get the list of exported symbols python3 "$(MODULE_SRC)/rust_exported_symbols.py" --rust-object $(RUST_OBJECT) --out-symbols-plain $(RUST_SYMBOLS) --out-symbols-cli $(RUST_SYMBOLS_CLI) diff --git a/lisa/_kmod.py b/lisa/_kmod.py index 3eb09f838..4c503532d 100644 --- a/lisa/_kmod.py +++ b/lisa/_kmod.py @@ -137,6 +137,7 @@ import fnmatch import sys from enum import IntEnum import traceback +import uuid from elftools.elf.elffile import ELFFile @@ -2556,6 +2557,42 @@ class KmodSrc(Loggable): else: return filenames[0] + @contextlib.contextmanager + def rust_target_dir(rust_home): + build_cache = rust_home / 'build_cache' + target_dir = build_cache / 'reference' + tmp_target_dir = build_cache / f'tmp_{uuid.uuid4().hex}' + + # Move the build cache to a temporary location, so we know no + # one else will be touching it while we are using it. Cargo + # protects itself from concurrent accesses to the target + # folder, but this does not extend to all the steps we do after + # cargo has run to use the build artifacts. + # + # Path.rename() gives the same guarantees as os.rename(), and + # on POSIX platforms this is an atomic operation if they are on + # the same filesystem. + try: + target_dir.mkdir(parents=True, exist_ok=True) + target_dir.rename(tmp_target_dir) + except Exception: + tmp_target_dir = None + + try: + yield tmp_target_dir + finally: + if tmp_target_dir: + # When we are finished with the CARGO_TARGET_DIR, we + # place it back at the expected location for the next + # build to find it. + try: + tmp_target_dir.rename(target_dir) + except Exception: + # If we could not promote it back to the reference + # CARGO_TARGET_DIR, there is no point in keeping it + # around. + shutil.rmtree(tmp_target_dir) + rust_spec = self._RUST_SPEC or {} if build_conf['build-env'] == 'alpine': settings = build_conf['build-env-settings']['alpine'] @@ -2563,48 +2600,55 @@ class KmodSrc(Loggable): alpine_packages = settings.get('packages', None) @contextlib.contextmanager - def cmd_cm(): + def rust_cm(rust_spec): if rust_spec: rust_home = Path('/opt/rust') rustup_home = rust_home / 'rustup' cargo_home = rust_home / 'cargo' - _rust_spec = { - **rust_spec, - 'rustup_home': rustup_home, - 'cargo_home': cargo_home, - } - rust_env = { - 'RUSTUP_HOME': rustup_home, - 'CARGO_HOME': cargo_home, - 'RUST_VERSION': rust_spec['version'], - } - else: - rust_env = {} - _rust_spec = None - with _make_build_chroot( - cc=cc.name, - cross_compile=cross_compile, - bind_paths=bind_paths, - abi=abi, - overlay_backend=build_conf['overlay-backend'], - version=alpine_version, - packages=alpine_packages, - rust_spec=_rust_spec, - ) as chroot: - # Do not use a CM here to avoid choking on permission - # issues. Since the chroot itself will be entirely - # removed it's not a problem. - mod_path = Path(tempfile.mkdtemp(dir=chroot / 'tmp')) - cmd = make_cmd( - tree_path=tree_path, - mod_path=f'/{mod_path.relative_to(chroot)}', - make_vars={ - **make_vars, - **rust_env, + with rust_target_dir(rust_home) as target_dir: + _rust_spec = { + **rust_spec, + 'rustup_home': rustup_home, + 'cargo_home': cargo_home, } - ) - yield (mod_path, _make_build_chroot_cmd(chroot, cmd)) + rust_env = { + 'RUSTUP_HOME': rustup_home, + 'CARGO_HOME': cargo_home, + 'CARGO_TARGET_DIR': target_dir, + 'RUST_VERSION': rust_spec['version'], + } + yield (_rust_spec, rust_env) + else: + yield (None, {}) + + @contextlib.contextmanager + def cmd_cm(): + with rust_cm(rust_spec) as (_rust_spec, rust_env): + with _make_build_chroot( + cc=cc.name, + cross_compile=cross_compile, + bind_paths=bind_paths, + abi=abi, + overlay_backend=build_conf['overlay-backend'], + version=alpine_version, + packages=alpine_packages, + rust_spec=_rust_spec, + ) as chroot: + # Do not use a CM here to avoid choking on permission + # issues. Since the chroot itself will be entirely + # removed it's not a problem. + mod_path = Path(tempfile.mkdtemp(dir=chroot / 'tmp')) + cmd = make_cmd( + tree_path=tree_path, + mod_path=f'/{mod_path.relative_to(chroot)}', + make_vars={ + **make_vars, + **rust_env, + } + ) + yield (mod_path, _make_build_chroot_cmd(chroot, cmd)) + elif build_conf['build-env'] == 'host': def install_rust(rust_spec): def populate(key, path): @@ -2627,28 +2671,33 @@ class KmodSrc(Loggable): return rust_home @contextlib.contextmanager - def cmd_cm(): + def rust_cm(rust_spec): if rust_spec: rust_home = install_rust(rust_spec) - rust_env = { - 'RUSTUP_HOME': rust_home / 'rustup', - 'CARGO_HOME': rust_home / 'cargo', - 'RUST_VERSION': rust_spec['version'], - } + with rust_target_dir(rust_home) as target_dir: + yield { + 'RUSTUP_HOME': rust_home / 'rustup', + 'CARGO_HOME': rust_home / 'cargo', + 'CARGO_TARGET_DIR': target_dir, + 'RUST_VERSION': rust_spec['version'], + } else: - rust_env = {} + yield {} + @contextlib.contextmanager + def cmd_cm(): with tempfile.TemporaryDirectory() as mod_path: - cmd = make_cmd( - tree_path=tree_path, - mod_path=mod_path, - make_vars={ - **make_vars, - **rust_env, - }, - ) + with rust_cm(rust_spec) as rust_env: + cmd = make_cmd( + tree_path=tree_path, + mod_path=mod_path, + make_vars={ + **make_vars, + **rust_env, + }, + ) - yield (mod_path, cmd) + yield (mod_path, cmd) else: raise ValueError('Unknown build-env kind: {build_env}') -- GitLab From bc3bfde91f46d84eb959d21196692568d9c3915c Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Mon, 2 Dec 2024 14:33:21 +0000 Subject: [PATCH 3/5] lisa._kmod: Rework pre-installed module detection FIX Ensure we do not emit a warning in normal conditions when no pre-installed module is detected. --- lisa/_kmod.py | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/lisa/_kmod.py b/lisa/_kmod.py index 4c503532d..750b95735 100644 --- a/lisa/_kmod.py +++ b/lisa/_kmod.py @@ -3260,35 +3260,33 @@ class LISADynamicKmod(FtraceDynamicKmod): logger.debug(f'Looking for pre-installed {kmod_filename} module in {base_path}') super_ = super() - def preinstalled_broken(e): - logger.debug(f'Pre-installed {kmod_filename} is unsuitable, recompiling: {e}') + def preinstalled_unsuitable(excep=None): + if excep is not None: + logger.debug(f'Pre-installed {kmod_filename} is unsuitable, recompiling: {excep.__class__.__qualname__}: {excep}') return super_.install(kmod_params=kmod_params) try: kmod_path = target.execute( f"{busybox} find {base_path} -name {quote(kmod_filename)}" ).strip() - except subprocess.CalledProcessError as e: - ret = preinstalled_broken(e) + except subprocess.CalledProcessError: + # If find fails, this means base_path does not even exist on the + # target, so we just install the module + return preinstalled_unsuitable() else: - if kmod_path: - - if len(kmod_path.splitlines()) > 1: - ret = preinstalled_broken(FileNotFoundError(kmod_filename)) - else: - @contextlib.contextmanager - def kmod_cm(): - yield kmod_path - - try: - ret = self._install(kmod_cm(), kmod_params=kmod_params) - except (subprocess.CalledProcessError, KmodVersionError) as e: - ret = preinstalled_broken(e) - else: - logger.warning(f'Loaded "{self.mod_name}" module from pre-installed location: {kmod_path}. This implies that the module was compiled by a 3rd party, which is available but unsupported. If you experience issues related to module version mismatch in the future, please contact them for updating the module. This may break at any time, without notice, and regardless of the general backward compatibility policy of LISA.') + kmod_path = kmod_path.strip() + if len((kmod_paths := kmod_path.splitlines())) > 1: + return preinstalled_unsuitable(ValueError(f'Multiple paths found for {kmod_filename}: {", ".join(kmod_paths)}')) else: - ret = preinstalled_broken(FileNotFoundError(kmod_filename)) - - return ret + # We found an installed module that could maybe be suitable, so + # we try to load it. + try: + return self._install(nullcontext(kmod_path), kmod_params=kmod_params) + except (subprocess.CalledProcessError, KmodVersionError) as e: + # Turns out to not be suitable, so we build our own + return preinstalled_unsuitable(e) + else: + logger.warning(f'Loaded "{self.mod_name}" module from pre-installed location: {kmod_path}. This implies that the module was compiled by a 3rd party, which is available but unsupported. If you experience issues related to module version mismatch in the future, please contact them for updating the module. This may break at any time, without notice, and regardless of the general backward compatibility policy of LISA.') + return None # vim :set tabstop=4 shiftwidth=4 expandtab textwidth=80 -- GitLab From 07146f762b0c23a4646901d368a92eca9fcd11db Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Mon, 2 Dec 2024 14:53:17 +0000 Subject: [PATCH 4/5] lisa._kmod: Use versioned LLVM tools in Alpine FIX Instead of just using the default version for LLVM tools, use the packages with the specific version we are looking for, e.g. llvm17-objcopy from llvm17 package. --- lisa/_kmod.py | 68 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 16 deletions(-) diff --git a/lisa/_kmod.py b/lisa/_kmod.py index 750b95735..52f263249 100644 --- a/lisa/_kmod.py +++ b/lisa/_kmod.py @@ -504,6 +504,15 @@ def _make_build_chroot(cc, cross_compile, abi, bind_paths=None, version=None, ov pass +def default_llvm_tool_name(tool, llvm): + if tool == 'clang': + return f'clang{llvm}' + elif tool == 'ld': + return f'ld.lld{llvm}' + else: + return f'llvm-{tool}{llvm}' + + @destroyablecontextmanager def _make_alpine_chroot(version, packages=None, abi=None, bind_paths=None, overlay_backend='overlayfs', rust_spec=None): logger = logging.getLogger(f'{__name__}.alpine_chroot') @@ -1527,14 +1536,6 @@ class _KernelBuildEnv(Loggable, SerializeViaConstructor): if 'clang' in cc.name and 'LLVM' not in make_vars: clang_version = _clang_version_static(cc.name) llvm_version = f'-{clang_version}' if clang_version else '1' - if build_conf['build-env'] == 'alpine': - # TODO: Revisit: - # We do not use llvm_version here as Alpine does not ship - # multiple versions of e.g. lld, only multiple versions of - # clang. Kbuild fails to find ld.lld- since that - # binary does not exist on Alpine. Same goes for other tools - # like "ar" or "nm" - llvm_version = '1' make_vars['LLVM'] = llvm_version # Turn errors into warnings by default, as this otherwise prevents the @@ -1555,18 +1556,53 @@ class _KernelBuildEnv(Loggable, SerializeViaConstructor): if make_vars.get('LLVM') == '0': del make_vars['LLVM'] + llvm = make_vars.get('LLVM') + # Some kernels have broken/old Kbuild that does not honor the LLVM=-N # suffixing, so force the suffixes ourselves. - llvm = make_vars.get('LLVM') + # + # Also, the expectation of Kbuild in terms of binary name (e.g. + # llvm-objcopy-17) are violated on Alpine that uses llvm17-objcopy + # convention instead. So we override Kbuild detection with something + # that works + if build_conf['build-env'] == 'alpine': + # Alpine has a different naming convention than most other distros, e.g.: + # https://pkgs.alpinelinux.org/contents?name=llvm15&repo=main&branch=edge&arch=aarch64 + def llvm_tool_name(tool, llvm): + version = llvm.strip('-') + return f'llvm{version}-{tool}' + + if llvm: + # TODO: Revisit: + # Alpine does not ship multiple versions of e.g. lld, only + # multiple versions of clang. Kbuild fails to find + # ld.lld- since that binary does not exist on + # Alpine. + # + # Note from Alpine 3.21, there should be an ld.lld18 package in + # addition to the main package, but then the main package + # should be in version 19 anyway so there is probably no point + # in supporting that. From some comments in the build recipe, + # that ld.lld18 package is only there for zig, so there is a + # good chance it disappears again in 3.22 + make_vars.setdefault('LD', 'ld.lld') + make_vars.setdefault('HOSTLD', 'ld.lld') + else: + llvm_tool_name = default_llvm_tool_name + if llvm and llvm.startswith('-'): updated = { - 'LD': f'ld.lld{llvm}', - 'AR': f'llvm-ar{llvm}', - 'NM': f'llvm-nm{llvm}', - 'OBJCOPY': f'llvm-objcopy{llvm}', - 'OBJDUMP': f'llvm-objdump{llvm}', - 'READELF': f'llvm-readelf{llvm}', - 'STRIP': f'llvm-strip{llvm}', + var: llvm_tool_name(_var.lower(), llvm) + for _var in ( + 'LD', + 'AR', + 'NM', + 'OBJCOPY', + 'OBJDUMP', + 'READELF', + 'STRIP', + ) + for var in (_var, f'HOST{_var}') } make_vars = {**updated, **make_vars} -- GitLab From 4942518a21ae72bf75921db51a0a2b64467c118a Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Mon, 2 Dec 2024 17:21:26 +0000 Subject: [PATCH 5/5] lisa._kmod: Fixup Alpine chroot binary names FIX The binaries for the version of clang and LLVM that Alpine ships as its default version will not have any version number in their name. This is an issue as it is not trivial to know what version Alpine uses by default, and there is no package providing versioned name either. Fix that by creating shims for versioned names that simply redirect to the normal binary when necessary. --- lisa/_kmod.py | 123 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 104 insertions(+), 19 deletions(-) diff --git a/lisa/_kmod.py b/lisa/_kmod.py index 52f263249..9c3c2ec9c 100644 --- a/lisa/_kmod.py +++ b/lisa/_kmod.py @@ -138,6 +138,7 @@ import sys from enum import IntEnum import traceback import uuid +import textwrap from elftools.elf.elffile import ELFFile @@ -152,6 +153,15 @@ import lisa._git as git from lisa.conf import SimpleMultiSrcConf, TopLevelKeyDesc, LevelKeyDesc, KeyDesc, VariadicLevelKeyDesc from lisa._kallsyms import parse_kallsyms +_KERNEL_BINUTILS = ( + 'ld', + 'ar', + 'nm', + 'strip', + 'objcopy', + 'objdump', + 'readelf', +) def _tar_extractall(f, *args, **kwargs): # Avoid DeprecationWarning, see: @@ -513,6 +523,22 @@ def default_llvm_tool_name(tool, llvm): return f'llvm-{tool}{llvm}' +def alpine_llvm_tool_name(tool, llvm): + """ + Alpine has a different naming convention than most other distros. + + See: + https://pkgs.alpinelinux.org/contents?name=llvm15&repo=main&branch=edge&arch=aarch64 + """ + if tool == 'clang': + return f'clang{llvm}' + elif tool == 'ld': + return f'ld.lld{llvm}' + else: + version = llvm.strip('-') if llvm else '' + return f'llvm{version}-{tool}' + + @destroyablecontextmanager def _make_alpine_chroot(version, packages=None, abi=None, bind_paths=None, overlay_backend='overlayfs', rust_spec=None): logger = logging.getLogger(f'{__name__}.alpine_chroot') @@ -548,12 +574,81 @@ def _make_alpine_chroot(version, packages=None, abi=None, bind_paths=None, overl path = path.resolve() run_cmd = lambda cmd: run_cmd_chroot(path, cmd) - def install_packages(packages): + def fixup_llvm_tools(root, packages): + """ + Alpine has packages with version names for LLVM and clang (e.g. + llvm17) but the actual binaries in that package may or may not have + the version number in them. + + They only have it if the package is not the default version for the + tool. E.g. Alpine 3.20 uses clang 17 as default for clang, so the + clang17 will ship a command named "clang" instead of "clang-17". + Packages for the non-default versions will ship binaries with the + version name in them, e.g. "clang-18" binary in the "clang18" + package. + """ + regex = re.compile(r'(?P[^0-9]*)(?P[0-9]*)') + packages = [ + package + for package in packages + if any( + x in package + for x in ('llvm', 'clang') + ) + ] + packages = [ + (tool, version) + for package in packages + if ( + (m := regex.match(package)) and + (version := m.group('version')) and + (tool := m.group('tool')) + ) + ] + + def expand_tool(tool): + if tool == 'llvm': + return _KERNEL_BINUTILS + else: + return (tool,) + + commands = [ + ( + alpine_llvm_tool_name(tool, None), + alpine_llvm_tool_name(tool, f'-{version}') + ) + for _tool, version in packages + for tool in expand_tool(_tool) + ] + + for unversioned_cmd, versioned_cmd in commands: + try: + run_cmd(['which', versioned_cmd]) + except subprocess.CalledProcessError: + logger.debug(f'Creating "{versioned_cmd}" shim for "{unversioned_cmd}" since this version of Alpine ships an unversioned binary name for that tool/version combination') + # We just write in /usr/bin instead of e.g. /usr/local/bin + # since /usr/local/bin is already bind-mounted to a folder + # in LISA with the binaries we ship for that arch. + # + # Also, we are done installing packages so it's somewhat ok + # to put our own files there if they don't exist already. + versioned_path = Path(root) / 'usr' / 'bin' / versioned_cmd + versioned_path.parent.mkdir(parents=True, exist_ok=True) + snippet = textwrap.dedent(f''' + #!/bin/sh + + exec {unversioned_cmd} "$@" + ''').strip() + versioned_path.write_text(snippet) + versioned_path.chmod(0o755) + + def install_packages(root, packages): if packages: run_cmd(['apk', 'add', *sorted(set(packages))]) + fixup_llvm_tools(root, packages) - def enable_network(): - shutil.copy('/etc/resolv.conf', reroot(path, '/etc/resolv.conf')) + def enable_network(root): + shutil.copy('/etc/resolv.conf', reroot(root, '/etc/resolv.conf')) # Packages have already been installed, so we can speed things up a # bit @@ -576,10 +671,10 @@ def _make_alpine_chroot(version, packages=None, abi=None, bind_paths=None, overl with tarfile.open(tar_path, 'r') as f: _tar_extractall(f, path=path) - enable_network() - install_packages(packages) + enable_network(path) + install_packages(path, packages) elif stage == 'finalize': - enable_network() + enable_network(path) else: raise ValueError(f'Unknown stage: {stage}') @@ -1566,12 +1661,7 @@ class _KernelBuildEnv(Loggable, SerializeViaConstructor): # convention instead. So we override Kbuild detection with something # that works if build_conf['build-env'] == 'alpine': - # Alpine has a different naming convention than most other distros, e.g.: - # https://pkgs.alpinelinux.org/contents?name=llvm15&repo=main&branch=edge&arch=aarch64 - def llvm_tool_name(tool, llvm): - version = llvm.strip('-') - return f'llvm{version}-{tool}' - + llvm_tool_name = alpine_llvm_tool_name if llvm: # TODO: Revisit: # Alpine does not ship multiple versions of e.g. lld, only @@ -1594,13 +1684,8 @@ class _KernelBuildEnv(Loggable, SerializeViaConstructor): updated = { var: llvm_tool_name(_var.lower(), llvm) for _var in ( - 'LD', - 'AR', - 'NM', - 'OBJCOPY', - 'OBJDUMP', - 'READELF', - 'STRIP', + tool.upper() + for tool in _KERNEL_BINUTILS ) for var in (_var, f'HOST{_var}') } -- GitLab