Added so attributes and cfgs are applied on the trampolines as well by korken89 · Pull Request #228 · rust-embedded/cortex-m-rt

korken89 · 2019-12-28T14:36:17Z

I made a first go at #223, but maybe this implementation takes things we do not want to propagate.
If you come up with something that should not propagate, please comment here.

Closes #223
Closes #213

rust-highfive · 2019-12-28T14:36:27Z

r? @thejpster

(rust_highfive has picked a reviewer for you, use r? to override)

korken89 · 2019-12-28T16:58:08Z

bors try

bors · 2019-12-28T17:09:56Z

try

Build succeeded

continuous-integration/travis-ci/push

jonas-schievink · 2019-12-28T19:25:03Z

This will also apply attributes like #[export_name] and #[inline], which doesn't make too much sense to me. I think the most robust approach would be to use a whitelist of attributes that are propagated to the trampoline. I'll start working on one.

korken89 · 2019-12-28T20:10:46Z

Sure, should be no problem to add.
But not a blacklist? Seems like doc, inline, and export_name are the direct ones we do not want.

korken89 · 2019-12-28T20:34:39Z

@jonas-schievink I added a blacklist, works as expected. Are there more we would like to blacklist?
Or do you have a specific whitelist I should change to?

jonas-schievink

The blacklist approach should also handle #[cfg_attr(some_cfg, blacklisted_attr)].

It's not strictly necessary to forbid #[inline] (it shouldn't have any effect), but #[no_mangle] might be an issue. #[must_use] might also be a problem since it might emit an unavoidable warning, and lint attributes should not be propagated or they might warn for code in the trampoline.

macros/src/lib.rs

korken89 · 2019-12-28T21:23:37Z

Fixed your comments.

The blacklist approach should also handle #[cfg_attr(some_cfg, blacklisted_attr)].

This is already handled and I tested it as well :)

korken89 · 2019-12-29T09:41:07Z

Also, I will rebase and squash this before we merge this

korken89 · 2019-12-29T11:19:02Z

Tests added to CI as well

korken89 · 2019-12-29T12:53:37Z

There, I think this should be ready now

thejpster · 2019-12-29T18:21:00Z

Handing to @jonas-schievink as this is a bit out of my comfort zone.

jonas-schievink

Thanks! I think the tests can be merge since they just test individual blacklist entries. It would also be good to test the #[cfg_attr] case.

