Hardfault trampoline is now optional

[diondokter]

Fixes #406

This was less work than I thought! (So please check if I didn't miss anything haha)

Basically, anything that works with the hardfault trampoline is cfg'ed behind the new feature flag.
I've opted to make the flag default to avoid breaking changes... (unless somebody has default-features = false in their Cargo.toml). So technically it's still a breaking change I think?

Also, we probably want to have a test for this all. There seems to be a test for the trampoline version.
But I don't really know how testing in this repo is set up.

[Dirbaio]

I think it should be some argument to the macro, instead of a Cargo feature. Something like #[exception(trampoline = true)]

The problem with Cargo features is that one crate might do #[exception] fn HardFault() expecting either trampoline or no trampoline, and another crate can break it by setting/unsetting the feature.

Also, are the current trampoline and ExceptionFrame useful in practice? The use cases I can think of are

• Crash dumps: print all registers
• RTOS-like context switching: read and write all registers.

ExceptionFrame only has R0-R4, so you still have to use ASM for R5-R13. So if you're going to use ASM anyway, you might as well do everything with ASM, so you have more control and can optimize it more. Maybe we should completely remove ExceptionFrame and the trampoline? Or expand it to cover all regs?

[diondokter]

I think it should be some argument to the macro, instead of a Cargo feature. Something like #[exception(trampoline = true)]

This could be done technically. Though that would pull some code from the crate into the macro. Not very nice for readability and maintainability.

The problem with Cargo features is that one crate might do #[exception] fn HardFault() expecting either trampoline or no trampoline, and another crate can break it by setting/unsetting the feature.

Since there can only be one handler anyways, only the crate that implements the handler cares about the feature being enabled. So I don't think this'll be a problem in practice.

Also, are the current trampoline and ExceptionFrame useful in practice? The use cases I can think of are

* Crash dumps: print all registers

* RTOS-like context switching: read and write all registers.

ExceptionFrame only has R0-R4, so you still have to use ASM for R5-R13. So if you're going to use ASM anyway, you might as well do everything with ASM, so you have more control and can optimize it more. Maybe we should completely remove ExceptionFrame and the trampoline? Or expand it to cover all regs?

I don't know how people use it. I do know that removing it would be a breaking change.

[diondokter]

Ok, so after the weekly meeting we decided to look more into the direction that Dario proposed. I'll describe how that is supposed to work.

So, the goal is to be backwards compatible so we can avoid bumping the major version.
Old code should provide the same API.

This should still compile:

#[exception]
fn HardFault(e: ExceptionFrame) {
    // ...
}

It will turn into:

#[exception(trampoline = true)]
fn HardFault(e: ExceptionFrame) {
    // ...
}

This way we can set the trampoline to false if we don't want a trampoline to be generated. For the next major version we could consider making false the default. In principle we could extend the ExceptionFrame to all registers in a minor release, but that's better suited for a different PR.

But what does this mean for the code?
There are a couple of things that together make the hardfault handler and (optional) trampoline.

1. The vector table needs to be populated with a default handler, the trampoline or the user handler
2. The trampoline must be generated in the macro if set to true. This is a global_asm! piece.
3. The macro must make sure that the function signature is correct.

All in all it isn't that much.
One extra possibility is that we simply make macro not have any parameters, but generate a trampoline if the user uses the ExceptionFrame as a parameter of the handler. I think it's more elegant at the cost of being less explicit.

[jonathanpallant]

Explicit over Implicit

[diondokter]

Explicit over Implicit

Yeah I'm leaning to making it explicit too. Just wanted to mention the possibility.

[diondokter]

Ok, the trampoline generation is done in the macro now.
It's probably a good idea to add a test somewhere, but I have trouble seeing where would be a good place. Any ideas?

[adamgreig]

Thanks, looks good! I was trying to follow through how the trampoline works now and I'm not sure when HardFaultTrampoline (the asm routine) actually gets called. The __EXCEPTIONS table refers to a HardFault symbol now (instead of the previous HardFaultTrampoline), but it seems that will cause the macro-generated #tramp_ident function to be called directly (since it has export_name="HardFault"), which then calls the user-provided handler.

Should the asm routine actually be called HardFault, and rename the export_name of the macro-generated function, and have the asm routine branch to that new name?

[diondokter]

Thanks, looks good! I was trying to follow through how the trampoline works now and I'm not sure when HardFaultTrampoline (the asm routine) actually gets called. The __EXCEPTIONS table refers to a HardFault symbol now (instead of the previous HardFaultTrampoline), but it seems that will cause the macro-generated #tramp_ident function to be called directly (since it has export_name="HardFault"), which then calls the user-provided handler.

Should the asm routine actually be called HardFault, and rename the export_name of the macro-generated function, and have the asm routine branch to that new name?

Oh I think you may be right. I'll investigate and fix

[diondokter]

Ok, it passes again!

It should all check out now. And I've confirmed it by looking at the exception frame values, they are very similar to what I get in the last release.

I've added a dummy function to improve diagnostics. Otherwise when you have two hardfaults, of which one uses the trampoline and one doesn't, you'd get a horribly long error about assembly code. The only real cost here is that the diagnostics now talk about _HardFault instead of HardFault.

If naked functions were stabilized yet, then I could've used that and get rid of _HardFault, but alas.

[adamgreig]

Thanks for updating that! I've reviewed the changes and run some tests and the generated output all seems good.

The docs need updating in a couple of places and then I'm happy to merge this:

• https://github.com/rust-embedded/cortex-m/blob/master/cortex-m-rt/src/lib.rs#L228-L233 the symbol names have changed and the description needs tweaking slightly
• https://github.com/rust-embedded/cortex-m/blob/master/cortex-m-rt/src/lib.rs#L752-L754 the required signature has changed

[diondokter]

@adamgreig Good catch! I've changed and added to the docs in the last commit. I think that should cover everything and it's technically more correct than the old docs.

[adamgreig]

thank you!

[diondokter]

thank you!

And thank you 😄