Conversation
0fe3a7c to
2abb7dd
Compare
ec6c7ef to
667de8f
Compare
b9cb8b5 to
74e91d2
Compare
The `cm7-r0p1` feature only works with the plateform feature `armv7em`. I added a compile error to help someone figure that out more redily if they accidentally combine those features.
This will allow getting all the asm in shape for edition 2024.
9cad22a to
256dc0f
Compare
3a0b6e9 to
0d75936
Compare
|
@thejpster Here's what you asked for. |
Inline asm has been supported in stable rust for some time, so I removed the separate asm build infra and added that code to the normal crate code. I also crated a mock of the asm functions that were needed to run clippy with a non-arm target (like x86_64).
0d75936 to
e7d07d2
Compare
.github/workflows/clippy.yml
Outdated
| strategy: | ||
| matrix: | ||
| # All generated code should be running on stable now | ||
| rust: [stable] |
There was a problem hiding this comment.
clippy should only be run on a fixed version, otherwise new releases that add new lints will break the build.
.github/workflows/clippy.yml
Outdated
|
|
||
| # steps: | ||
| # - uses: actions/checkout@v4 | ||
| # - uses: dtolnay/rust-toolchain@1.85 |
There was a problem hiding this comment.
Can the commented out text be removed?
|
Had a quick look on my phone - looks ok, just a few notes. I want to inspect the symbol table later when I get to a laptop. |
e7d07d2 to
29ef316
Compare
|
I removed the change to the clippy runs since I added the mock so that it can be run without issue with clippy targeting x86. |
|
If you want me to add that change to the tests back in for clippy I can. It was not really related to this change, so I didn't want to add more complexity. |
| #[cfg_attr(any(armv6m, armv7m, armv7em, armv8m), path = "asm/inner.rs")] | ||
| #[cfg_attr( | ||
| all(not(armv6m), not(armv7m), not(armv7em), not(armv8m)), | ||
| path = "asm/inner_mock.rs" | ||
| )] |
There was a problem hiding this comment.
If I understand this correctly this changes the behavior from declaring an extern "C" unsafe fn for the asm functions (which is what call_asm! does if inline-asm is not enabled) to silently replacing all functions with nops if the target is not one of these.
Before this PR, it would be a compile time error if one of these functions is used on an unsupported target (I think).
Is this the intended new behavior?
There was a problem hiding this comment.
I'm fine with that. If you are on an incompatible target, the point is to do enough to make clippy/unit tests/whatever happy, not do anything useful.
| use core::sync::atomic::{Ordering, compiler_fence}; | ||
|
|
||
| #[inline(always)] | ||
| pub unsafe fn __bkpt() { |
There was a problem hiding this comment.
These need marking with #[unsafe(no_mangle)] to ensure they have the same symbol as the old asm functions.
Perhaps like:
#[unsafe(link_section = ".text.__function_name_goes_here")]
#[cfg(not(feature = "inline-asm", unsafe(no_mangle)))]
#[inline(always)]
pub unsafe fn __bkpt() {
// inline asm goes here
}That way, anyone who has written code like the following example will still find their code works when not using inline-asm:
extern "C" {
fn __wfi();
}
unsafe {
__wfi();
}And anyone who is using inline-asm will find the following still works:
#[unsafe(no_mangle)]
pub extern "C" fn __wfi() {
// do stuff
}There was a problem hiding this comment.
OK, nah, we talked about this on Matrix and the __foo functions are not part of the public API - that's what the __ prefix means. So it's OK for the functions to be renamed.
There was a problem hiding this comment.
In fact you could remove the __ prefixes from all these functions (and the mock functions). It adds no value and suggests a backwards compatibility that isn't actually present.
| #[inline] | ||
| pub fn disable() { | ||
| call_asm!(__cpsid()); | ||
| unsafe { crate::asm::inner::__cpsid() }; |
There was a problem hiding this comment.
We should probably call the public api when we can
| #[inline] | ||
| pub unsafe fn enable() { | ||
| call_asm!(__cpsie()); | ||
| unsafe { crate::asm::inner::__cpsie() }; |
This is a draft for updating the asm toolchain. It's based on my #628, so it shouldn't be landed before that is.