Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove references to static mut #483

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

richardeoin
Copy link
Member

@richardeoin richardeoin commented Jan 20, 2024

rust/#114447 made &mut MY_STATIC_MUT a warning, with the intention of disallowing it entirely in the 2024 edition. We never used this in the crate itself, but it is used in many examples.

This commit removes these references from the examples. In general the new approach is to change static mut of type X to type MaybeUninit<X>. These can then cleanly be accessed with MY_STATIC_MUT.write() followed by MY_STATIC_MUT.assume_init_mut().

Where we need to cast a (reference to a)MaybeUninit<[X; N]> to a [MaybeUninit<X>; N] in order to initialise it element-by-element, the &mut *ptr::addr_of_mut!(MY_STATIC_MUT) construction is still used. After initialisation, we can then access it with MY_STATIC_MUT.assume_init_mut().

In a few DMA Examples we were taking a reference &mut TARGET_BUFFER without initialising TARGET_BUFFER. This commit changes this to TARGET_BUFFER.assume_init_mut() without initialising TARGET_BUFFER. This was and remains UB.

  • USB Examples: static mut EP_MEMORY now has type MaybeUninit<[u32; 1024]>
  • Ethernet Examples: static mut DES_RING now has type MaybeUninit<ethernet::DesRing<4, 4>>

@richardeoin richardeoin force-pushed the no-reference-static-mut branch 2 times, most recently from 835136f to a40ffdd Compare January 20, 2024 22:10
@richardeoin
Copy link
Member Author

I re-tested the following examples and they are still working:

  • examples/ethernet-rtic-stm32h747i-disco.rs
  • examples/mdma.rs
  • examples/usb_serial.rs

I'm going to tag a few of the original authors by their examples below. If you still have access to some suitable hardware it would be really awesome if you can re-test these examples in the next week or two(!)

examples/ethernet-rtic-nucleo-h723zg.rs @jlogan03
examples/sai_dma_passthru.rs @antoinevg
examples/serial-dma.rs @mattico
examples/spi-dma-rtic.rs @ryan-summers

@jlogan03
Copy link
Contributor

Hi! Thanks for doing this - I'm traveling this week, but I'll test it as soon as I'm back to my hardware

@mattico
Copy link
Contributor

mattico commented Jan 24, 2024

@richardeoin thanks again for all the work you do maintaining this!

I tested examples/serial-dma.rs on an ST NUCLEO-H743ZI2 board and it worked, I checked the transmitted data.

I also tested examples/spi-dma-rtic.rs on the same board and checked the transmitted data. Well the first few bytes at least, I had to decode manually because my scope's SPI decoder wasn't working. It's been a while.


Rather than doing TARGET_BUFFER.assume_init_mut() on an uninitialized buffer, what would the "correct" solution be? Creating a wrapper struct containing a pointer and implementing embedded_dma::WriteBuffer on it? I won't ask you to implement that in this PR but maybe you could add a comment saying it's UB and pointing in the right direction, wherever that may be.

@antoinevg
Copy link
Contributor

Can confirm examples/sai_dma_passthru.rs is still working fine.

@richardeoin
Copy link
Member Author

Rather than doing TARGET_BUFFER.assume_init_mut() on an uninitialized buffer, what would the "correct" solution be? Creating a wrapper struct containing a pointer and implementing embedded_dma::WriteBuffer on it? I won't ask you to implement that in this PR but maybe you could add a comment saying it's UB and pointing in the right direction, wherever that may be.

Creating a mutable reference like &mut [u8] to uninitialised memory is UB. The solutions would be:

  • Initialise the memory first
  • Or yes have a wrapper type around a raw pointer that implements embedded_dma::WriteBuffer without creating that &mut reference

rust/#114447 made `&mut MY_STATIC_MUT` a warning, with the intention of
disallowing it entirely in the 2024 edition. We never used this in the crate
itself, but it is used in many examples.

This commit removes these references from the examples. In general the new
approach is to change `static mut` of type `X` to type `MaybeUninit<X>`. These
can then cleanly be accessed with
[`MY_STATIC_MUT.write()`](https://doc.rust-lang.org/core/mem/union.MaybeUninit.html#method.write)
followed by
[`MY_STATIC_MUT.assume_init_mut()`](https://doc.rust-lang.org/core/mem/union.MaybeUninit.html#method.assume_init_mut).

Where we need to cast a `MaybeUninit<[X; N]>` to a `[MaybeUninit<X>; N]` in
order to initialise it element-by-element, the
`&mut *ptr::addr_of_mut!(MY_STATIC_MUT)` construction is still used. After
initialisation, we can then access it with `MY_STATIC_MUT.assume_init_mut()`.

In a few DMA Examples we were taking a reference `&mut TARGET_BUFFER` without
initialising TARGET_BUFFER. This commit changes this to
`TARGET_BUFFER.assume_init_mut()` *without* initialising `TARGET_BUFFER`. This
was and remains UB.

* USB Examples: `static mut EP_MEMORY` now has type `MaybeUninit<[u32; 1024]>`
* Ethernet Examples: `static mut DES_RING` now has type `MaybeUninit<ethernet::DesRing<4, 4>>`
@richardeoin
Copy link
Member Author

Rebased onto master

@jlogan03
Copy link
Contributor

Confirmed ethernet-rtic-nucleo-h723zg still runs

@richardeoin richardeoin added this pull request to the merge queue Jan 31, 2024
Merged via the queue into stm32-rs:master with commit 2bcf2af Jan 31, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants