Support for atomic bitwise operations in MSP430 PAC API

[YuhanLiin]

Typically, when setting or clearing bits of a register, RMW in the form of reg.modify(|r, w| ...) is needed, which can be interrupted. On the MSP430 architecture however there are instructions such as bis and xor that can modify specific bits of a memory location atomically. This change adds APIs to the register proxies to take advantage of those instructions. I also updated the MSP430 regression test cases, though they're still broken because MSP430 doesn't compile on stable.

As discussed on matrix, we don't want to merge this until after the MSP430 improvement patch goes in, so I'm uploading the PR for feedback.

[rust-highfive]

r? @reitermarkus

(rust_highfive has picked a reviewer for you, use r? to override)

[cr1901]

FWIW, this looks fine to me. Will take a closer look after the msp430 improvements are merged.

Please keep in mind that msp430_atomics will never be stable because of inline asm (unless somehow we can get them into libcore...)

Additionally, why are your set_bits, clear_bits, and toggle_bits methods unsafe?

[YuhanLiin]

The new API is unsafe because the bit patterns specified in the write proxy may not be equal to the bits in the actual register after the operation. For example, calling set_bits with write proxy of 11000000 on a register with value 00110000 results in a register value of 11110000. The resulting bit pattern may not be valid for the register, so the method is unsafe (same reason why reg.bits() is unsafe).
The stability thing is an issue. Hopefully we get libcore intrinsic support for the required instructions before building MSP430 is stabilized in its entirety.

[therealprof]

What's the status of this?

[cr1901]

@therealprof No further movement on my end, and I just realized this PR as-is would conflict with getting msp430 to work completely on stable (since msp430-atomic uses feature(asm)), which is on my radar.

I would like to hear from @YuhanLiin before closing (and resubmitting) later, though.

[YuhanLiin]

Haven't looked into this PR in a while, so it's probably out of date right now. The stability issue still stands, since there're no ASM intrinsics in MSP430 right now, and I don't see any workarounds at all. Closing with the intention of reopening later makes sense.

[burrbull]

Is there any progress?

[YuhanLiin]

The PR still relies on msp430-atomic, which requires the unstable llvm_asm feature. If that's still a concern then I don't see any way forward, since asm isn't going to be available on MSP430 anytime soon. Paging @cr1901 for more info.

[burrbull]

The PR still relies on msp430-atomic, which requires the unstable llvm_asm feature. If that's still a concern then I don't see any way forward, since asm isn't going to be available on MSP430 anytime soon. Paging @cr1901 for more info.

As said one guy llvm_asm never be stable.
But you can put this under feature to hide.

[cr1901]

If that's still a concern then I don't see any way forward, since asm isn't going to be available on MSP430 anytime soon.

I think it's more accurate to say there are other blockers on getting msp430 stabilized (I have a list somewhere) that makes it low priority to swap from llvm_asm to asm across the core msp430 crates. That said, there is relatively little asm in the msp430 ecosystem:

• msp430-atomic is one, because it's copied from libcore with changes that should be upstreamed eventually.
• msp430-rt sidesteps the problem by compiling assembly to a static lib.
• msp430 has llvm_asm because I considered the overhead of function calls for intrinsics unacceptable. -Clinker-plugin-lto can't be used because we're stuck with binutils.

I could change this all to asm if necessary, though I think we should hold off on this change for now personally, as I don't quite have the bandwidth to change even that much assembly.

In addition, I thought the "new" asm syntax was also unstable (but eventually will be stabilized)? Did that change?

[YuhanLiin]

asm is still unstable. It also doesn't support the MSP430 architecture. The real question is, do we want code generated by svd2rust to rely on msp430-atomic (and by extension, llvm_asm)? If we do, then should that dependency be gated behind a feature flag?

[cr1901]

It also doesn't support the MSP430 architecture.

Oh great, that means it's gonna be involved to add, doesn't it?

The real question is, do we want code generated by svd2rust to rely on msp430-atomic (and by extension, llvm_asm)?

PACs generated with svd2rust already rely on nightly, unfortunately. So at first glance, it seems like it makes little difference to depend on more unstable features.

That said, just because I accept the consequences of using nightly doesn't mean other people need to. I still intend to create a stable path for msp430 code:

• In principle, until msp430-interrupt is stabilized, it is possible to get rid of this dependency using a carefully coded interrupt handlers in asm that we link to. But I haven't implemented this yet.
• The other asm can be removed using analogous object files/static archives we link to.
• AtomicBool, strictly speaking, isn't necessary- Mutex<Cell<bool>> will do the same thing, with more code :).

If we do, then should that dependency be gated behind a feature flag?

I think so, as unstable features can allow extremely useful optimizations at the cost of being tied to nightly. I can put msp430-abi-interrupt behind a feature as well once a stable path exist. Call the feature atomic and maybe create an unstable feature that encompasses all the features?

[burrbull]

unstable feature that encompasses all the features

I have the same opinion

[burrbull]

cc @therealprof What do you think about?

[burrbull]

@YuhanLiin instead of using compile time feature, you can use runtime --nightly option already present in svd2rust

[YuhanLiin]

So should the atomics be gated purely by the nightly runtime flag or should there be a compile-time feature gating it as well, as per the previous discussions?

[burrbull]

I think runtime flag is enough for this.

[burrbull]

LGTM. Please, use rebase instead of merge and update changelog.

[YuhanLiin]

How should I undo my previous merge?

[burrbull]

Just use this branch https://github.com/rust-embedded/svd2rust/tree/msp-atomic2