I'm still not convinced of the blacklist approach though. Rust itself and other crates can add attributes at any point, which might interact badly with cortex-m-rt's, so I'd prefer a whitelist instead. It's also unclear to me if these are all the problematic attributes. For example, what about lint-controlling attributes? We might want to put them only on the user function and not the trampoline. Most of the builtin attributes are at best useless, and may cause very bad compiler errors (what about #[test]?), and I don't think there's any use case for using other proc macro attributes.

macros/src/lib.rs

korken89 · 2019-12-29T18:56:12Z

Sure, we can switch to a whitelist as well.
What more than doc and link_section should be allowed?

jonas-schievink

We also need to allow cfg and cfg_attr (and add a test for them). The lint-controlling attributes definitely shouldn't be disallowed either (like I mentioned above), that is allow, warn, deny and forbid. #[cold] should also be allowed, it can potentially affect optimization choices.

If this list is complete then these should be all.

macros/src/lib.rs

korken89 · 2020-01-07T20:18:49Z

Updated per your comments, ready for another review

macros/src/lib.rs

jonas-schievink

Thanks, looks good to me now!

Does someone else from @rust-embedded/cortex-m have opinions on this? This is also a breaking change, so we should probably hold off on merging this until we have a more clear plan for the next cortex-m-rt version.

jonas-schievink · 2020-01-08T00:16:00Z

Oh one more thing, does this happen to also fix #213 by any chance? I'm not sure if proc. macro attributes can see other proc. macro attributes, but if so this should fail the whitelist check (and if not that issue can be closed as wontfix).

korken89 · 2020-01-08T07:35:51Z

Sure! I added a test for that and it does catch this issue as well.

korken89 · 2020-01-08T10:54:35Z

Hmm, I should probably look into a better error messages for these

jonas-schievink

r=me unless you still want to improve the diagnostics

korken89 · 2020-01-16T11:08:16Z

I will have a look at the message, should go fast

korken89 · 2020-01-16T11:58:08Z

@jonas-schievink I added better diagnostics, please have a look again and then we can merge

macros/src/lib.rs

korken89 · 2020-01-16T12:03:07Z

Sounds good, I committed it

Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>

macros/src/lib.rs

# Please enter the commit message for your changes. Lines starting Formating fix

jonas-schievink

Thanks, this is great!

macros/src/lib.rs

korken89 · 2020-01-16T16:59:03Z

Very true, updated and removed blacklist

jonas-schievink · 2020-01-16T17:42:34Z

bors r+

228: Added so attributes and cfgs are applied on the trampolines as well r=jonas-schievink a=korken89 I made a first go at #223, but maybe this implementation takes things we do not want to propagate. If you come up with something that should not propagate, please comment here. Closes #223 Closes #213 Co-authored-by: Emil Fresk <emil.fresk@gmail.com>

bors · 2020-01-16T17:51:48Z

Build succeeded

continuous-integration/travis-ci/push

korken89 requested a review from a team as a code owner December 28, 2019 14:36

rust-highfive assigned thejpster Dec 28, 2019

rust-highfive added S-waiting-on-review T-cortex-m labels Dec 28, 2019

korken89 mentioned this pull request Dec 28, 2019

Fixed changed compile-fail messages #229

Merged

bors bot added a commit that referenced this pull request Dec 28, 2019

Try #228:

b28bbb2

jonas-schievink reviewed Dec 28, 2019

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

macros/src/lib.rs Outdated Show resolved Hide resolved

korken89 requested a review from jonas-schievink December 29, 2019 11:18

korken89 force-pushed the attrs_cfgs_to_trampoline branch from b49af40 to 5a878d0 Compare December 29, 2019 12:52

Added blacklisted attributes

5a878d0

thejpster assigned jonas-schievink and unassigned thejpster Dec 29, 2019

jonas-schievink reviewed Dec 29, 2019

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

korken89 added 2 commits December 29, 2019 20:05

Changed to whitelist

48ac9a9

Fixed logic error

ff7bfee

jonas-schievink suggested changes Dec 30, 2019

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

macros/src/lib.rs Outdated Show resolved Hide resolved

jonas-schievink added the breaking change label Jan 6, 2020

Updated per comments

35c3735

jonas-schievink reviewed Jan 7, 2020

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

Reduced number of whitelist checks

45f8f55

jonas-schievink previously approved these changes Jan 7, 2020

View reviewed changes

korken89 dismissed jonas-schievink’s stale review via 2e17995 January 8, 2020 07:35

Added test for double attribute

2e17995

jonas-schievink previously approved these changes Jan 16, 2020

View reviewed changes

korken89 dismissed jonas-schievink’s stale review via d779bdd January 16, 2020 11:57

Better error message on multiple attributes

d779bdd

jonas-schievink reviewed Jan 16, 2020

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

korken89 force-pushed the attrs_cfgs_to_trampoline branch from 8b2732c to 44985bc Compare January 16, 2020 12:06

Better error message

44985bc

Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>

jonas-schievink reviewed Jan 16, 2020

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

Even better error messages

cd219c4

# Please enter the commit message for your changes. Lines starting Formating fix

jonas-schievink reviewed Jan 16, 2020

View reviewed changes

macros/src/lib.rs Outdated Show resolved Hide resolved

macros/src/lib.rs Outdated Show resolved Hide resolved

Updated to not need blacklist

941ed5b

jonas-schievink approved these changes Jan 16, 2020

View reviewed changes

bors bot merged commit 941ed5b into master Jan 16, 2020

bors bot deleted the attrs_cfgs_to_trampoline branch January 16, 2020 17:51

Conversation

korken89 commented Dec 28, 2019 • edited by jonas-schievink Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 28, 2019

Uh oh!

korken89 commented Dec 28, 2019

Uh oh!

bors bot commented Dec 28, 2019

try

Build succeeded

Uh oh!

jonas-schievink commented Dec 28, 2019

Uh oh!

korken89 commented Dec 28, 2019

Uh oh!

korken89 commented Dec 28, 2019

Uh oh!

jonas-schievink left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

korken89 commented Dec 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

korken89 commented Dec 29, 2019

Uh oh!

korken89 commented Dec 29, 2019

Uh oh!

korken89 commented Dec 29, 2019

Uh oh!

thejpster commented Dec 29, 2019

Uh oh!

jonas-schievink left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

korken89 commented Dec 29, 2019

Uh oh!

jonas-schievink left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

korken89 commented Jan 7, 2020

Uh oh!

Uh oh!

jonas-schievink left a comment

Choose a reason for hiding this comment

Uh oh!

jonas-schievink commented Jan 8, 2020

Uh oh!

korken89 commented Jan 8, 2020

Uh oh!

korken89 commented Jan 8, 2020

Uh oh!

jonas-schievink left a comment

Choose a reason for hiding this comment

Uh oh!

korken89 commented Jan 16, 2020

Uh oh!

korken89 commented Jan 16, 2020

Uh oh!

Uh oh!

korken89 commented Jan 16, 2020

Uh oh!

Uh oh!

jonas-schievink left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

korken89 commented Jan 16, 2020

Uh oh!

jonas-schievink commented Jan 16, 2020

Uh oh!

bors bot commented Jan 16, 2020

Build succeeded

Uh oh!

korken89 commented Dec 28, 2019 •

edited by jonas-schievink

Loading

korken89 commented Dec 28, 2019 •

edited

Loading