| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DBKTNE4OF0IB.3C32NVHQ9UKQ3@nvidia.com>
Date: Fri, 25 Jul 2025 12:38:57 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Benno Lossin" <lossin@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>,
"Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>,
"Gary Guo" <gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
"Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
"Danilo Krummrich" <dakr@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
<nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH 1/3] rust: add `num` module with `PowerOfTwo` type
Hi Benno,
Sorry, took some time to come back to this!
On Sun Jun 22, 2025 at 5:11 PM JST, Benno Lossin wrote:
> On Fri Jun 20, 2025 at 3:14 PM CEST, Alexandre Courbot wrote:
>> +/// An unsigned integer which is guaranteed to be a power of 2.
>> +///
>> +/// # Invariants
>> +///
>> +/// The stored value is guaranteed to be a power of two.
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
>> +#[repr(transparent)]
>> +pub struct PowerOfTwo<T>(T);
>> +
>> +macro_rules! power_of_two_impl {
>> + ($($t:ty),+) => {
>> + $(
>> + impl PowerOfTwo<$t> {
>
> I tried to use this type in a doctest like this:
>
> use kernel::num::PowerOfTwo;
>
> fn new(x: usize) -> PowerOfTwo<usize> {
> PowerOfTwo::new(1 << x)
> }
>
> And it doesn't compile :(
>
> error[E0034]: multiple applicable items in scope
> --> rust/doctests_kernel_generated.rs:4930:17
> |
> 4930 | PowerOfTwo::new(1 << x)
> | ^^^ multiple `new` found
> |
> = note: candidate #1 is defined in an impl for the type `PowerOfTwo<u128>`
> = note: candidate #2 is defined in an impl for the type `PowerOfTwo<u16>`
> = note: candidate #3 is defined in an impl for the type `PowerOfTwo<u32>`
> = note: candidate #4 is defined in an impl for the type `PowerOfTwo<u64>`
> = note: and 2 others
>
> error: aborting due to 1 previous error
>
> The problem is that the function `new` exists 6 times for each of the
> integer types. You can write `PowerOfTwo::<usize>::new()` instead, but
> that's annoying...
This should go away as we switch to the non-generic `Alignment` type
thankfully.
>
> We probably need an `Integer` trait and then do
>
> impl<I: Integer> PowerOfTwo<I> {
> pub const fn new(value: I) -> Self;
> }
>
>> + /// Validates that `v` is a power of two at build-time, and returns it wrapped into
>> + /// [`PowerOfTwo`].
>> + ///
>> + /// A build error is triggered if `v` cannot be asserted to be a power of two.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::PowerOfTwo;
>> + ///
>> + #[doc = concat!("let v = PowerOfTwo::<", stringify!($t), ">::new(16);")]
>> + /// assert_eq!(v.value(), 16);
>> + /// ```
>> + #[inline(always)]
>> + pub const fn new(v: $t) -> Self {
>> + build_assert!(v.count_ones() == 1);
>
> Why not `v.is_power_of_two()`?
Why not indeed. :) Fixed.
>
>> + Self(v)
>
> Missing `// INVARIANT` comment.
Added (and in other places as well).
>
>> + }
>> +
>> + /// Validates that `v` is a power of two at runtime, and returns it wrapped into
>> + /// [`PowerOfTwo`].
>> + ///
>> + /// [`None`] is returned if `v` was not a power of two.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::PowerOfTwo;
>> + ///
>> + #[doc = concat!(
>> + "assert_eq!(PowerOfTwo::<",
>> + stringify!($t),
>> + ">::try_new(16), Some(PowerOfTwo::<",
>> + stringify!($t),
>> + ">::new(16)));"
>> + )]
>> + #[doc = concat!(
>> + "assert_eq!(PowerOfTwo::<",
>> + stringify!($t),
>> + ">::try_new(15), None);"
>> + )]
>> + /// ```
>> + #[inline(always)]
>> + pub const fn try_new(v: $t) -> Option<Self> {
>
> Maybe `new_checked` is a better name, since it doesn't return a result?
Definitely.
>
>> + match v.count_ones() {
>
> Why not `is_power_of_two()`?
Fixed, thanks.
>
>> + 1 => Some(Self(v)),
>
> Missing `// INVARIANT` comment.
>
>> + _ => None,
>> + }
>> + }
>> +
>> + /// Returns the value of this instance.
>> + ///
>> + /// It is guaranteed to be a power of two.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::PowerOfTwo;
>> + ///
>> + #[doc = concat!("let v = PowerOfTwo::<", stringify!($t), ">::new(16);")]
>> + /// assert_eq!(v.value(), 16);
>> + /// ```
>> + #[inline(always)]
>> + pub const fn value(self) -> $t {
>> + self.0
>
> Let's add:
>
> if !self.0.is_power_of_two() {
> core::hint::unreachable_unchecked()
> }
> self.0
Sure. Is it to enable compiler optimizations by making assumptions about
the returned value?
>
>> + }
>> +
>> + /// Returns the mask corresponding to `self.value() - 1`.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::PowerOfTwo;
>> + ///
>> + #[doc = concat!("let v = PowerOfTwo::<", stringify!($t), ">::new(0x10);")]
>> + /// assert_eq!(v.mask(), 0xf);
>> + /// ```
>> + #[inline(always)]
>> + pub const fn mask(self) -> $t {
>> + self.0.wrapping_sub(1)
>
> Then use `self.value().wrapping_sub(1)` here instead to also propagate
> the information.
Ack.
Powered by blists - more mailing lists