[RFC] revamp the exception / interrupt registration mechanism

[japaric]

What

I propose that we change our exception / interrupt registration mechanism to
look like this:

#![no_std]

#[macro_use(default_handler, exception)]
extern crate cortex_m;
extern crate cortex_m_rt;
#[macro_use(interrupt)]
extern crate stm32f103xx;

fn main() {}

default_handler!(default_handler);

fn default_handler() {
    cortex_m::asm::bkpt();
}

exception!(HARD_FAULT, fault::hard);

mod fault {
    pub fn hard() {
        ::cortex_m::asm::bkpt();
        ::cortex_m::asm::bkpt();
    }
}

exception!(SYS_TICK, sys_tick, local: {
    counter: u32 = 0;
});

fn sys_tick(local: &mut SYS_TICK::Locals) {
    local.counter += 1;
}

interrupt!(TIM2, tim2);

fn tim2() {}

interrupt!(TIM3, tim3, local: {
    counter: u32 = 0;
});

fn tim3(local: &mut TIM3::Locals) {
    local.counter += 1;
}

This produces:

Disassembly of section .vector_table:

08000000 <_svector_table>:
 8000000:       20005000

08000004 <cortex_m_rt::RESET_HANDLER>:
 8000004:       08000131

08000008 <cortex_m_rt::EXCEPTIONS>:
 8000008:       0800017f
 800000c:       08000183
 8000010:       0800017f
 8000014:       0800017f
 8000018:       0800017f
        ...
 800002c:       0800017f
        ...
 8000038:       0800017f
 800003c:       08000189

08000040 <_eexceptions>:
 8000040:       0800017f

(..)

0800017e <DEFAULT_HANDLER>:
 800017e:       be00            bkpt    0x0000
 8000180:       4770            bx      lr

08000182 <HARD_FAULT>:
 8000182:       be00            bkpt    0x0000
 8000184:       be00            bkpt    0x0000
 8000186:       4770            bx      lr

