asm/inline: explicitly use asm macro

[jordens]

asm!() removed from prelude in current nightly
rust-lang/rust#91728

close #371

This is also a good candidate for the cortex-m v0.7 series.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

Please see the contribution instructions for more information.

[adamgreig]

LGTM, any reason it's still marked as draft?

[jordens]

Just that I haven't actually been able to test it with the affected new nightly...

[TDHolmes]

Looks like it should be core::asm?

error[E0432]: unresolved import `core::arch::asm`
149
  --> asm/inline.rs:10:5
150
   |
151
10 | use core::arch::asm;
152
   |     ^^^^^^^^^^^^^^^ no `asm` in `arch`
153
   |
154
   = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined
155
help: a macro with this name exists at the root of the crate
156
   |
157
10 | use core::asm;
158
   |     ^^^^^^^^^

[jordens]

@adamgreig good to go now.

[adamgreig]

Thanks!

bors merge

[bors]

Build failed:

• ci-linux (stable)

[adamgreig]

Ah, the pinned nightly we use to build the pre-built binaries is too old to know about core::arch::asm. Probably fine to simply bump the version in rust-toolchain.toml but it would be good to check and understand any differences in the generated asm.

[TDHolmes]

I think every user of cortex_m::asm::inline will need to do use core::arch::asm with this approach. Should we just change all calls to asm! to core::arch::asm! in inline.rs?

[jordens]

@adamgreig do you prefer a minimal bump or a maximal bump to 2021-12-16?
It doesn't look good.

--- 2020-08-20-lib      2021-12-17 08:21:37.685375537 +0100
+++ 2021-12-16-lib      2021-12-17 08:26:04.931123275 +0100
@@ -6,229 +6,291 @@
 Disassembly of section .text.__bkpt:

 00000000 <__bkpt>:
-   0:  be00            bkpt    0x0000
-   2:  4770            bx      lr
+   0:  b580            push    {r7, lr}
+   2:  466f            mov     r7, sp
+   4:  be00            bkpt    0x0000
+   6:  bd80            pop     {r7, pc}

 Disassembly of section .text.__control_r:

 00000000 <__control_r>:
-   0:  f3ef 8014       mrs     r0, CONTROL
-   4:  4770            bx      lr
+   0:  b580            push    {r7, lr}
+   2:  466f            mov     r7, sp
+   4:  f3ef 8014       mrs     r0, CONTROL
+   8:  bd80            pop     {r7, pc}

 Disassembly of section .text.__control_w:

 00000000 <__control_w>:
-   0:  f380 8814       msr     CONTROL, r0
-   4:  f3bf 8f6f       isb     sy
-   8:  4770            bx      lr
+   0:  b580            push    {r7, lr}
+   2:  466f            mov     r7, sp
+   4:  f380 8814       msr     CONTROL, r0
+   8:  f3bf 8f6f       isb     sy
+   c:  bd80            pop     {r7, pc}

 Disassembly of section .text.__cpsid:

 00000000 <__cpsid>:
-   0:  b672            cpsid   i
-   2:  4770            bx      lr
+   0:  b580            push    {r7, lr}
+   2:  466f            mov     r7, sp
+   4:  b672            cpsid   i
+   6:  bd80            pop     {r7, pc}

...

[jordens]

I'll try a minimal bump.

[adamgreig]

Oh jeez, the new assembly doesn't look good indeed. I wonder what changed there.

[jordens]

There is no nightly that has asm!() in the location that is in now but does not do that stack dance by default.
options(nostack) helps and looks like it's the right approach. But somebody with deeper ARM/Thumb machine knowledge needs to make the analysis where that can actually be used safely. It's a bunch of functions.

[adamgreig]

Thanks for checking, I'd just been going down that path myself. Adding suitable nomem and nostack options to the asm calls gets us identical disassembly output so looks like it should be OK and we can then bump to the latest nightly. I've made the changes but will just check them over now. OK for me to push a commit to this PR for review?

[jordens]

Absolutely! Thanks!

[adamgreig]

Hm, github is suggesting I can add more commits by pushing to quartiq/cortex-m, but I get permission denied when I try to actually do that. Anyway, it's this commit.

@rust-embedded/cortex-m, anyone else able to check over the new nostack, nomem, preserves_flags options? I think it's mostly straightforward (nomem means we don't read/write memory, but almost all our asm routines are just modifying registers or special registers; nostack means we don't read/write stack which again is almost always the case, preserves_flags means no status-modifying instructions (like the 'S' variants 'ADDS' etc)).

The reference for what the options mean is here.

With this change the disassembly output is identical to what we have now (phew).

[jordens]

Feel free to hijack this (close, reopen with your branch). Alternatively, I can just pull your changes.
I went ahead and pulled them in here.

[jordens]

I went through the functions and checked their behavior w.r.t. the options constraints. Looks correct AFAICT.
Also and explicitly "Unless the nostack option is set ... The stack pointer must be restored to its original value before leaving the asm block." implying that to mess with the stack pointer nostack is required.

[adamgreig]

I took that to mean "without nostack, you may use the stack, but you must restore SP to its original value before finishing", but without the further implication that "if you do specify nostack, it's fine to mess with the stack pointer".

Anyway, good enough for now, thanks for the PR and checking the options!

bors merge

[bors]

Build succeeded:

• ci-linux (1.40.0)
• ci-linux (stable)
• clippy
• rustfmt

[jordens]

It just says that with nostack it may be fine to mess with the stack pointer. Without it you have to undo what you did.

[jeffy1009]

If compiler_fence prevents compiler from moving memory load/stores across it, and nomem tells the compiler that the asm block doesn't perform memory operations, isn't it theoretically possible that the asm block is moved across the compiler_fence?

1. Is it impossible to happen?
2. or is it possible (there is no such checks in the compiler), but the compiler doesn't really reorder asm and compiler_fence in such ways according to how it's implemented?