Tidy up some inline asm and add compiler fences where appropriate #264
Conversation
|
r? @therealprof (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
S-waiting-on-review
|
Nice one. Looks good on first glance but I want to give people some time to look it over. |
|
Hm, superweird that some archives are shrinking massively in size. These functions are always compiled with optimisation? Otherwise the |
|
I don't understand the GitHub percentages (I think it's "number of lines" somehow?); the files are all around 14k-25kB; the changes are all less than 1kB. I don't understand why they're changing this much still though, the ones I inspected manually had some different debug strings in the ELF but otherwise no obvious clues. |
|
The xtask build generates the staticlib and LTO libs with optimisations enabled. If you're using the |
I wonder whether that's the right play here or whether we should place the fences ourselves, rather than using |
|
Without the fences the functions are potentially unsound but they are marked unsafe, so we could just document the fence requirement in each relevant method's safety requirements. It's a bit of a chore though and it means users must put those fences in themselves, where they're subject to the exact same issues. At least having them here means that for most users not using Having said that, it's not clear to me how the fence here makes a difference to a caller of either the staticlib or LTO lib, which presumably only sees the FFI call, which also suggests to me that LLVM really shouldn't dare reorder instructions around an FFI call. |
| asm!("msr CONTROL, {}", "isb", in(reg) w); | ||
|
|
||
| // Ensure instructions are not reordered around the CONTROL update. | ||
| compiler_fence(Ordering::SeqCst); |
There was a problem hiding this comment.
This only affects memory instructions. Probably a good idea to clarify that in the comments.
There was a problem hiding this comment.
I've done so, but will the asm! block's default memory clobber ensure no memory accesses can be reordered around the block anyway? Will the compiler fence even survive being turned into an FFI call?
I've added these fences on the understanding that the compiler might otherwise reorder instructions around the asm call, but I don't fully see what the fence would prevent that's not prevented by the asm! block and/or FFI call (with or without LTO inlining).
There's no CHANGELOG entry for the new asm stuff at all, and it doesn't change the user-facing API (in other words it's an implementation detail). I've added a very brief description to the CHANGELOG about it. |
I agree, but that doesn't make it any better.
Not quite. It can now be linked with via the LTO linker plugin. |
Good point, I've added a specific note about that to the CHANGELOG too. |
|
Hm, weird. Why is it complaining about the blobs not being up-to-date? bors try |
tryBuild failed: |
|
Because 5d5e151 didn't update the blobs |
There was a problem hiding this comment.
@jonas-schievink Ah, thanks!
I guess all comments have been sufficiently addressed so I'm inclined to get this on the road then?
Anyone want to give it another nod and pull the trigger?
|
I'm away from my computer for a few days but if anyone else wants to rebuild the blobs and push to the branch please go ahead! |
Already done. |
|
bors r=therealprof |
|
Build succeeded: |
264: Document MSRV in README r=korken89 a=eldruin See: rust-embedded/wg#445 Co-authored-by: Diego Barrios Romero <[email protected]>
This PR updates the inline asm:
Open to feedback on whether more or fewer fences are necessary; I've thought about these a bit but I think it's a tricky subject. I think in general the FFI-esque treatment of the new
asm!block probably does most of what we need, but I'm told LLVM may still reorder instructions around FFI calls, which we really don't want to happen here.