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 aliasing &mut references to DMA buffers (possible UB) #41

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ErikNatanael
Copy link
Contributor

Getting the "warning: creating a mutable reference to mutable static is discouraged" warning prompted me to look into the code in audio.rs. Changing

let tx_buffer: &'static mut [u32; DMA_BUFFER_SIZE] = unsafe { &mut TX_BUFFER };

into

let tx_buffer: &'static mut [u32; DMA_BUFFER_SIZE] = unsafe { &mut *core::ptr::addr_of_mut!(TX_BUFFER) };

gets rid of the error, but I'm not convinced this is not undefined behaviour. The reason being that multiple &mut references are created to both TX_BUFFER and RX_BUFFER, and used via Input and Output.

This pull request gets around the issue by wrapping a raw pointer to the buffer instead of storing the references directly. *mut pointers are allowed to alias. References are created whenever the buffer is accessed, but these are short lived.

I have verified this works in practice: https://github.com/ErikNatanael/daisy-blank/tree/example-sine-wave

The first commit is the changes in PR #40

@mtthw-meyer
Copy link
Owner

I need to look into the correct way to do this myself and see what I dig up

@mtthw-meyer
Copy link
Owner

Not seeing anything better. The example in stm32h7xx-hal was updated but still has this warning.

@ErikNatanael
Copy link
Contributor Author

Thanks for taking a look. "anything better" about or than what do you mean? Sorry, don't quite understand.

I had a look at the stm32h7xx-hal dma example which uses MaybeUninit as well as core::ptr::addr_of_mut!. That might be better. But that example doesn't store multiple &mut references to the buffers, it sends them via raw pointers, so I still think using raw pointers is required instead of multiple &mut to avoid UB.

Since I wrote this code, the new &raw mut syntax was also stabilized, equivalent to core::ptr::addr_of_mut! IIUC.

@mtthw-meyer
Copy link
Owner

I tried using the MaybeUninit from the stm32h7xx-hal crate but it still gave the same warning with the assume_init() method. Raw pointers seems correct but not sure exactly how to use them yet. Maybe a combination of the two. Use Maybeuninit to handle the Init (does it even need to be initialized for DMA?) and then convert to the &raw mut

@mtthw-meyer
Copy link
Owner

Cleans up nicely with
let tx_buffer = DmaBufferRawRef { ptr: &raw mut TX_BUFFER, };

@mtthw-meyer
Copy link
Owner

Need a similar wrapper for usb_midi and we can merge it.

@ErikNatanael
Copy link
Contributor Author

Great, I'll get on it later this week!

@ErikNatanael
Copy link
Contributor Author

So UsbBus::new requires a &'static mut so &raw mut cannot be used. Until they change their API, I suppressed the warning and added a comment documenting that any additional references to the EP_MEMORY is UB. Also fixed some other simple warnings while I was at it.

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.

2 participants