calling convention for MSP430 interrupts

[japaric]

This calling convention is used to define interrup handlers on MSP430 microcontrollers. Usage looks like this:

#[no_mangle]
#[link_section = "__interrupt_vector_10"]
pub static TIM0_VECTOR: unsafe extern "msp430-interrupt" fn() = tim0;

unsafe extern "msp430-interrupt" fn tim0() {
  P1OUT.write(0x00);
}

which generates the following assembly:

Disassembly of section __interrupt_vector_10:

0000fff2 <TIM0_VECTOR>:
    fff2:       10 c0           interrupt service routine at 0xc010

Disassembly of section .text:

0000c010 <_ZN3msp4tim017h3193b957fd6a4fd4E>:
    c010:       c2 43 21 00     mov.b   #0,     &0x0021 ;r3 As==00
    c014:       00 13           reti
        ...

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[petrochenkov]

This should probably be feature gated as well - https://github.com/rust-lang/rust/blob/master/src/libsyntax/feature_gate.rs#L963 (+ the match needs to be exhaustive so a new ABI is not missed unintentionally next time).

[japaric]

I was wondering if we had feature gates for new ABIs. Thanks for the heads up, @petrochenkov. I'll add the gate in a bit.

[alexcrichton]

Yeah we'll definitely want to gate this, but otherwise seems ok to me. In that sense that name shouldn't matter too much for now.

[pftbest]

I have tested this PR, and it works fine. This rust code:

#[no_mangle]
#[link_section = "__interrupt_vector_10"]
pub static TIM0_VECTOR: unsafe extern "msp430-interrupt" fn() = tim0;

unsafe extern "msp430-interrupt" fn tim0() {
  P1OUT.write(0x00);
}

Generates this assembly:

Disassembly of section __interrupt_vector_10:

0000fff2 <TIM0_VECTOR>:
    fff2:       10 c0           interrupt service routine at 0xc010

Disassembly of section .text:

0000c010 <_ZN3msp4tim017h3193b957fd6a4fd4E>:
    c010:       c2 43 21 00     mov.b   #0,     &0x0021 ;r3 As==00
    c014:       00 13           reti
        ...

It looks good to me.

[japaric]

@pftbest Excellent

r? @alexcrichton Added the feature gate. Should I create a tracking issue for the stability of the new ABI?

[alexcrichton]

Looks like the travis failure is legitimate, and could you also add a test to ensure this ABI is gated? And yeah let's add a tracking issue.

[petrochenkov]

could you also add a test to ensure this ABI is gated

(ABI stability tests are centralized in src\test\compile-fail\feature-gate-abi.rs)

[japaric]

@alexcrichton Updated. Still testing locally.

Thanks for the info, @petrochenkov!

[nagisa]

Okay, so if my memory doesn’t mislead me, we added #[naked] functions precisely so that interrupt ABIs wouldn’t proliferate. Perhaps it would make more sense to focus more effort on stabilising (and fixing) that approach than adding a new ABI?

[japaric]

Perhaps it would make more sense to focus more effort on stabilising (and fixing) that approach than adding a new ABI?