08000188 <SYS_TICK>:
 8000188:       f240 0000       movw    r0, #0
 800018c:       f2c2 0000       movt    r0, #8192       ; 0x2000
 8000190:       6801            ldr     r1, [r0, #0]
 8000192:       3101            adds    r1, #1
 8000194:       6001            str     r1, [r0, #0]
 8000196:       4770            bx      lr

08000198 <TIM2>:
 8000198:       4770            bx      lr

0800019a <TIM3>:
 800019a:       f240 0004       movw    r0, #4
 800019e:       f2c2 0000       movt    r0, #8192       ; 0x2000
 80001a2:       6801            ldr     r1, [r0, #0]
 80001a4:       3101            adds    r1, #1
 80001a6:       6001            str     r1, [r0, #0]
 80001a8:       4770            bx      lr

That is exceptions / interrupts can be overridden individually using the
exception! / interrupt! macro. And the default "catch everything" handler
can be overridden using the default_handler! macro.

Why

Pros

• This solves Local's borrow problem. It also makes using interrupt /
  exception local data much more ergonomic. No Local newtype or tokens; just
  direct access to the local data through the Locals struct.

• This solves Default handlers get mono-morphized needlessly #19. So no duplicated handlers. All the exceptions / interrupts
  handlers that have not been overridden point to the same default handler.

• Using breakpoints is more uniform: break HARD_FAULT works regardless of how
  the application is structured or whether is using RTFM (break app::main::INTERRUPTS::TIM2) or not.

• Less footgun-y. The current mechanism tries to be typed but requires the user
  to place a variable with the right type at the right linker section. There's
  no mechanism to prevent the user from placing wrong stuff in the right linker
  section by mistake. Example:

#[used]
#[link_section = ".vector_table.interrupts"]
static INTERRUPTS: exception::Handlers = exception::DEFAULT_HANDLERS;

• Better error messages. The compiler, rather than the linker, will report
  misuses of the exception! / interrupt! macros:

exception!(FOO, foo);
//~^ error: no associated item named `FOO` found for type
// | `cortex_m::exception::Exception` in the current scope

fn foo() {}

exception(HARD_FAULT, bar);
exception(HARD_FAULT, baz);
//~^ error: the name `HARD_FAULT` is defined multiple times

fn bar() {}
fn baz() {}

Cons

• Breaks the world. cortex-m, cortex-m-rt, cortex-m-quickstart, cortex-m-rtfm
  and svd2rust would suffer breaking changes.

• Requires that device crates provide an interrupts.x linker script (see the
  "how" section for details) so svd2rust would have to learn to generate that
  linker script. We can drop this requirement if we are OK with not fixing Default handlers get mono-morphized needlessly #19
  though (see alternatives).

How

The core idea is that, instead of requiring the user to place a variable /
struct at the right linker section, the cortex-m-rt and device crate will create
a vector table full of undefined symbols. The user can then override those
undefined symbols. The symbols that don't get overridden will resolve to a
default handler, which can also be overridden.

cortex-m-rt

This crate will place an array of undefined symbols in the
.vector_table.exceptions section.

extern "C" {
    fn BUS_FAULT();
    fn HARD_FAULT();
    fn MEM_MANAGE();
    fn NMI();
    fn PENDSV();
    fn SVCALL();
    fn SYS_TICK();
    fn USAGE_FAULT();
}

#[used]
#[link_section = ".vector_table.exceptions"]
static EXCEPTIONS: [Option<unsafe extern "C" fn()>; 14] = [
    Some(NMI),
    Some(HARD_FAULT),
    Some(MEM_MANAGE),
    Some(BUS_FAULT),
    Some(USAGE_FAULT),
    None,
    None,
    None,
    None,
    Some(SVCALL),
    None,
    None,
    Some(PENDSV),
    Some(SYS_TICK),
];

The crate will also provide a default handler in the form of a weak
DEFAULT_HANDLER symbol.

#[doc(hidden)]
#[linkage = "weak"]
#[naked]
#[no_mangle]
pub unsafe extern "C" fn DEFAULT_HANDLER() -> ! {
    // ..
}

Finally, the linker script provided by this crate will weakly bind the
undefined symbols to the DEFAULT_HANDLER symbol.

/* link.x */
PROVIDE(NMI = DEFAULT_HANDLER);
PROVIDE(HARD_FAULT = DEFAULT_HANDLER);
/* .. */

cortex-m

The whole ctxt module will be removed, this includes the Local abstraction
and the Context marker trait, as well as exception::default_handler,
exception::Handlers, exception::DEFAULT_HANDLERS and the exception "tokens"
(struct NMI { _0: () }).

This crate will provide a default_handler! and an exception! macros that
provide a type safe interface to override the default handler and an exception
handler respectively. The expansion of those macros is shown in the "expansion"
section.

svd2rust

svd2rust generated crates will no longer include interrupts tokens (struct TIM2 { _0: () }). They'll place an array of undefined symbols in the
.vector_table.interrupts section like cortex-m does:

#[link_section = ".vector_table.interrupts"]
#[used]
static INTERRUPTS: [Option<unsafe extern "C" fn()>; 60] =
    [
        Some(WWDG),
        Some(PVD),
        // ..
    ];

And a linker script interrupts.x that weakly binds those undefined symbols
to DEFAULT_HANDLER.

/* interrupts.x */
PROVIDE(WWDG = DEFAULT_HANDLER);
PROVIDE(PVD = DEFAULT_HANDLER);
/* .. */

As well as an interrupt! macro that allows individually overriding these
symbols. The expansion of that macro is shown in the next section.

Expansion

This is what the opening example expands to:

#![feature(prelude_import)]
#![no_std]
#![no_std]
#[prelude_import]
use core::prelude::v1::*;
#[macro_use]
extern crate core as core;

#[macro_use(default_handler, exception)]
extern crate cortex_m;
extern crate cortex_m_rt;
#[macro_use(interrupt)]
extern crate stm32f103xx;

fn main() {}

#[allow(non_snake_case)]
#[no_mangle]
pub extern "C" fn DEFAULT_HANDLER() {
    let f: fn() = default_handler;
    f();
}
fn default_handler() {
    cortex_m::asm::bkpt();
}
#[allow(non_snake_case)]
#[no_mangle]
pub extern "C" fn HARD_FAULT() {
    let _ = ::exception::Exception::HARD_FAULT;
    let f: fn() = fault::hard;
    f();
}
mod fault {
    pub fn hard() {
        ::cortex_m::asm::bkpt();
        ::cortex_m::asm::bkpt();
    }
}
#[allow(non_snake_case)]
mod SYS_TICK {
    pub struct Locals {
        pub counter: u32,
    }
}
#[allow(non_snake_case)]
#[no_mangle]
pub extern "C" fn SYS_TICK() {
    let _ = ::exception::Exception::SYS_TICK;
    static mut LOCALS: self::SYS_TICK::Locals =
        self::SYS_TICK::Locals { counter: 0 };
    let f: fn(&mut self::SYS_TICK::Locals) = sys_tick;
    f(unsafe { &mut LOCALS });
}
fn sys_tick(local: &mut SYS_TICK::Locals) {
    local.counter += 1;
}
#[allow(non_snake_case)]
#[no_mangle]
pub extern "C" fn TIM2() {
    let _ = ::interrupt::Interrupt::TIM2;
    let f: fn() = tim2;
    f();
}
fn tim2() {}
#[allow(non_snake_case)]
mod TIM3 {
    pub struct Locals {
        pub counter: u32,
    }
}
#[allow(non_snake_case)]
#[no_mangle]
pub extern "C" fn TIM3() {
    let _ = ::interrupt::Interrupt::TIM3;
    static mut LOCALS: self::TIM3::Locals = self::TIM3::Locals { counter: 0 };
    let f: fn(&'static mut self::TIM3::Locals) = tim3;
    f(unsafe { &mut LOCALS });
}
fn tim3(local: &'static mut TIM3::Locals) {
    local.counter += 1;
}

Unresolved questions

• Check that this works: have a dependency override an exception handler. Check
  that the top crate can't override the same handler. Check that the handler
  makes it to the final binary.

• fn SYS_TICK(local: &'static mut SYS_TICK::Locals) or fn SYS_TICK(local: &mut SYS_TICK::Locals) for the signature of exceptions / interrupts?. &mut T and &'static mut T are not equivalent. The latter has a bit stronger
  ownership semantics. See below:

fn SYS_TICK(local: &'static mut SYS_TICK::Locals) {
    foo(local); // this reborrows `local` for the duration of `foo`
    foo(local); // OK

    foo_static(local); // this is semantically a move, not a reborrow
    foo_static(local); // that's why `local` can't be accessed afterwards
    //~^ error: cannot borrow `*local` as mutable more than once at a time
}

fn foo<T>(_: &mut T) {}
fn foo_static<T>(_: &'static mut T) {}

The reason I think we should not use &'static mut here is that some people
may create APIs that take a &'static mut argument assuming move semantics as
in the receiver of the &'static mut assumes it has exclusive access to the
static variable for the rest of the program. The problem is that exceptions
can be called more than once so something like this:

fn SYS_TICK(local: &'static mut SYS_TICK::Locals) {
    take_ownership(local);
}

fn take_ownership<T>(t: &'static mut T) {
    // and do something with it
}

will effectively be mutably aliasing the LOCALS static variable every time the
SYS_TICK exception is called.

Alternatives

• We could move the default_handler! and exception! macros to the
  cortex-m-rt crate.

I'd prefer to keep the rt crate API-less. That way it's more likely that it will
only appear as a direct dependency of the top crate.

The downside of having those macros in the cortex-m crate is that if the
application doesn't link to the cortex-m-rt then the default_handler! and
exception! macros have no effect. The same applies to the interrupt! macros
in all the device crates though.

• We could drop the interrupts.x requirement by having the cortex-m-rt and the
  device crates provide default weak implementations for their exceptions /
  interrupts in their source code. Something like this:

#[linkage = "weak"]
#[no_mangle]
extern "C" fn NMI() {
    loop {}
}

#[linkage = "weak"]
#[no_mangle]
extern "C" fn HARD_FAULT() {
    loop {}
}

// ..

#[used]
#[link_section = ".vector_table.exceptions"]
static EXCEPTIONS: [Option<unsafe extern "C" fn()>; 14] = [
    Some(NMI),
    Some(HARD_FAULT),
    Some(MEM_MANAGE),
    Some(BUS_FAULT),
    Some(USAGE_FAULT),
    None,
    None,
    None,
    None,
    Some(SVCALL),
    None,
    None,
    Some(PENDSV),
    Some(SYS_TICK),
];

However this brings back #19 :-(

08000008 <cortex_m_rt::EXCEPTIONS>:
 8000008:       08000195
 800000c:       08000197
 8000010:       08000199
 8000014:       0800019b
 8000018:       0800019d
        ...
 800002c:       0800019f
        ...
 8000038:       080001a1
 800003c:       080001a3

(..)

08000194 <NMI>:
 8000194:       e7fe            b.n     8000194 <NMI>

08000196 <HARD_FAULT>:
 8000196:       e7fe            b.n     8000196 <HARD_FAULT>

08000198 <MEM_MANAGE>:
 8000198:       e7fe            b.n     8000198 <MEM_MANAGE>

0800019a <BUS_FAULT>:
 800019a:       e7fe            b.n     800019a <BUS_FAULT>

0800019c <USAGE_FAULT>:
 800019c:       e7fe            b.n     800019c <USAGE_FAULT>

0800019e <SVCALL>:
 800019e:       e7fe            b.n     800019e <SVCALL>

080001a0 <PENDSV>:
 80001a0:       e7fe            b.n     80001a0 <PENDSV>

080001a2 <SYS_TICK>:
 80001a2:       e7fe            b.n     80001a2 <SYS_TICK>

Implementation bits

• cortex-m: this PR
• cortex-m-rt
• svd2rust
• cortex-m-quickstart TODO
• cortex-m-rtfm TODO

cc @whitequark @pftbest

[whitequark]

I like the proposal a lot! My opinions:

• Breakage is a-ok.
• interrupts.x is a-ok and svd2rust should be taught to generate it.
• Locals could not possibly be a &'static mut because of what you listed, but we also have another issue: what happens if we have a reentrant exception handler? There is nothing in principle prohibiting that right now, critical sections handle it.
• The question of where default_handler! and exception! should go is a question of ownership not convenience. Who is responsible for providing the interface that default_handler! and exception! create? The cortex-m crate knows absolutely nothing about memory layouts or linker scripts so it's not the place for it. Conversely, cortex-m-rt dictates that interface. I would argue that device crates, e.g. my tm4c129x crate, should both depend on cortex-m-rt, export the interrupt! macro, and re-export the default_handler! and exception! macro. (tm4c129x could have a default feature that enables this, and disabling this feature would kill both the cortex-m-rt dependency and the export of the interrupt! macro, in principle, if people eventually want even more control over the memory layout).
• I think we should fix Default handlers get mono-morphized needlessly #19. It might not be that much of bloat but it genuinely complicates reading assembly and, perhaps most importantly, it leaves a rather bad impression.

[whitequark]

Oh but one thing is we can simply fix #19 in LLVM, see my comment: #19 (comment). It's really not a big deal and if push comes to shove I'll write that patch I guess. So whether or not any change discussed here has any effect on #19 should not affect our decision on this issue.

[pftbest]

However this brings back #19 :-(

@japaric have you tried something like this? https://is.gd/6IqMsG

[whitequark]

Come on this is a straightforward LLVM fix, let's not add global_asm! to the mix

[pftbest]

@whitequark I don't like having yet another *.x file, so any working fix is good to me.

[japaric]

@whitequark

what happens if we have a reentrant exception handler?

Do you mean that you see a problem with using &mut Locals? Or do you mean that as an argument for using &mut Locals over &'static mut Locals?

The question of where default_handler! and exception! should go is a question of ownership not convenience. (..)

Makes sense. I agree with the argument and the re-exports in the device crate part.

I think we should fix #19. (..) and, perhaps most importantly, it leaves a rather bad impression.

nods

@pftbest

have you tried something like this?

Actually, yes. LLVM yells at me (LLVM assertion) when I use that, override any exception and use LTO (it works fine without LTO). Some "assertion: symbol SYS_TICK is variable" nonsense IIRC. I can post the exact error message later.

I don't like having yet another *.x file

Me neither. I would prefer to avoid it but also want to fix #19.

[whitequark]

Do you mean that you see a problem with using &mut Locals?

I thought I did but, nevermind, I misunderstood a few things on how NVIC and exception handling worked.

[japaric]

@pftbest This is the error message / LLVM assertion I mentioned:

rustc: /checkout/src/llvm/lib/MC/MCStreamer.cpp:294: virtual void llvm::MCStreamer::EmitLabel(llvm::MCSymbol*): Assertion `!Symbol->isVariable() && "Cannot emit a variable symbol!"' failed.

It happens if I remove the PROVIDE(..) stuff from the linker script, add the global_asm! block with the .weakref directives as you have shown, use exception! at least once in the top crate and compile the crate with LTO. The LTO part is important: there's no assertion if I don't use LTO. I suppose that LLVM doesn't like it when it has to generate an object file that has both an undefined reference to a symbol and the required symbol, which is what happens in the LTO case where only one object file is produced.

I'll see what changes are required to get rid of interrupts.x. I'll assume that #19 will get fixed some other way.

[japaric]

I'll see what changes are required to get rid of interrupts.x

Changing:

extern "C" {
    fn USAGE_FAULT();
    fn HARD_FAULT();
    ..
}

to

#[allow(non_snake_case)]
#[allow(private_no_mangle_fns)]
#[linkage = "weak"]
#[naked]
#[no_mangle]
extern "C" fn USAGE_FAULT() {
    unsafe {
        asm!("b DEFAULT_HANDLER");
        ::core::intrinsics::unreachable()
    }
}

#[allow(non_snake_case)]
#[allow(private_no_mangle_fns)]
#[linkage = "weak"]
#[naked]
#[no_mangle]
extern "C" fn HARD_FAULT() {
    unsafe {
        asm!("b DEFAULT_HANDLER");
        ::core::intrinsics::unreachable()
    }
}

// ..

seems to be enough to get rid of interrupts.x and seems to preserve semantics. Disassembly looks like:

08000090 <NMI>:
 8000090:       f000 b80e       b.w     80000b0 <DEFAULT_HANDLER>

08000094 <HARD_FAULT>:
 8000094:       f000 b80c       b.w     80000b0 <DEFAULT_HANDLER>

08000098 <MEM_MANAGE>:
 8000098:       f000 b80a       b.w     80000b0 <DEFAULT_HANDLER>

0800009c <BUS_FAULT>:
 800009c:       f000 b808       b.w     80000b0 <DEFAULT_HANDLER>

080000a0 <USAGE_FAULT>:
 80000a0:       f000 b806       b.w     80000b0 <DEFAULT_HANDLER>

080000a4 <SVCALL>:
 80000a4:       f000 b804       b.w     80000b0 <DEFAULT_HANDLER>

080000a8 <PENDSV>:
 80000a8:       f000 b802       b.w     80000b0 <DEFAULT_HANDLER>

080000ac <SYS_TICK>:
 80000ac:       f000 b800       b.w     80000b0 <DEFAULT_HANDLER>

080000b0 <DEFAULT_HANDLER>:
 80000b0:       f3ef 8008       mrs     r0, MSP
 80000b4:       f000 b800       b.w     80000b8 <cortex_m_rt::default_handler>

080000b8 <cortex_m_rt::default_handler>:
 80000b8:       be00            bkpt    0x0000
 80000ba:       e7fe            b.n     80000ba <cortex_m_rt::default_handler+0x2>

Are we OK with the tradeoff? Replacing interrupts.x by #19 like bloat.

[whitequark]

@japaric I just wrote the LLVM patch for #19. There is no tradeoff.

[japaric]

Thanks @whitequark. Is the patch published somewhere? I'd like to test it locally.

[japaric]

When the processor takes an exception, unless the exception is a tail-chained or a late-arriving exception, the processor pushes information onto the current stack. This operation is referred to as stacking and the structure of eight data words is referred as the stack frame.

(emphasis not mine)

Source

But the documentation also calls it "exception frame", so maybe ExceptionFrame would be less ambiguous?

[whitequark]

Oh, that's pretty ambiguous on their part. I like ExceptionFrame.

[whitequark]

@japaric see #19 (comment)

[japaric]

Thanks for the patch, @whitequark. I tested it but didn't solve the problem. Maybe because the mergefunc pass is not enabled by default? I used --release ( -C opt-level=3 -C lto).

Then I tried to recompile with RUSTFLAGS="-C passes=mergefunc" but recompiling core failed with an LLVM assertion:

$ RUSTFLAGS="-C passes=mergefunc" xargo build --release -v
+ "rustc" "--print" "sysroot"
+ "rustc" "--print" "target-list"
+ "cargo" "build" "--release" "--manifest-path" "/tmp/xargo.Og0mapmWRPsU/Cargo.toml" "--target" "thumbv7m-none-eabi" "-v" "-p" "core"
   Compiling core v0.0.0 (file:///shared/rust/master/src/libcore)
     Running `rustc --crate-name core /shared/rust/master/src/libcore/lib.rs --crate-type lib --emit=dep-info,link -C opt-level=3 -C debuginfo=2 -C metadata=cc33b668ba0ee1fd -C extra-filename=-cc33b668ba0ee1fd --out-dir /tmp/xargo.Og0mapmWRPsU/target/thumbv7m-none-eabi/release/deps --target thumbv7m-none-eabi -L dependency=/tmp/xargo.Og0mapmWRPsU/target/thumbv7m-none-eabi/release/deps -L dependency=/tmp/xargo.Og0mapmWRPsU/target/release/deps -C passes=mergefunc --sysroot /home/japaric/.xargo -Z force-unstable-if-unmarked`
rustc: /shared/rust/master/src/llvm/include/llvm/IR/ValueMap.h:275: void llvm::ValueMapCallbackVH<KeyT, ValueT, Config>::allUsesReplacedWith(llvm::Value*) [with KeyT = llvm::GlobalValue*; ValueT = long unsigned int; Config = llvm::GlobalNumberState::Config]: Assertion `isa<KeySansPointerT>(new_key) && "Invalid RAUW on key of ValueMap<>"' failed.
error: Could not compile `core`.

This is the patch I applied. Hopefully I didn't mess it up.

[whitequark]

@japaric Thaaat is interesting; the patch passed the entire LLVM testsuite (but not bootstrap tests). Can you please call that cargo command with --emit llvm-ir and attach the output? I already know what the issue is, but I don't want to rebuild rustc myself.

[japaric]

@whitequark Interestingly if I compile core with -C passes=mergefunc for x86_64-unknown-linux-gnu then I don't get any assertion. I get the assertion when cross compiling for thumbv7m-none-eabi. As for using --emit=llvm-ir it seems that the build crashes before rustc can emit any .ll file :-/

Here's the IR for the command cargo rustc --target x86_64-unknown-linux-gnu --manifest-path /shared/rust/master/src/libcore/Cargo.toml --release -- --emit=llvm-ir -C passes=mergefunc which doesn't crash.

[japaric]

If it's of any help, here's the IR for the test project I have been using compiled without and with -C passes=mergefunc (applied only at "the top" with cargo rustc because RUSTFLAGS=... recompiles core and hits the assertion I mentioned)

[whitequark]

As for using --emit=llvm-ir it seems that the build crashes before rustc can emit any .ll file :-/

I mean, of course it does if you keep -C passes=mergefunc on the command line... For the same reason the x86_64 IR is not useful to me. Please capture the IR without the mergefunc pass.

[japaric]

@whitequark core.ll

(I wonder how is this different from generating the IR using an unpatched nightly given that the mergefunc pass is not enabled.)

[whitequark]

@japaric facedesk sorry. I have completely missed that I couldn't do that.

[japaric]

I'm going to land this and all the pieces in other repos to ease exploring a solution for #50. I'm going to cut a release yet so feel free to comment on this or on the other PRs if you have any concern.

@homunkulus r+

[homunkulus]

📌 Commit 6c642b7 has been approved by japaric

[homunkulus]

⌛ Testing commit 6c642b7 with merge 6c642b7...

[homunkulus]

☀️ Test successful - status-travis
Approved by: japaric
Pushing 6c642b7 to master...