rustc: Tweak `#[target_feature]` syntax #47223

alexcrichton · 2018-01-05T23:43:33Z

This is an implementation of the #[target_feature] syntax-related changes of
RFC 2045. Notably two changes have been implemented:

The new syntax is #[target_feature(enable = "..")] instead of
#[target_feature = "+.."]. The enable key is necessary instead of the +
to indicate that a feature is being enabled, and a sub-list is used for
possible expansion in the future. Additionally within this syntax the feature
names being enabled are now whitelisted against a known set of target feature
names that we know about.
The #[target_feature] attribute can only be applied to unsafe functions. It
was decided in the RFC that invoking an instruction possibly not defined for
the current processor is undefined behavior, so to enable this feature for now
it requires an unsafe intervention.

rust-highfive · 2018-01-05T23:43:36Z

r? @eddyb

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

eddyb · 2018-01-05T23:46:38Z

src/librustc/ty/maps/mod.rs

@@ -360,6 +360,9 @@ define_maps! { <'tcx>
    // however, which uses this query as a kind of cache.
    [] fn erase_regions_ty: erase_regions_ty(Ty<'tcx>) -> Ty<'tcx>,
    [] fn fully_normalize_monormophic_ty: normalize_ty_node(Ty<'tcx>) -> Ty<'tcx>,
+
+    [] fn target_features_whitelist: target_features_whitelist_node(CrateNum) -> Rc<Vec<String>>,
+    [] fn target_features_enabled: TargetFeaturesEnabled(DefId) -> Rc<Vec<String>>,


Could you maybe use BTreeSet<Symbol>? Or is that too much of a microoptimization? (I was thinking we could speed up checking for a specific target feature that way)

Realistically there will be at most a dozen or so target features, I wouldn't expect a b-tree to beat linear search for such small collections.

If we really cared about performance we would use a binary search into a sorted Vec.

eddyb · 2018-01-05T23:47:06Z

cc @nagisa @nikomatsakis

eddyb · 2018-01-06T02:52:41Z

src/librustc_trans/attributes.rs

+            if tcx.fn_sig(id).unsafety() == Unsafety::Normal {
+                let msg = "#[target_feature(..)] can only be applied to \
+                           `unsafe` function";
+                tcx.sess.span_err(attr.span, msg);


I almost missed this: it's a very bad idea to have these checks here (as opposed to rustc_typeck) because they'd only trigger on monomorphizations.

Do you have a suggestion for where this might go? This is the same bug about validating #[inline(never)] and such I think? (in the sense that this isn't the first of it's kind and it's not clear if there's an obvious way to fix this)

inline and some aspects of repr (whether they are applied to the right kind of item, there are further checks in rustc_typeck) are checked in rustc::hir::check_attr. It seems these checks would fit in nicely there, since they doen't seem to need any type information.

Thanks for the pointer! Needed a bit of refactoring but it fits there now.

nagisa · 2018-01-06T11:37:00Z

This PR seems fine to me, other than the small nit that I find it weird that the implementation of target_feature(disable) was omitted.

bors · 2018-01-08T07:14:33Z

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

alexcrichton · 2018-01-08T17:27:08Z

@nagisa disabling features was explicitly mentioned as a future feature, so it's not implemented here (as it's more conservative to not implement it yet).

alexcrichton · 2018-01-10T14:59:25Z

r? @eddyb

eddyb · 2018-01-10T15:51:12Z

@bors r+

bors · 2018-01-10T15:51:13Z

📌 Commit 40a4131 has been approved by eddyb

bors · 2018-01-13T11:50:24Z

⌛ Testing commit 40a41316d1fb9d9da9409e7527fd00fedd752d46 with merge 235732a61138adc2c7118c9275a3f2fb1e45deb3...

bors · 2018-01-13T12:42:29Z

💔 Test failed - status-travis

kennytm · 2018-01-13T17:32:50Z

src/test/ui/target-feature-wrong.rs

+// except according to those terms.
+
+// ignore-arm
+// ignore-aarch64


Needs ignore-wasm here. Actually I think all non-x86 architectures need to be ignored.

[00:47:27] failures: [00:47:27] [00:47:27] ---- [ui] ui/target-feature-wrong.rs stdout ---- [00:47:27] diff of stderr: [00:47:27] [00:47:27] 28 26 | #[target_feature(enable = "sse2")] [00:47:27] 29 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [00:47:27] 30 [00:47:27] + error: the feature named `sse2` is not valid for this target [00:47:27] + --> $DIR/target-feature-wrong.rs:26:18 [00:47:27] + | [00:47:27] + 26 | #[target_feature(enable = "sse2")] [00:47:27] + | ^^^^^^^^^^^^^^^ [00:47:27] + [00:47:27] + error: aborting due to 5 previous errors [00:47:27] - error: aborting due to 4 previous errors [00:47:27] 32 [00:47:27] 33

This is an implementation of the `#[target_feature]` syntax-related changes of [RFC 2045][rfc]. Notably two changes have been implemented: * The new syntax is `#[target_feature(enable = "..")]` instead of `#[target_feature = "+.."]`. The `enable` key is necessary instead of the `+` to indicate that a feature is being enabled, and a sub-list is used for possible expansion in the future. Additionally within this syntax the feature names being enabled are now whitelisted against a known set of target feature names that we know about. * The `#[target_feature]` attribute can only be applied to unsafe functions. It was decided in the RFC that invoking an instruction possibly not defined for the current processor is undefined behavior, so to enable this feature for now it requires an `unsafe` intervention. [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/2045-target-feature.md

alexcrichton · 2018-01-13T18:08:22Z

@bors: r=eddyb

bors · 2018-01-13T18:08:23Z

📌 Commit 03d76e6 has been approved by eddyb

bors · 2018-01-13T21:07:04Z

⌛ Testing commit 03d76e67ee47f6c0642bf6b9f725d1909106170e with merge d48ae78bcb4de40f94288ee34fef5f3cddd9151c...

bors · 2018-01-13T21:48:10Z

💔 Test failed - status-travis

This'll enable running queries that could be cached and overall be more amenable to the query infastructure.

alexcrichton · 2018-01-14T00:07:24Z

@bors: r=eddyb

bors · 2018-01-14T00:07:25Z

📌 Commit 0ecaa67 has been approved by eddyb

bors · 2018-01-14T03:27:41Z

⌛ Testing commit 0ecaa67 with merge b762c2d...

rustc: Tweak `#[target_feature]` syntax This is an implementation of the `#[target_feature]` syntax-related changes of [RFC 2045][rfc]. Notably two changes have been implemented: * The new syntax is `#[target_feature(enable = "..")]` instead of `#[target_feature = "+.."]`. The `enable` key is necessary instead of the `+` to indicate that a feature is being enabled, and a sub-list is used for possible expansion in the future. Additionally within this syntax the feature names being enabled are now whitelisted against a known set of target feature names that we know about. * The `#[target_feature]` attribute can only be applied to unsafe functions. It was decided in the RFC that invoking an instruction possibly not defined for the current processor is undefined behavior, so to enable this feature for now it requires an `unsafe` intervention. [rfc]: https://github.com/rust-lang/rfcs/blob/master/text/2045-target-feature.md

bors · 2018-01-14T06:09:09Z

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

@alexcrichton

whitelist x86 fxsr feature rust-lang#47223 properly checks that only white-listed features are allowed in combination with `target_feature`, but the `fxsr` feature used by `stdsimd` was not white-listed. r? @alexcrichton

rust-highfive assigned eddyb Jan 5, 2018

eddyb reviewed Jan 5, 2018

View reviewed changes

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2018

eddyb reviewed Jan 6, 2018

View reviewed changes

alexcrichton force-pushed the new-target-feature branch 3 times, most recently from dc2b1e7 to 40a4131 Compare January 9, 2018 15:31

alexcrichton mentioned this pull request Jan 10, 2018

incr.comp.: Turn translation-related attributes into a query. #47320

Closed

kennytm reviewed Jan 13, 2018

View reviewed changes

alexcrichton force-pushed the new-target-feature branch from 40a4131 to 03d76e6 Compare January 13, 2018 18:08

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 13, 2018

rustc: Refactor attribute checking to operate on HIR

0ecaa67

This'll enable running queries that could be cached and overall be more amenable to the query infastructure.

alexcrichton force-pushed the new-target-feature branch from 03d76e6 to 0ecaa67 Compare January 14, 2018 00:07

bors merged commit 0ecaa67 into rust-lang:master Jan 14, 2018

bors mentioned this pull request Jan 14, 2018

rustc_trans: reorganize CrateContext and rename context types. #47209

Merged

This was referenced Jan 17, 2018

Tracking issue for cfg_target_feature #29717

Closed

whitelist x86 fxsr feature #47514

Merged

alexcrichton deleted the new-target-feature branch January 23, 2018 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Tweak `#[target_feature]` syntax #47223

rustc: Tweak `#[target_feature]` syntax #47223

alexcrichton commented Jan 5, 2018

rust-highfive commented Jan 5, 2018

eddyb Jan 5, 2018

hanna-kruppe Jan 5, 2018

retep998 Jan 6, 2018

eddyb commented Jan 5, 2018

eddyb Jan 6, 2018

alexcrichton Jan 8, 2018

hanna-kruppe Jan 8, 2018

alexcrichton Jan 8, 2018

nagisa commented Jan 6, 2018

bors commented Jan 8, 2018

alexcrichton commented Jan 8, 2018

alexcrichton commented Jan 10, 2018

eddyb commented Jan 10, 2018

bors commented Jan 10, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

kennytm Jan 13, 2018

alexcrichton commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

alexcrichton commented Jan 14, 2018

bors commented Jan 14, 2018

bors commented Jan 14, 2018

bors commented Jan 14, 2018

rustc: Tweak #[target_feature] syntax #47223

rustc: Tweak #[target_feature] syntax #47223

Conversation

alexcrichton commented Jan 5, 2018

rust-highfive commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jan 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

nagisa commented Jan 6, 2018

bors commented Jan 8, 2018

alexcrichton commented Jan 8, 2018

alexcrichton commented Jan 10, 2018

eddyb commented Jan 10, 2018

bors commented Jan 10, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

Choose a reason for hiding this comment

alexcrichton commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

bors commented Jan 13, 2018

alexcrichton commented Jan 14, 2018

bors commented Jan 14, 2018

bors commented Jan 14, 2018

bors commented Jan 14, 2018

rustc: Tweak `#[target_feature]` syntax #47223

rustc: Tweak `#[target_feature]` syntax #47223