Replace UnsafeCell with MaybeUnit

[jannic]

trafficstars

This leads to slightly better code generation in some cases where the mutex contents are accessed multiple times in a row.

Closes: #52

[thalesfragoso]

Thanks for taking the time to investigate and create this PR.

We will have to also add a Drop implementation with assume_init_drop. I guess this is also non-breaking, since I think this would only break code that tries to destructure the Mutex, which isn't possible because the field is private.

[Dirbaio]

I'm not sure we should do this.

The only advantage AFAICT is better codegen when the contents are inmutable. I.e. Mutex<u32> and not Mutex<Cell<u32>>. However, what's the use case for that? It seems to me you can always replace a Mutex<u32> with just a u32.

When the mutex content does have interior mutability (like Cell or RefCell) the optimization will be prevented anyway by the inner RefCell, so we gain nothing by removing the outer RefCell. (and the optimization has to be prevented, because it's incorrect when you do have mutability).

So there's no codegen improvements for the way Mutex is actually used in the wild, is there?

and there are a few downsides:

• MaybeUninit feels the wrong tool for the job, the contents can never be actually uninit.
• Due to the above, the code ends up a bit more complex: it equires a manual drop impl now, and two more unsafe {} blocks (into_inner, and Drop).
• I still haven't seen a convincing explanation for why removing this UnsafeCell is sound.

[jannic]

The only advantage AFAICT is better codegen when the contents are inmutable. I.e. Mutex<u32> and not Mutex<Cell<u32>>. However, what's the use case for that? It seems to me you can always replace a Mutex<u32> with just a u32.

It can actually improve the generated code in case T contains some pointer - e.g. to a register. (That would also explain why it's wrapped in a Mutex in the first place, even though it's immutable: To avoid concurrent access to the register.)

In that case, the compiler can sometimes avoid a load instruction: https://godbolt.org/z/x8j4zn4Mb

So there's no codegen improvements for the way Mutex is actually used in the wild, is there?

Indeed I don't know if this is a common pattern.

and there are a few downsides:

* MaybeUninit feels the wrong tool for the job, the contents can never be actually uninit.

Sure, it's a workaround to say "don't use niches in here". Is there a better way.

* I still haven't seen a convincing explanation for why removing this UnsafeCell is sound.

How could it not be sound? UnsafeCell is only needed for interior mutability.

For any given T, one of two things must be true:

• Either T does provide interior mutability (ie. its internal representation can change even if a &T reference exists). In this case, it must already be or contain an UnsafeCell. So an additional UnsafeCell layer in Mutex<T> wouldn't change anything.
• Or T does not provide interior mutability at all. Then we know that, as long as we have access to a &Mutex<T>, the memory representation will not change. In that case, UnsafeCell is not needed and doesn't provide any advantages.

[jannic]

Alternatively, the comment https://github.com/rust-embedded/critical-section/pull/53/files#diff-71560e745278a3bdedc2c5684fa201e28b80663af84e25e82352cb8f5315610bR78-R85 would also fit for the current implementation, just replace MaybeUninit by UnsafeCell. That would answer #52 as well.

[Dirbaio]

contains some pointer - e.g. to a register. (That would also explain why it's wrapped in a Mutex in the first place, even though it's immutable: To avoid concurrent access to the register.)

If the pointer is Copy, this is a rather weak protection, it's easy to bypass by accident with let ptr = critical_section::with(|cs| *mutex.borrow(cs)); ptr.write(...);.

I could imagine a HAL struct that contains the pointer, and has &self methods that is Send but not Sync because the methods aren't threadsafe. A user might wrap it with a Mutex to be able to call these methods from different priorities, yes. Interesting. I wonder if anyone does this in practice tho?

Sure, it's a workaround to say "don't use niches in here". Is there a better way.

UnsafeCell also does that :)

How could it not be sound? UnsafeCell is only needed for interior mutability. (...)

yeah, maybe! It's just that UnsafeCell has so much extra spooky magic that scares me.

UnsafeCell is needed to obtain a &mut from a &. You can't simply transmute references, even if you're manually ensuring only one &mut exists at a time. You have to "inform" the compiler you're doing special stuff by using UnsafeCell.

So, is UnsafeCell needed to obtain a &SomethingSync from a &SomethingSend? In this case it is OK to manually ensure only one thread has &SomethingSend a time, no need to "inform" the compiler you're doing special stuff anymore? Why?

[jannic]

contains some pointer - e.g. to a register. (That would also explain why it's wrapped in a Mutex in the first place, even though it's immutable: To avoid concurrent access to the register.)

If the pointer is Copy, this is a rather weak protection, it's easy to bypass by accident with let ptr = critical_section::with(|cs| *mutex.borrow(cs)); ptr.write(...);.

I could imagine a HAL struct that contains the pointer, and has &self methods that is Send but not Sync because the methods aren't threadsafe. A user might wrap it with a Mutex to be able to call these methods from different priorities, yes.

Sure - exactly as shown in the example in the compiler exporer link:

pub struct Register {
    ptr: *mut u32,
}

impl Register {
    fn read(&self) -> u32 {
        unsafe { core::ptr::read_volatile(self.ptr) }
    }
}

Interesting. I wonder if anyone does this in practice tho?

:shrug:

Sure, it's a workaround to say "don't use niches in here". Is there a better way.

UnsafeCell also does that :)

I know, but it's not a "better way". It has the same disadvantage that it's meant for something else (interior mutability) that we don't need here, and is only used for this side-effect.

How could it not be sound? UnsafeCell is only needed for interior mutability. (...)

yeah, maybe! It's just that UnsafeCell has so much extra spooky magic that scares me.

That's essentially the same reason why @thalesfragoso wanted to keep the MaybeUninit if we removed UnsafeCell: Too much spooky magic so nobody is sure what's actually required.

On the one hand, it's of course wise to not take risks for negligible gains. And even if there was a clear specification that said omitting UnsafeCell is actually fine: If we are the only ones implementing a Mutex without an UnsafeCell, we might also be the first ones to run into obscure compiler bugs... On the other hand, this feels a bit like cargo-cult programming.

[Dirbaio]

done in #54