Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc: Tweak #[target_feature] syntax #47223

Merged
merged 2 commits into from
Jan 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,10 @@ define_dep_nodes!( <'tcx>
[] Null,

[] SubstituteNormalizeAndTestPredicates { key: (DefId, &'tcx Substs<'tcx>) },

[input] TargetFeaturesWhitelist,
[] TargetFeaturesEnabled(DefId),

);

trait DepNodeParams<'a, 'gcx: 'tcx + 'a, 'tcx: 'a> : fmt::Debug {
Expand Down
66 changes: 38 additions & 28 deletions src/librustc/hir/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@
//! conflicts between multiple such attributes attached to the same
//! item.

use session::Session;
use ty::TyCtxt;

use syntax::ast;
use syntax::visit;
use syntax::visit::Visitor;
use hir;
use hir::intravisit::{self, Visitor, NestedVisitorMap};

#[derive(Copy, Clone, PartialEq)]
enum Target {
Expand All @@ -30,45 +29,51 @@ enum Target {
}

impl Target {
fn from_item(item: &ast::Item) -> Target {
fn from_item(item: &hir::Item) -> Target {
match item.node {
ast::ItemKind::Fn(..) => Target::Fn,
ast::ItemKind::Struct(..) => Target::Struct,
ast::ItemKind::Union(..) => Target::Union,
ast::ItemKind::Enum(..) => Target::Enum,
hir::ItemFn(..) => Target::Fn,
hir::ItemStruct(..) => Target::Struct,
hir::ItemUnion(..) => Target::Union,
hir::ItemEnum(..) => Target::Enum,
_ => Target::Other,
}
}
}

struct CheckAttrVisitor<'a> {
sess: &'a Session,
struct CheckAttrVisitor<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
}

impl<'a> CheckAttrVisitor<'a> {
impl<'a, 'tcx> CheckAttrVisitor<'a, 'tcx> {
/// Check any attribute.
fn check_attributes(&self, item: &ast::Item, target: Target) {
fn check_attributes(&self, item: &hir::Item, target: Target) {
self.tcx.target_features_enabled(self.tcx.hir.local_def_id(item.id));

for attr in &item.attrs {
if let Some(name) = attr.name() {
if name == "inline" {
self.check_inline(attr, item, target)
}
}
}

self.check_repr(item, target);
}

/// Check if an `#[inline]` is applied to a function.
fn check_inline(&self, attr: &ast::Attribute, item: &ast::Item, target: Target) {
fn check_inline(&self, attr: &hir::Attribute, item: &hir::Item, target: Target) {
if target != Target::Fn {
struct_span_err!(self.sess, attr.span, E0518, "attribute should be applied to function")
struct_span_err!(self.tcx.sess,
attr.span,
E0518,
"attribute should be applied to function")
.span_label(item.span, "not a function")
.emit();
}
}

/// Check if the `#[repr]` attributes on `item` are valid.
fn check_repr(&self, item: &ast::Item, target: Target) {
fn check_repr(&self, item: &hir::Item, target: Target) {
// Extract the names of all repr hints, e.g., [foo, bar, align] for:
// ```
// #[repr(foo)]
Expand Down Expand Up @@ -144,7 +149,7 @@ impl<'a> CheckAttrVisitor<'a> {
}
_ => continue,
};
struct_span_err!(self.sess, hint.span, E0517,
struct_span_err!(self.tcx.sess, hint.span, E0517,
"attribute should be applied to {}", allowed_targets)
.span_label(item.span, format!("not {} {}", article, allowed_targets))
.emit();
Expand All @@ -154,32 +159,37 @@ impl<'a> CheckAttrVisitor<'a> {
if (int_reprs > 1)
|| (is_simd && is_c)
|| (int_reprs == 1 && is_c && is_c_like_enum(item)) {
// Just point at all repr hints. This is not ideal, but tracking precisely which ones
// are at fault is a huge hassle.
// Just point at all repr hints. This is not ideal, but tracking
// precisely which ones are at fault is a huge hassle.
let spans: Vec<_> = hints.iter().map(|hint| hint.span).collect();
span_warn!(self.sess, spans, E0566,
span_warn!(self.tcx.sess, spans, E0566,
"conflicting representation hints");
}
}
}

impl<'a> Visitor<'a> for CheckAttrVisitor<'a> {
fn visit_item(&mut self, item: &'a ast::Item) {
impl<'a, 'tcx> Visitor<'tcx> for CheckAttrVisitor<'a, 'tcx> {
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::None
}

fn visit_item(&mut self, item: &'tcx hir::Item) {
let target = Target::from_item(item);
self.check_attributes(item, target);
visit::walk_item(self, item);
intravisit::walk_item(self, item);
}
}

pub fn check_crate(sess: &Session, krate: &ast::Crate) {
visit::walk_crate(&mut CheckAttrVisitor { sess: sess }, krate);
pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
let mut checker = CheckAttrVisitor { tcx };
tcx.hir.krate().visit_all_item_likes(&mut checker.as_deep_visitor());
}

fn is_c_like_enum(item: &ast::Item) -> bool {
if let ast::ItemKind::Enum(ref def, _) = item.node {
fn is_c_like_enum(item: &hir::Item) -> bool {
if let hir::ItemEnum(ref def, _) = item.node {
for variant in &def.variants {
match variant.node.data {
ast::VariantData::Unit(_) => { /* continue */ }
hir::VariantData::Unit(_) => { /* continue */ }
_ => { return false; }
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/librustc/ty/maps/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,12 @@ impl<'tcx> QueryDescription<'tcx> for queries::substitute_normalize_and_test_pre
}
}

impl<'tcx> QueryDescription<'tcx> for queries::target_features_whitelist<'tcx> {
fn describe(_tcx: TyCtxt, _: CrateNum) -> String {
format!("looking up the whitelist of target features")
}
}

macro_rules! impl_disk_cacheable_query(
($query_name:ident, |$key:tt| $cond:expr) => {
impl<'tcx> QueryDescription<'tcx> for queries::$query_name<'tcx> {
Expand Down
9 changes: 9 additions & 0 deletions src/librustc/ty/maps/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,11 @@ define_maps! { <'tcx>

[] fn substitute_normalize_and_test_predicates:
substitute_normalize_and_test_predicates_node((DefId, &'tcx Substs<'tcx>)) -> bool,

[] fn target_features_whitelist:
target_features_whitelist_node(CrateNum) -> Rc<FxHashSet<String>>,
[] fn target_features_enabled: TargetFeaturesEnabled(DefId) -> Rc<Vec<String>>,

}

//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -508,3 +513,7 @@ fn substitute_normalize_and_test_predicates_node<'tcx>(key: (DefId, &'tcx Substs
-> DepConstructor<'tcx> {
DepConstructor::SubstituteNormalizeAndTestPredicates { key }
}

fn target_features_whitelist_node<'tcx>(_: CrateNum) -> DepConstructor<'tcx> {
DepConstructor::TargetFeaturesWhitelist
}
3 changes: 3 additions & 0 deletions src/librustc/ty/maps/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,9 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
}
DepKind::IsTranslatedFunction => { force!(is_translated_function, def_id!()); }
DepKind::OutputFilenames => { force!(output_filenames, LOCAL_CRATE); }

DepKind::TargetFeaturesWhitelist => { force!(target_features_whitelist, LOCAL_CRATE); }
DepKind::TargetFeaturesEnabled => { force!(target_features_enabled, def_id!()); }
}

true
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_driver/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,6 @@ pub fn compile_input(sess: &Session,
Ok(()));
}

time(sess.time_passes(), "attribute checking", || {
hir::check_attr::check_crate(sess, &expanded_crate);
});

let opt_crate = if control.keep_ast {
Some(&expanded_crate)
} else {
Expand Down Expand Up @@ -1038,6 +1034,10 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(control: &CompileController,
// tcx available.
rustc_incremental::dep_graph_tcx_init(tcx);

time(sess.time_passes(), "attribute checking", || {
hir::check_attr::check_crate(tcx)
});

time(time_passes,
"stability checking",
|| stability::check_unstable_api_usage(tcx));
Expand Down
122 changes: 109 additions & 13 deletions src/librustc_trans/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,18 @@
//! Set and unset common attributes on LLVM values.

use std::ffi::{CStr, CString};
use std::rc::Rc;

use rustc::hir::Unsafety;
use rustc::hir::def_id::{DefId, LOCAL_CRATE};
use rustc::session::config::Sanitizer;
use rustc::ty::TyCtxt;
use rustc::ty::maps::Providers;
use rustc_data_structures::fx::FxHashSet;

use llvm::{self, Attribute, ValueRef};
use llvm::AttributePlace::Function;
use llvm_util;
pub use syntax::attr::{self, InlineAttr};
use syntax::ast;
use context::CrateContext;
Expand Down Expand Up @@ -94,23 +101,16 @@ pub fn set_probestack(ccx: &CrateContext, llfn: ValueRef) {

/// Composite function which sets LLVM attributes for function depending on its AST (#[attribute])
/// attributes.
pub fn from_fn_attrs(ccx: &CrateContext, attrs: &[ast::Attribute], llfn: ValueRef) {
pub fn from_fn_attrs(ccx: &CrateContext, llfn: ValueRef, id: DefId) {
use syntax::attr::*;
inline(llfn, find_inline_attr(Some(ccx.sess().diagnostic()), attrs));
let attrs = ccx.tcx().get_attrs(id);
inline(llfn, find_inline_attr(Some(ccx.sess().diagnostic()), &attrs));

set_frame_pointer_elimination(ccx, llfn);
set_probestack(ccx, llfn);
let mut target_features = vec![];
for attr in attrs {
if attr.check_name("target_feature") {
if let Some(val) = attr.value_str() {
for feat in val.as_str().split(",").map(|f| f.trim()) {
if !feat.is_empty() && !feat.contains('\0') {
target_features.push(feat.to_string());
}
}
}
} else if attr.check_name("cold") {

for attr in attrs.iter() {
if attr.check_name("cold") {
Attribute::Cold.apply_llfn(Function, llfn);
} else if attr.check_name("naked") {
naked(llfn, true);
Expand All @@ -123,6 +123,8 @@ pub fn from_fn_attrs(ccx: &CrateContext, attrs: &[ast::Attribute], llfn: ValueRe
unwind(llfn, false);
}
}

let target_features = ccx.tcx().target_features_enabled(id);
if !target_features.is_empty() {
let val = CString::new(target_features.join(",")).unwrap();
llvm::AddFunctionAttrStringValue(
Expand All @@ -134,3 +136,97 @@ pub fn from_fn_attrs(ccx: &CrateContext, attrs: &[ast::Attribute], llfn: ValueRe
fn cstr(s: &'static str) -> &CStr {
CStr::from_bytes_with_nul(s.as_bytes()).expect("null-terminated string")
}

pub fn provide(providers: &mut Providers) {
providers.target_features_whitelist = |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
Rc::new(llvm_util::target_feature_whitelist(tcx.sess)
.iter()
.map(|c| c.to_str().unwrap().to_string())
.collect())
};

providers.target_features_enabled = |tcx, id| {
let whitelist = tcx.target_features_whitelist(LOCAL_CRATE);
let mut target_features = Vec::new();
for attr in tcx.get_attrs(id).iter() {
if !attr.check_name("target_feature") {
continue
}
if let Some(val) = attr.value_str() {
for feat in val.as_str().split(",").map(|f| f.trim()) {
if !feat.is_empty() && !feat.contains('\0') {
target_features.push(feat.to_string());
}
}
let msg = "#[target_feature = \"..\"] is deprecated and will \
eventually be removed, use \
#[target_feature(enable = \"..\")] instead";
tcx.sess.span_warn(attr.span, &msg);
continue
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
from_target_feature(tcx, attr, &whitelist, &mut target_features);
}
Rc::new(target_features)
};
}

fn from_target_feature(
tcx: TyCtxt,
attr: &ast::Attribute,
whitelist: &FxHashSet<String>,
target_features: &mut Vec<String>,
) {
let list = match attr.meta_item_list() {
Some(list) => list,
None => {
let msg = "#[target_feature] attribute must be of the form \
#[target_feature(..)]";
tcx.sess.span_err(attr.span, &msg);
return
}
};

for item in list {
if !item.check_name("enable") {
let msg = "#[target_feature(..)] only accepts sub-keys of `enable` \
currently";
tcx.sess.span_err(item.span, &msg);
continue
}
let value = match item.value_str() {
Some(list) => list,
None => {
let msg = "#[target_feature] attribute must be of the form \
#[target_feature(enable = \"..\")]";
tcx.sess.span_err(item.span, &msg);
continue
}
};
let value = value.as_str();
for feature in value.split(',') {
if whitelist.contains(feature) {
target_features.push(format!("+{}", feature));
continue
}

let msg = format!("the feature named `{}` is not valid for \
this target", feature);
let mut err = tcx.sess.struct_span_err(item.span, &msg);

if feature.starts_with("+") {
let valid = whitelist.contains(&feature[1..]);
if valid {
err.help("consider removing the leading `+` in the feature name");
}
}
err.emit();
}
}
}
3 changes: 1 addition & 2 deletions src/librustc_trans/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ pub fn get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
if instance.def.is_inline(tcx) {
attributes::inline(llfn, attributes::InlineAttr::Hint);
}
let attrs = instance.def.attrs(ccx.tcx());
attributes::from_fn_attrs(ccx, &attrs, llfn);
attributes::from_fn_attrs(ccx, llfn, instance.def.def_id());

let instance_def_id = instance.def_id();

Expand Down
1 change: 1 addition & 0 deletions src/librustc_trans/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ impl rustc_trans_utils::trans_crate::TransCrate for LlvmTransCrate {
back::symbol_names::provide(providers);
back::symbol_export::provide(providers);
base::provide(providers);
attributes::provide(providers);
}

fn provide_extern(providers: &mut ty::maps::Providers) {
Expand Down
Loading