Fix some clippy warnings

[usbalbin]

An attempt at making clippy happy

• 1. There is a feature stm32h747cm7 but no stm32h757cm7, should I add that? - Removed
• 2. I do not see this being used anywhere in any config, should this be removed or allow(dead-code)ed? - Allowed
• 3. It seems that the code that creates an instance is commented out, should this also be commented out? - Removed

[richardeoin]

Thanks for the PR!

There is a feature stm32h747cm7 but no stm32h757cm7, should I add that?

No, both H747 and H757 are supported through the same feature gate. So actually just remove line 114 115

I do not see this being used anywhere in any config, should this be removed or allow(dead-code)ed?

It's the start of an incomplete implementation. This trait is implemented - but not used as a trait bound and not available outside the sai module. I would dead-code it

It seems that the code that creates an instance is commented out, should this also be commented out?

Yes, or removed alongside all the commented code. The commented code is still available in the git history and previous releases

[usbalbin]

Done, thanks :)

Any recommendations for this one?

error: creating a mutable reference to mutable static is discouraged
  --> examples/serial-dma.rs:92:18
   |
92 |         unsafe { SHORT_BUFFER.assume_init_mut() }
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable reference to mutable static
   |
   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html>
   = note: mutable references to mutable statics are dangerous; it's undefined behavior if any other pointer to the static is used or if any other reference is created for the static while the mutable reference lives
note: the lint level is defined here
  --> examples/serial-dma.rs:10:9
   |
10 | #![deny(warnings)]
   |         ^^^^^^^^
   = note: `#[deny(static_mut_refs)]` implied by `#[deny(warnings)]`

[usbalbin]

mutable references to mutable statics are dangerous; it's undefined behavior if any other pointer to the static is used or if any other reference is created for the static while the mutable reference lives
note: the lint level is defined here

I think I understand the reason for the lint generally. However in this particular instance where we as programmers can easily check that there in fact is no other reference to the static, so it should be ok right? As long as we dont have multiple mutable references it is fine, right?

Reading post on urlo seems to suggest that static (not mut) FOO: UnsafeCell<T> might in some cases be preferred over plain static mut BAR: T.

Do you have any preferences?

Edit:

Since UnsafeCell impls !Sync, things got quite clunky. Here ad0d1fc is what I came up with, not sure it is worth it...

Also, sorry, but what does the linker section things do on the statics? cortex_m::singleton solves quite a lot except any way to do the linker thing right?

Edit: 2 Having that AtomicBool in that magic linker section place might be not great...

[richardeoin]

This is also being discussed over on #509, and in #513 . In #509 we discussed the static-cell crate which provides a use-once wrapper around UnsafeCell. That is basically what you implemented in ad0d1fc 😅 . So you might think it's a good idea to use the static-cell crate, but there's more.

The link_section attribute places the static in a memory location outside the memory that is initialised by the runtime. In this case the runtime is cortex-m-rt. Therefore the assignment = StaticCell::new() / BorrowOnce::new(..) will not actually happen, and the value of the AtomicBool is undefined, triggering UB.

[richardeoin]

One way forward would be to define data: UnsafeCell<MaybeUninit<T>>, and don't perform runtime checks for double use. As long as it really is only used once there's no static mut and it's functional.

[usbalbin]

Thanks for the info and links :)

One way forward would be to define data: UnsafeCell<MaybeUninit<T>>, and don't perform runtime checks for double use. As long as it really is only used once there's no static mut and it's functional.

How does the safety rules apply to structs where all the actual data is behind a MaybeUninit but the entire struct is uninit. Is it ok to call methods on it like get_mut since that takes a reference to the uninited BorrowOnce?

// In our case T=[MaybeUninit<u8>; 10], so all actual data is behind `MaybeUninit`(?)
struct BorrowOnce<T> {
    data: UnsafeCell<T>,
}
unsafe impl<T: Sync> Sync for BorrowOnce<T> {}

impl<T> BorrowOnce<T> {
    const fn new(data: T) -> Self {
        Self {
            data: UnsafeCell::new(data),
        }
    }
    /// SAFETY: Make sure to only ever have at most one single reference live at any point in time
    #[allow(clippy::mut_from_ref)]
    unsafe fn get_mut(&self) -> &mut T {
        &mut *self.data.get()
    }
}

I am not entirely sure of the rules here

[richardeoin]

According to the documentation for MaybeUninit::as_ptr these rules are not finalised even.

There exists a UnsafeCell::raw_get method to avoid the creation of temporary references, which suggests there is a reason for avoiding this. The documentation for that method offers some hints about combining MaybeUninit and UnsafeCell, I think it would look something like this

struct BorrowOnce<T> {
    data: MaybeUninit<UnsafeCell<T>>,
}
unsafe impl<T: Sync> Sync for BorrowOnce<T> {}
impl<T> BorrowOnce<T> {
    const fn new() -> Self {
        Self {
            data: MaybeUninit::uninit(),
        }
    }

    unsafe fn init(&'static self, val: T) -> &'static mut T {
        // initialise data without creating a temporary reference
        // this is safe as long as our reference to self.data is unique (no aliasing)
        UnsafeCell::raw_get(self.data.as_ptr()).write(val);
        // this is safe as long as our reference to self.data is unique (no aliasing)
        &mut *self.data.assume_init_ref().get()
    }
}

[usbalbin]

But are we even allowed to call any methods on BorrowOnce that take self by reference since the BorrowOnce (the stuff outside the MaybeUninit) itself is uninitialized?

               Reference to uninitialized memory?
               |
               v
unsafe fn init(&'static self, val: T) -> &'static mut T {

#[weird_linker_things_which_makes_this_be_not_initialized_at_start]
static FOO: BorrowOnce<T> = ...;

FOO.init(...); // <-- Here we take a reference to uninitialized memory

Is that even ok?

Or does init have to take a pointer to the BorrowOnce to avoid any references?

[richardeoin]

It's certainly allowed to call methods on MaybeUninit that take self by reference whilst uninitialised. I would assume that also extends to newtypes containing MaybeUninit, since the compiler is then aware that the data within may be uninitialised.

[usbalbin]

My concern is not with the data within MaybeUninit but with what is outside(technically nothing?). Also, just to clarify I am no memory safety expert so take my concern/questions with a truck load of salt :)

Not sure if this is close enough to what we are trying to do memory wise but Miri seems happy playground

[usbalbin]

Either way, the static mut thing is perhaps better discussed and solved in #509 and #513?

Anything more to fix in this PR?

[richardeoin]

@usbalbin Yes, agreed. The contents of this PR look good!