Before even considering using this ABI as an argument for stabilizing naked functions, we should first verify that this particular ABI can actually be implemented using naked functions (I don't know the answer). If that's the case, this unstable ABI could be removed in favor of that (but ergonomics should be considered before reaching a decision).

Personally, I'd prefer to land a sure and ergonomic (but unstable) way to write interrupt handlers for this architecture, this ABI, right now and then consider whether such ABI should be stabilized or removed/deprecated in favor of (stable) naked functions.

[eddyb]

TBH I would prefer an "interrupt" pseudo-ABI with platform-specific constraints that "does the right thing".

[pftbest]

@nagisa
Consider this code:

unsafe extern "msp430-interrupt" fn tim0() {
    not_inlined_foo();
}

I can write it using naked functions like this:

#[naked]
unsafe extern "C" fn tim1() {
    asm! {
        "
        push	r15
        push	r14
        push	r13
        push	r12
        "
    }
    not_inlined_foo();
    asm! {
        "
        pop	r12
        pop	r13
        pop	r14
        pop	r15
        reti
        "
    }
}

It does not look pretty but at least it works the same.
Now consider this code:

unsafe extern "msp430-interrupt" fn tim0() {
    P1OUT.write(0x00);
}

This code I can't write as a naked function, because I don't know how many registers I need to save for this inlined .write call. I can wrap the code into a new inline(never) function and save all registers like in the first case, but it will be a massive overhead for such tiny MCU.

Also we would like to have support for the __bic_SR_register_on_exit intrinsic in the future, and it has a restriction that it can only be used inside an isr handler, so maybe this ABI will help to check for that.

[bors]

☔ The latest upstream changes (presumably #38099) made this pull request unmergeable. Please resolve the merge conflicts.

[japaric]

rebased

@eddyb an "interrupt" ABI makes sense to me. I'd like to have at least one more interrupt-ish ABI (e.g. "avr-interrupt") before prototyping that; that way we can test in properly.

@pftbest those are my concerns exactly: ergonomics and error-proneness. In theory, it might be possible to create a syntax extension that implements "msp430-interrupt" on top of #[naked] but I don't know if that could have all the features that a real ABI enables (cf. the __bic_SR_register_on_exit intrinsic).

[nagisa]

This code I can't write as a naked function, because I don't know how many registers I need to save for this inlined .write call.

The C ABI for your platform should have somewhere a list of caller-saved (volatile) and callee-saved (non-volatile) registers. The saving and restoring as well as call overhead are unfortunate, though and this is the reason why an interrupt ABI is strictly better than naked functions.

That being said, I would still rather we only had either one (naked fns) or the other (interrupt ABIs). The issue with 2nd thing is that LLVM obviously does not support your arbitrary hardware interrupt ABI, which is why naked got accepted, consciously and instead of interrupt ABIs.

[pftbest]

@nagisa

The C ABI for your platform should have somewhere a list of caller-saved (volatile) and callee-saved (non-volatile) registers.

I was talking about it in my examples. If the code contains only call to non inlined function like in the first example, it is enough to save only caller-saved registers (r12-r15).
But if the code is inlined like in the second example it may be neccessary to save all the registers both volatile and non-volatile.

Some architectures do not have special ABI for interrupts, so they use naked functions. But for MSP430 it is available, and it would be unfortunate not being able to use it.

[pftbest]

Just to give a perspective on the overhead of using naked functions, I did a caculation.
All numbers are taked from the datasheet

If the code is using proposed ABI it will have
6 cycles to enter ISR and start running user code
5 cycles to exit ISR

If the code is using naked functions it will get this numbers:
22 cycles to enter ISR and start running user code
(6 + 3x4 for each push + 4 for call) and
16 cycles to exit ISR
(3 for return + 2x4 for pop + 5)

You can say that if you care about this numbers you should be writing in assembly, but I think It would be great if rust could be at least as efficient as C in this regard.

[bors]

☔ The latest upstream changes (presumably #38697) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #38482) made this pull request unmergeable. Please resolve the merge conflicts.

[whitequark]

@nagisa

The C ABI for your platform should have somewhere a list of caller-saved (volatile) and callee-saved (non-volatile) registers.

How is the C ABI (or any ABI) relevant here? An interrupt handler must save every register it uses, precisely because it is not invoked on a function boundary.

[nagisa]

@whitequark Since we’re talking trampolines here, you can avoid saving the non-volatile (aka. callee-saved) registers because no callee is allowed to modify them without also saving them beforehand and restoring register later, right?

The preserve_mostcc and preserve_allcc LLVM CCs are relevant.

---

Remember that I’m not arguing against interrupt ABIs. I’m arguing for having EITHER one or the other and not both. Honestly, though, I dislike naked functions and would rather have interrupt ABIs (having seen what mess #[naked] is and despite what I said back then), but RFC process has ruled otherwise.

[whitequark]

@whitequark Since we’re talking trampolines here, you can avoid saving the non-volatile (aka. callee-saved) registers because no callee is allowed to modify them without also saving them beforehand and restoring register later, right?

Ah, so what you're suggesting is a naked function that only contains a single call instruction, which calls the actual interrupt handler. I see how this may work.

Now, the $1M question: why not define the "interrupt" ABI to generate such a trampoline automatically?

[pftbest]

@nagisa What conflict do you see in having both of them? C has both and no one complained as far as I know. Besides, naked functions may have other uses apart from the interrupts, like JITs and green threading for example.

[alexcrichton]

er nvmd, I'll check it for you, you've reviewed

[alexcrichton]

@japaric I believe this is ready to go after a rebase now.

[eddyb]

r=me w/ rebase

[japaric]

@bors r=eddyb

yes, certainly. I was expecting a tracking issue for the relevant feature gate.

@nikomatsakis #38487

[bors]

📌 Commit 6296d52 has been approved by eddyb

[bors]

⌛ Testing commit 6296d52 with merge d7c4093...

[bors]

💔 Test failed - status-travis

[nikomatsakis]

@japaric

This failed because you have no test for this feature gate (it's a new requirement). The very helpful error message by @est31 explains what you have to do to fix it =)

Expected a gate test for the feature 'abi_msp430_interrupt'.

Hint: create a file named 'feature-gate-abi_msp430_interrupt.rs' in the compile-fail

      test suite, with its failures due to missing usage of

      #![feature(abi_msp430_interrupt)].

Hint: If you already have such a test and don't want to rename it,

      you can also add a // gate-test-abi_msp430_interrupt line to the test file.

Found 1 features without a gate test.

thread 'main' panicked at 'some tidy checks failed', /checkout/src/tools/tidy/src/main.rs:55

note: Run with `RUST_BACKTRACE=1` for a backtrace.

[japaric]

Thanks, @nikomatsakis. I had done that (the cfail test) for another PR but hadn't realize that it had to be done for this one too.

@bors r=eddyb

[bors]

📌 Commit e928c75 has been approved by eddyb

[bors]

⌛ Testing commit e928c75 with merge 8e29b4d...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 8e29b4d to master...