Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[kernel test robot]

Hi Danilo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 51d0de7596a458096756c895cfed6bc4a7ecac10]

url:    https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/rust-device-implement-Device-parent/20250321-063101
base:   51d0de7596a458096756c895cfed6bc4a7ecac10
patch link:    https://lore.kernel.org/r/20250320222823.16509-4-dakr%40kernel.org
patch subject: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device
config: x86_64-buildonly-randconfig-005-20250321 (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/config)
compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503220040.TDePlxma-lkp@intel.com/

All errors (new ones prefixed by >>):

   ***
   *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
   *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
   *** unless patched (like Debian's).
   ***   Your bindgen version:  0.65.1
   ***   Your libclang version: 20.1.1
   ***
   ***
   *** Please see Documentation/rust/quick-start.rst for details
   *** on how to set up the Rust support.
   ***
>> error[E0133]: use of extern static is unsafe and requires unsafe block
   --> rust/kernel/pci.rs:473:43
   |
   473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
   |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
   |
   = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Danilo Krummrich]

On Sat, Mar 22, 2025 at 12:56:58AM +0800, kernel test robot wrote:
> >> error[E0133]: use of extern static is unsafe and requires unsafe block
>    --> rust/kernel/pci.rs:473:43
>    |
>    473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
>    |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
>    |
>    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
turns into a warning *if* using an unsafe block.

*Not* requiring unsafe for this seems like the correct thing -- was this a
bugfix in the compiler?

I guess to make it work for all compiler versions supported by the kernel we
have to use unsafe and suppress the warning?

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Miguel Ojeda]

On Fri, Mar 21, 2025 at 6:44 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
> turns into a warning *if* using an unsafe block.
>
> *Not* requiring unsafe for this seems like the correct thing -- was this a
> bugfix in the compiler?
>
> I guess to make it work for all compiler versions supported by the kernel we
> have to use unsafe and suppress the warning?

It was a feature, but it has been fairly annoying -- it affected
several series, e.g. the latest KUnit one as well as:

    https://lore.kernel.org/rust-for-linux/CANiq72kuebpOa4aPxmTXNMA0eo-SLL+Ht9u1SGHymXBF5_92eA@mail.gmail.com/

Please see:

    https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics

So, yeah, we use `allow(unused_unsafe)` (no `expect`, since it depends
on the version).

I hope that helps.

Cheers,
Miguel

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Danilo Krummrich]

On Fri, Mar 21, 2025 at 07:59:08PM +0100, Miguel Ojeda wrote:
> On Fri, Mar 21, 2025 at 6:44 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > This requires an unsafe block for compilers < 1.82. For compilers >= 1.82 it
> > turns into a warning *if* using an unsafe block.
> >
> > *Not* requiring unsafe for this seems like the correct thing -- was this a
> > bugfix in the compiler?
> >
> > I guess to make it work for all compiler versions supported by the kernel we
> > have to use unsafe and suppress the warning?
> 
> It was a feature, but it has been fairly annoying -- it affected
> several series, e.g. the latest KUnit one as well as:

From the second link:

"Previously, the compiler's safety checks were not aware that the raw ref
operator did not actually affect the operand's place, treating it as a possible
read or write to a pointer. No unsafety is actually present, however, as it just
creates a pointer.

That sounds like it was a bug, or do I miss anything?

> 
>     https://lore.kernel.org/rust-for-linux/CANiq72kuebpOa4aPxmTXNMA0eo-SLL+Ht9u1SGHymXBF5_92eA@mail.gmail.com/
> 
> Please see:
> 
>     https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
> 
> So, yeah, we use `allow(unused_unsafe)` (no `expect`, since it depends
> on the version).
> 
> I hope that helps.

Yeah, thanks a lot. Especially for the second link, I couldn't find it even
after quite a while of searching.

I will respin right away, since otherwise the patches of v3 are reviewed.

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Miguel Ojeda]

On Fri, Mar 21, 2025 at 8:11 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> From the second link:
>
> "Previously, the compiler's safety checks were not aware that the raw ref
> operator did not actually affect the operand's place, treating it as a possible
> read or write to a pointer. No unsafety is actually present, however, as it just
> creates a pointer.
>
> That sounds like it was a bug, or do I miss anything?

Yeah, if they didn't intend it originally, then I would call it a bug
-- they also seemed to considered it a bug themselves in the end, so I
think you are right.

I meant it from the point of view of the language, i.e. in the sense
that it was a restriction before, and now they relaxed it, so more
programs are accepted, and the feature would be "you need less
`unsafe`" etc.

The blog post seems to mention both sides of the coin ("This code is
now allowed", "Relaxed this", "A future version of Rust is expected to
generalize this").

> Yeah, thanks a lot. Especially for the second link, I couldn't find it even
> after quite a while of searching.

You're welcome!

Cheers,
Miguel

Re: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device

[Benno Lossin]

On Fri Mar 21, 2025 at 5:56 PM CET, kernel test robot wrote:
> Hi Danilo,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 51d0de7596a458096756c895cfed6bc4a7ecac10]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/rust-device-implement-Device-parent/20250321-063101
> base:   51d0de7596a458096756c895cfed6bc4a7ecac10
> patch link:    https://lore.kernel.org/r/20250320222823.16509-4-dakr%40kernel.org
> patch subject: [PATCH v2 3/4] rust: pci: impl TryFrom<&Device> for &pci::Device
> config: x86_64-buildonly-randconfig-005-20250321 (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/config)
> compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)
> rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250322/202503220040.TDePlxma-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202503220040.TDePlxma-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    ***
>    *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
>    *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
>    *** unless patched (like Debian's).
>    ***   Your bindgen version:  0.65.1
>    ***   Your libclang version: 20.1.1
>    ***
>    ***
>    *** Please see Documentation/rust/quick-start.rst for details
>    *** on how to set up the Rust support.
>    ***
>>> error[E0133]: use of extern static is unsafe and requires unsafe block
>    --> rust/kernel/pci.rs:473:43
>    |
>    473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
>    |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
>    |
>    = note: extern statics are not controlled by the Rust type system: invalid data, aliasing violations or data races will cause undefined behavior

This is just a minor annoyance with these error reports, but I would
very much like the error to have the correct indentation:

>> error[E0133]: use of extern static is unsafe and requires unsafe block
    --> rust/kernel/pci.rs:473:43
        |
    473 |         if dev.bus_type_raw() != addr_of!(bindings::pci_bus_type) {
        |                                           ^^^^^^^^^^^^^^^^^^^^^^ use of extern static
        |

Would that be possible to fix? That way the `^^^^` align with the item
they are referencing.

Otherwise they are very helpful!

---
Cheers,
Benno