Make the HardFault trampoline opt-in
#406
Comments
|
From the lack of comments I guess this isn't too objectionable to anyone! I don't think the 24 byte flash overhead is a huge price to pay as-is, and definitely not fussed about runtime cost of the trampoline, but I agree it's very rarely used so if we can totally eliminate it for most users and leave it feature gated for users who need it, that seems fine. Having it available on SVCall and a few other faults seems useful too; what is the cost implication? Presumably a generic trampoline will only be in flash once, but I guess there's a small runtime overhead to all the faults and exceptions it intercepts, which might be fine for HardFault but perhaps less so for other more commonly used exceptions. |
|
Yeah, that's right. The generic trampoline is also larger than the specialized one, and much more complicated to actually write/maintain. |
|
We could also start by adding Cargo features only for |
|
I just had to fight CMRT to workaround the mandatory trampoline because I needed access to more things (like r4-r11), so +1 to make it optional. Alternative idea: why not completely remove the trampoline in CMRT (so HardFault works like other eceptions), and then create a new crate with exception frame manipulation functionality? The current ExceptionFrame functionality feels like "porcelain" to me (see rust-embedded/cortex-m-rt#250), and quite opinionated (read-only access, no access to r4-r11, no way to return to a custom EXC_RETURN value (the magic 0xFFFFFFFD addresses in LR). It would be better suited for another crate, with probably expanded functionality and customizability. |
423: Swap to just-stabilised asm!() and global_asm!() macros r=thejpster a=adamgreig Once Rust 1.59 is released in a couple of days, the `asm!()` and `global_asm!()` macros will become stable, and we will no longer require the various precompiled binaries we've used until now in cortex-m and cortex-m-rt. cc #420. This PR uses `asm!()` in cortex-m, and removes the inline-asm feature, since I anticipate this going into cortex-m 0.8 and so we don't need to leave it behind for compatibility. In various places the previous version would call an extern C method when built for the native target, which would only fail at link time; to preserve the ability to build on x86 I've either made the whole method require the `cortex_m` configuration, or where appropriate/convenient simply skipped the `asm!()` call. This PR replaces the old gcc-preprocessed `asm.S` in cortex-m-rt with use of `global_asm!()`, although since you can't normally use `#[cfg(...)]` attributes with `global_asm!()`, there's also a slightly scary macro modified from one invented by `@Dirbaio` for a similar purpose. I considered putting the initialisation of LR behind an armv6m flag, but since we want to restore it after calling `__pre_init` it seemed better to just leave it the same on both targets. I added Cargo features to optionally set SP and VTOR at startup, which has been variously requested but would previously have required multiplicatively more pre-built binaries. Now: no problem. Relevant issues: * rust-embedded/cortex-m-rt#283 * rust-embedded/cortex-m-rt#55 * rust-embedded/cortex-m-rt#254 * rust-embedded/cortex-m-rt#102 * rust-embedded/cortex-m-rt#338 I've tested these on a couple of targets (and updated the CI): on the whole there's a small improvement in code size due to everyone getting inlined asm, especially in `cortex_m::interrupt::free()`. The major downside is we bump our MSRV from 1.42 (March 2020) to 1.59 (Feb 2022). For cortex-m, I propose putting these changes in the upcoming 0.8 release (which is technically what the master branch is already on) and not backporting. For cortex-m-rt I'm not sure: we don't have any other pending breaking changes, so we could consider a patch release. Anyway, this PR doesn't commit to any particular releases, so we can decide that later. For cortex-m-semihosting/panic-semihosting I think a patch release would be ideal, especially since we had to yank the last c-m-sh release due to conflicting prebuilt binaries (a problem that should now vanish). Also tagging these issues that I think might also benefit from new inline asm: * #265 * #215 * #406 Co-authored-by: Adam Greig <[email protected]>
|
@adamgreig - I believe this was closed inadvertently after being merely mentioned on #423. (Since I still can't work out how to turn off the trampoline without replacing the linker script.) |
This makes sure we indicate fault on an external pin, whether it's a
software fault (panic) or a hardware fault (e.g. a bus error).
By some careful rearranging, I was able to do this without adding to the
size of the compiled program.
We are unfortunately still paying for the cortex-m-rt
HardFaultTrampoline routine. See:
rust-embedded/cortex-m#406
|
I'm still seeing it as open, but it certainly shouldn't be closed yet, you're right. Now that we have cfg_global_asm being used with features like set-vtor and set-sp, and soon the zero-init-ram in #455, I wonder if we can use a similar trick to conditionally include/remove the trampoline, which at least wouldn't be a breaking change (unlike just removing it entirely). |
|
You're right, I appear to have mistaken a PR merge icon for an Issue Closed icon! My mistake. And, yeah, I assume someone out there is using it, so removing it entirely seems aggressive. (Though I'm not sure who's using it, and removing it is of course consistent with my bias toward simpler single-purpose crates.) |
|
Hi, so was some agreement reached regarding what to do with this issue? I am facing a situation where the current implementation is not suitable (i.e. I need to mutate the ExceptionFrame so that the PC points to the instruction that comes after the one that triggered the exception, as well as having access to the SP at the moment the HardFault is entered so I can pop the EXC_RETURN). For my case, either the implementation would need to be slightly modified, or as suggested, this should be made optional / removed from the crate, so I can use a custom handler... |
Currently, any user of this crate has to pay for a small Assembly trampoline that provides access to the
&ExceptionFrameto theHardFaulthandler (even if they don't use the&ExceptionFrame, or don't even define anyHardFaulthandler):https://github.com/rust-embedded/cortex-m-rt/blob/492c7bf6212486b378ecf0518572d180a50c83a1/asm.s#L8-L19
Since few people make use of the
&ExceptionFrameargument, we could add a Cargo feature to turn it on instead, and generate the right linker script inbuild.rs. By forwarding the feature to the macro crate,#[exception]can provide good diagnostics when trying to use&ExceptionFramewithout the feature enabled.I am planning on extending the trampoline mechanism to other faults and maybe even all exceptions, which I've prototyped by making the ASM trampoline generic and have it work for all faults. This makes it larger and adds more data to the excutable, so making this opt-in would help reduce the cost. The trampoline code could also benefit from ARMv7/v8-specific improvements, which I'll work on later, which should shrink it again (for the higher platforms at least).
I suppose we could also have one Cargo feature per exception. The build script would then generate a linker script that pulls in exactly the trampolines that are needed. This should even still work with the precompiled
.afiles, if we do it right (putting every trampoline in its own section and making sure they're only pulled in when necessary).Initially, defining 10 or 11 Cargo features for this sounds pretty sketchy, but most apps won't use any of them, and even "advanced" apps that implement an RTOS can get away with just 1 or 2 (one for SVCall to read syscall arguments, and one on HardFault/DefaultHandler for debugging crashes).
An ideal solution would not require Cargo features at all and instead make the
#[exception]macro pull in the Assembly trampoline. I have not found any way to do this though. If one does come up (perhaps using the currently unstable#[link_args]attribute), we can make the Cargo features no-ops until the next major version.The text was updated successfully, but these errors were encountered: