turn macros into attributes #90

japaric · 2018-08-18T20:48:30Z

This is a PoC implementation of RFC #82. Assuming that we are OK with this implementation we should
add some compile fail tests (e.g. using a function with the wrong signature as the entry point)
before landing this.

Look at the diff of the examples to get an overview of the changes.

cc @rust-embedded/cortex-m

japaric · 2018-08-18T20:56:30Z

These attributes can be used on the latest nightly without any feature gate (use_extern_macros was stabilized yesterday) and you will be able to use them on the next beta.

This is a breaking change because it removes the macros by example.

Instead of directly breaking the rt crate we could have an intermediate test phase where we first publish the rt-macros crate. Then people would be able to test out the attributes by changing their programs in the following way:

 #![no_main]
 #![no_std]

-#[macro_use(entry)]
 -extern crate cortex_m_rt as rt;
+extern crate cortex_m_rt_macros as rt;
 extern crate panic_semihosting;

-entry!(main);
+use rt::entry;

+#[entry]
 fn main() -> ! {
     loop {}
 }

Though I'm not sure if there's any advantage to doing the release that way.

japaric · 2018-08-18T20:59:48Z

macros/src/lib.rs

+                    pub fn #ident() {
+                        extern crate cortex_m_rt;
+
+                        cortex_m_rt::Exception::#name;


@adamgreig this is reason why an hypothetical #[interrupt] attribute would need the device crate name in its argument. We want to check that the interrupt exists and emit a compiler error if it doesn't and for that we need the full path to the Interrupt variant: e.g. stm32f103xx::Interrupt::EXTI0.

Makes sense. I guess we could require that whatever is passed in (to some future #[interrupt]) is resolvable to an Interrupt, so it could be either a full path, or the user could use stm32f103::Interrupt::* and then just call #[interrupt(EXTI0)]?

the user could use stm32f103::Interrupt::* and then just call #[interrupt(EXTI0)]?

If we omit the full path that would kind of defeat the purpose of the check. For example, this would compile:

struct InterruptThatDoesntExist; #[interrupt(InterruptThatDoesntExist)] fn handler() { /* .. */ }

Though thinking about it more having the user enter the name of the device crate makes it possible to nullify the check. For example,

extern crate stm32f103xx; // real device crate mod stm32f1337 { enum Interrupt { InterruptThatDoesntExist; } } #[interrupt(stm32f1337, InterruptThatDoesntExist)] fn handler() { /* .. */ }

japaric · 2018-08-18T21:02:40Z

examples/state.rs

-
-fn sys_tick(state: &mut u32) {
-    *state += 1;
+#[exception(SysTick, static STATE: u32 = 0)]


An alternative syntax would be to have the second argument be a string. i.e.

#[exception(SysTick, "static STATE: u32 = 0")] fn sys_tick() {}

But the current syntax seems to parse without problems and have better spans for error messages. e.g.

#[exception(SysTick, static STATE: u32 = true)] // ~~~~ error: expected `u32`, found `bool` fn sys_tick() {}

therealprof · 2018-08-18T21:04:26Z

@japaric Lovely, I like it!

japaric · 2018-08-18T21:11:25Z

Error messages are terrible by default because all the span information is lost. By this I mean that any compile error caused by the body of e.g. an exception function will be reported with the span of the attribute. Example:

#[entry]
// ~~~~~ error: mismatched types, expected `!`, found `()`
fn main() -> ! {
    asm::bkpt()
}

To improve you can opt into the nightly feature of the proc-macro2 crate like this:

# Cargo.toml
[build-dependencies.proc-macro2]
version = "0.4"
features = ["nightly"]

But, as the name of the feature implies, this requires a nightly compiler. The error message then becomes what you would expect:

#[entry]
fn main() -> ! {
    asm::bkpt()
//  ~~~~~~~~~~~ error: mismatched types, expected `!`, found `()`
}

I don't know if it's planned to stabilize these span improvements for the edition release.

japaric · 2018-08-31T00:07:38Z

I have rebased this PR.

@rust-embedded/cortex-m Please vote on RFC #82 by reviewing (approving) this PR.

Amendments to #82

The unresolved question about how to support state in the #[exception] attribute has been solved with the following syntax:

#[exception(SysTick, static STATE: u32 = 0)]
fn sys_tick() { // <- note that the function has no argument
    *STATE += 1; // but that `STATE` can be referenced only within this function
}

Also a reminder that both the proc_macro and the use_extern_macro features has been already stabilized in nightly so you'll be able to use these attributes in the next beta release (2 weeks from now) so I think it's OK to land this right now -- it'll be in a new minor version (v0.6.0) since it's a breaking change.

japaric · 2018-08-31T10:18:19Z

I just thought of an alternative syntax for adding state that may feel less strange:

#[exception(SysTick)]
fn sys_tick() {
    static mut STATE: u32 = 0;

    // user code
    *STATE += 1;
}

The attribute will magically make static mut variables safe (no unsafe) to access. It will also cause their type to change to &mut Type. The expansion would look like this:

#[export_name = "SysTick"]
pub fn sys_tick() {
    // <expansion>
    static mut STATE_: u32 = 0;

    let STATE: &mut u32 = unsafe { &mut STATE_ };
    // </expansion>

    // user code
    *STATE += 1;
}

japaric · 2018-08-31T10:29:39Z

And now I'm seeing a problem with the current implementation: it allows data races in safe code. For example:

#[exception(SysTick)]
fn sys_tick() {
    static mut STATE: u64 = 0;

    // user code
    *STATE += 1;
}

#[entry]
fn main() -> ! {
    loop {
        sys_tick();
    }
}

Here main and SysTick handler race for access to STATE -- this is UB.

The fix is to either (a) make sys_tick unsafe to call, or (b) make sys_tick impossible to call. If we do the latter then we can simplify the attribute a bit:

#[exception]
fn SysTick() {
    static mut STATE: u32 = 0;

    // user code
    *STATE += 1;
}

#[entry]
fn main() -> ! {
    loop {
        SysTick(); //~ error: this function doesn't exist
    }
}

We can use the function name as the exception name; that way we don't need an argument. The above code would expand to:

#[export_name = "SysTick"]
pub fn cf1dglqbt3jr7uiu() { // some randomly generated string that the user can't rely on
    // <expansion>
    static mut STATE_: u32 = 0;

    let STATE: &mut u32 = unsafe { &mut STATE_ };
    // </expansion>

    // user code
    *STATE += 1;
}

#[entry]
fn main() -> ! {
    SysTick(); // error: this function doesn't exist
}

korken89 · 2018-08-31T10:55:49Z

I'm not sure I'm a fan of "magically" making statics safe, but I also do not have any good input for a better solution, though the original idea was quite nice on the eyes and properly conveys that there is a state.
Why not keep this syntax? I can't quite see the issue with it.

japaric · 2018-08-31T11:06:13Z

@korken89 with the static in the attribute it's not clear that the static is only visible to the function or if it's in the global / outer scope since it's between those two spaces. Also there's no precedent for attributes that have statics as arguments so that syntax will look odd to most people.

With the static inside the function it's immediately clear that it can't be referred to from outside the function. I believe that making it safe to access it's not surprising exactly because it's local to the function.

korken89 · 2018-08-31T12:04:05Z

I see, and agree. Having the static in the function would indeed be cleaner.

therealprof · 2018-08-31T14:31:29Z

@japaric Yes, please! The local state definition for the interrupt handlers was very weird in the past and caused lots of weird syntactic problems with non-integral types which should be all gone with the new syntax. This is very similar to something a C/C++ programmer might do, too, so that's also a big plus. And last but not least this reduces the boilerplating quite a bit and makes things safer (by preventing calling an interrupt handler from another function) so goodies all around!

japaric · 2018-08-31T20:06:11Z

OK, I have made the exception handlers uncallable and also change the syntax of #[exception]: no arguments required and the state must now be declared as static mut variables at the beginning of the function body.

japaric · 2018-08-31T20:07:25Z

Also, we can use the static mut trick in #[entry] too. That will turn local static mut variables into &'static mut references. We would have to make the #[entry] function uncallable as well.

extend / update documentation

japaric · 2018-09-03T21:27:34Z

latest commit makes it safe to access local static mut variables in the entry point.

@therealprof @adamgreig @korken89 in #82 it sounded like you were in favor of this change. Could you confirm?

@thejpster @ithinuel I'd love to get your input on RFC #82 and this implementation. Are you in favor of that RFC or do you have concerns about it?

adamgreig · 2018-09-03T21:41:55Z

I really like the new no-argument attributes and the new syntax for state. Having mutable statics work in entry would solve a bunch of nuisance problems I've had in the past with initialising statics, for example setting up buffers for smoltcp. As far as I can tell this will actually be extremely useful for a lot of embedded development and maybe warrants a little more exposition in the documentation, but that can come later when people have a bit more experience with it.

👍 from me!

therealprof

Fantastic! 👍

korken89

Looks good, great work!

ithinuel

LGTM :) The macros are definitely fancier.

My only "concern" would be about the stability of proc-macro that I don't know. Other than that it looks great !

japaric · 2018-09-05T16:33:49Z

@ithinuel

The proc macros I'm using here are planned to be stable by the 2018 edition; they don't require feature gates in nightly; and they won't require features gates in RC1 (next beta) and RC2 (next next beta).

Alright, thanks for the input everyone. We have majority so this is officially accepted.

I'm going to add some compile fail tests then any of you can review and land this.

we'll test on beta when 1.30-beta is out

they are no longer required due to rust-lang/rust#52467

japaric · 2018-09-05T22:28:38Z

This should be ready to land, but while adding the compile-fail tests I noticed that we could relax some constraints. I'll leave some inline comments about that.

japaric · 2018-09-05T22:29:55Z

tests/compile-fail/default-handler-bad-signature-2.rs

+
+#[exception] //~ ERROR custom attribute panicked
+//~^ HELP `DefaultHandler` exception must have signature `fn(i16)`
+unsafe fn DefaultHandler(_irqn: i16) {}


Do we want to allow this? I don't see a problem with letting the user state this is unsafe because this function can't be called from software anyways.

👍 from me, no reason to not permit this, and I wouldn't be surprised if a lot of unsafe code in embedded projects is in the ISRs.

japaric · 2018-09-05T22:30:46Z

tests/compile-fail/default-handler-bad-signature-3.rs

+
+#[exception] //~ ERROR custom attribute panicked
+//~^ HELP `DefaultHandler` exception must have signature `fn(i16)`
+fn DefaultHandler(_irqn: i16) -> ! {


Do we want to allow this? I see no problem with letting this be a divergent function.

japaric · 2018-09-05T22:31:39Z

tests/compile-fail/entry-bad-signature-3.rs

+
+#[entry] //~ ERROR custom attribute panicked
+//~^ HELP `#[entry]` function must have signature `fn() -> !`
+unsafe fn foo() -> ! {


Do we want to allow this? Same rationale as allowing unsafe DefaultHandler.

japaric · 2018-09-05T22:32:10Z

tests/compile-fail/exception-bad-signature-2.rs

+
+#[exception] //~ ERROR custom attribute panicked
+//~^ HELP `#[exception]` functions other than `DefaultHandler` and `HardFault` must have signature `fn()`
+unsafe fn SysTick() {}


Do we want to allow this? Same rationale as unsafe DefaultHandler

japaric · 2018-09-05T22:32:22Z

tests/compile-fail/exception-bad-signature-3.rs

+
+#[exception] //~ ERROR custom attribute panicked
+//~^ HELP `#[exception]` functions other than `DefaultHandler` and `HardFault` must have signature `fn()`
+fn SysTick() -> ! {


Do we want to allow this? Same rationale as divergent DefaultHandler

japaric · 2018-09-05T22:32:42Z

tests/compile-fail/hard-fault-bad-signature-2.rs

+
+#[exception] //~ ERROR custom attribute panicked
+//~^ HELP `HardFault` exception must have signature `fn(&ExceptionFrame) -> !`
+unsafe fn HardFault(_ef: &ExceptionFrame) -> ! {


Do we want to allow this? Same rationale as unsafe DefaultHandler

adamgreig · 2018-09-05T22:52:23Z

Rather than commenting inline on each one: I'm happy to allow all of those unsafe/divergent functions. There's no safety reasons not to, so I'd rather let the user decide.

since we can't statically check that it holds

japaric · 2018-09-05T23:37:38Z

Ok, I have relaxed the signature checks and added more compile-fail tests and examples (which I'm using as compile-pass tests). I have also made the reachability requirement more prominent in the docs.

adamgreig · 2018-09-06T00:44:04Z

Looks great! I've gone through the new code and also tested this branch on a few small examples on hardware with exceptions and pre_init and everything seems to work as expected.

bors r+

90: turn macros into attributes r=adamgreig a=japaric This is a PoC implementation of RFC #82. Assuming that we are OK with this implementation we should add some compile fail tests (e.g. using a function with the wrong signature as the entry point) before landing this. Look at the diff of the examples to get an overview of the changes. cc @rust-embedded/cortex-m Co-authored-by: Jorge Aparicio <[email protected]>

bors · 2018-09-06T00:49:16Z

Build succeeded

continuous-integration/travis-ci/push

japaric requested a review from a team as a code owner August 18, 2018 20:48

japaric added the needs-decision label Aug 18, 2018

japaric mentioned this pull request Aug 18, 2018

[RFC] [breaking-change] Turn entry!, exception! into attributes #82

Closed

japaric added the breaking change label Aug 18, 2018

japaric commented Aug 18, 2018

View reviewed changes

turn macros into attributes

bc9499a

japaric force-pushed the attr branch from 72c2264 to bc9499a Compare August 30, 2018 23:59

japaric mentioned this pull request Aug 31, 2018

[RFC] #[ramfunc] #100

Open

japaric added 2 commits August 31, 2018 22:02

fix soundness issue; change the syntax of the exception attribute

1978f22

remove the now redundant -C linker=rust-lld argument

61212b5

japaric added 2 commits August 31, 2018 22:13

remove dead code

0e6f310

make static mut variables safe to access in the entry point

4974583

extend / update documentation

therealprof previously approved these changes Sep 3, 2018

View reviewed changes

korken89 previously approved these changes Sep 4, 2018

View reviewed changes

ithinuel previously approved these changes Sep 4, 2018

View reviewed changes

japaric added 2 commits September 6, 2018 00:25

add compile-fail tests; test only on nightly

8d675f8

we'll test on beta when 1.30-beta is out

remove #[allow]s

de35ebb

they are no longer required due to rust-lang/rust#52467

japaric dismissed stale reviews from ithinuel, korken89, and therealprof via de35ebb September 5, 2018 22:27

japaric changed the title ~~[DO NOT MERGE] turn macros into attributes~~ turn macros into attributes Sep 5, 2018

japaric removed the needs-decision label Sep 5, 2018

japaric commented Sep 5, 2018

View reviewed changes

japaric added 2 commits September 6, 2018 00:39

whoops, it seems like we do need some #[allow]s after all

70000a1

add a test / example about using static mut vars in the entry point

0608596

japaric added 4 commits September 6, 2018 01:04

add compile-fail tests for passing arguments to the attributes

5001bd0

relax checks of the signatures of entry and the exceptions

9124d0f

test that function with wrong return types are rejected

dc19f12

make the reachability requirement more **prominent**

82e985b

since we can't statically check that it holds

adamgreig approved these changes Sep 6, 2018

View reviewed changes

bors bot merged commit 82e985b into master Sep 6, 2018

bors bot deleted the attr branch September 6, 2018 00:49

adamgreig mentioned this pull request Sep 18, 2018

Regression on 0.6 on rust messages with attribute macro #125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

turn macros into attributes #90

turn macros into attributes #90

japaric commented Aug 18, 2018

japaric commented Aug 18, 2018

japaric Aug 18, 2018

adamgreig Aug 19, 2018

japaric Aug 19, 2018

japaric Aug 18, 2018

therealprof commented Aug 18, 2018

japaric commented Aug 18, 2018

japaric commented Aug 31, 2018

japaric commented Aug 31, 2018

japaric commented Aug 31, 2018

korken89 commented Aug 31, 2018

japaric commented Aug 31, 2018

korken89 commented Aug 31, 2018

therealprof commented Aug 31, 2018

japaric commented Aug 31, 2018

japaric commented Aug 31, 2018

japaric commented Sep 3, 2018

adamgreig commented Sep 3, 2018

therealprof left a comment

korken89 left a comment

ithinuel left a comment

japaric commented Sep 5, 2018

japaric commented Sep 5, 2018

japaric Sep 5, 2018

adamgreig Sep 5, 2018

japaric Sep 5, 2018

japaric Sep 5, 2018

japaric Sep 5, 2018

japaric Sep 5, 2018

japaric Sep 5, 2018

adamgreig commented Sep 5, 2018

japaric commented Sep 5, 2018

adamgreig commented Sep 6, 2018

bors bot commented Sep 6, 2018

turn macros into attributes #90

turn macros into attributes #90

Conversation

japaric commented Aug 18, 2018

japaric commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealprof commented Aug 18, 2018

japaric commented Aug 18, 2018

japaric commented Aug 31, 2018

japaric commented Aug 31, 2018

japaric commented Aug 31, 2018

korken89 commented Aug 31, 2018

japaric commented Aug 31, 2018

korken89 commented Aug 31, 2018

therealprof commented Aug 31, 2018

japaric commented Aug 31, 2018

japaric commented Aug 31, 2018

japaric commented Sep 3, 2018

adamgreig commented Sep 3, 2018

therealprof left a comment

Choose a reason for hiding this comment

korken89 left a comment

Choose a reason for hiding this comment

ithinuel left a comment

Choose a reason for hiding this comment

japaric commented Sep 5, 2018

japaric commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgreig commented Sep 5, 2018

japaric commented Sep 5, 2018

adamgreig commented Sep 6, 2018

bors bot commented Sep 6, 2018

Build succeeded