Possible unsoundness on architectures without atomics

[ROMemories]

My understanding is that log relies on atomics to implement some of its functionality, especially set_logger() and set_max_level().

It seems there is an internal "fake" AtomicUsize on platforms that do not provide atomics, with the assumption that no such architectures are multicore:

https://github.com/rust-lang/log/blob/53594839745cb2bc8ba09d6ba9a3758f45c5da47/src/lib.rs#L451-L454

However this assumption does not hold on the RP2040 MCU, which features two Cortex-M0+ (ARMv6-M) cores.

I am not sure this actually currently results in unsoundness because it is not yet clear to me whether the instances of AtomicUsize are actually used in ways that could result in race conditions on these architectures.

I suppose leveraging portable-atomic on architectures that do not provide atomics could help fix this, if this is an acceptable dependency for log, which would only be required on such architectures.

---

For context, we at Ariel OS are building an embedded OS which, among many other things, abstracts over log and makes it easily usable on many processor architectures, making this easy to test.

[Thomasdezeeuw]

Note that set_logger has the cfg(target_has_atomic = "ptr") attribute, i.e. it shouldn't be available if the atomic are not present https://github.com/rust-lang/log/blob/53594839745cb2bc8ba09d6ba9a3758f45c5da47/src/lib.rs#L1402-L1405

In that case the unsafe set_logger_racy has to be used: https://github.com/rust-lang/log/blob/53594839745cb2bc8ba09d6ba9a3758f45c5da47/src/lib.rs#L1454-L1467

That doesn't make things any safer, but it means that log isn't unsound.

[ROMemories]

Pondering on this some more, it seems to be me that the platform being multicore or not is actually irrelevant, it just has to support multi-threading for this to be an issue.

In that case the unsafe set_logger_racy has to be used

I am aware of this function, but then the logger() getter function actually relies on the happens-before relationship between LOGGER and STATE:

https://github.com/rust-lang/log/blob/53594839745cb2bc8ba09d6ba9a3758f45c5da47/src/lib.rs#L1506-L1521

However, on architectures without atomics, STATE really is not an atomic, and so this happens-before relationship is not guaranteed.

[Thomasdezeeuw]

However, on architectures without atomics, STATE really is not an atomic, and so this happens-before relationship is not guaranteed.

Agreed. But then you'll most likely also not have threads. Personally I don't see how threads can do any cross-thread communication safely without atomics.

[ROMemories]

But then you'll most likely also not have threads.

These MCUs—the RP2040 and the ESP32-C3 (RISC-V without the (A) extension) for instance—do support threading however, and we do support it in Ariel OS (even multicore).

Personally I don't see how threads can do any cross-thread communication safely without atomics.

This is done through critical sections on bare metal, which involve temporarily disabling interrupts (see the critical-section crate and the implementation in the cortex-m crate for instance). These can be used to emulate atomics on architectures that do not support them, which is what portable-atomic does (when needed).

[Thomasdezeeuw]

This is done through critical sections on bare metal, which involve temporarily disabling interrupts (see the critical-section crate and the implementation in the cortex-m crate for instance). These can be used to emulate atomics on architectures that do not support them, which is what portable-atomic does (when needed).

Interesting, I didn't know about that -- I don't do a lot of embedded work myself.

Would calling set_logger_racy in such a critical section be sufficient? (it's something to docs already call out: https://docs.rs/log/latest/log/fn.set_logger_racy.html#safety)

[ROMemories]

(it's something to docs already call out: https://docs.rs/log/latest/log/fn.set_logger_racy.html#safety)

The docs mention that:

It is safe to use other logging functions while this function runs (including all logging macros).

So I think this is not exactly complete, as it would not be safe to call logger() while set_logger_racy() runs.

Would calling set_logger_racy in such a critical section be sufficient?

Wrapping these calls in a critical section should indeed enforce that.