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

Associated functions that contain extern indicator or have #[rustc_std_internal_symbol] are reachable #86492

Merged
merged 4 commits into from Aug 13, 2021
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 compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1499,6 +1499,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}

fn visit_assoc_item(&mut self, item: &'a AssocItem, ctxt: AssocCtxt) {
if self.session.contains_name(&item.attrs, sym::no_mangle) {
self.check_nomangle_item_asciionly(item.ident, item.span);
}

if ctxt == AssocCtxt::Trait || !self.in_trait_impl {
self.check_defaultness(item.span, item.kind.defaultness());
}
Expand Down
86 changes: 64 additions & 22 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,25 @@ impl EarlyLintPass for UnsafeCode {
}
}

fn check_impl_item(&mut self, cx: &EarlyContext<'_>, it: &ast::AssocItem) {
if let ast::AssocItemKind::Fn(..) = it.kind {
if let Some(attr) = cx.sess().find_by_name(&it.attrs, sym::no_mangle) {
self.report_overriden_symbol_name(
cx,
attr.span,
"declaration of a `no_mangle` method",
);
}
if let Some(attr) = cx.sess().find_by_name(&it.attrs, sym::export_name) {
self.report_overriden_symbol_name(
cx,
attr.span,
"declaration of a method with `export_name`",
);
}
}
}

fn check_fn(&mut self, cx: &EarlyContext<'_>, fk: FnKind<'_>, span: Span, _: ast::NodeId) {
if let FnKind::Fn(
ctxt,
Expand Down Expand Up @@ -1115,31 +1134,37 @@ declare_lint_pass!(InvalidNoMangleItems => [NO_MANGLE_CONST_ITEMS, NO_MANGLE_GEN
impl<'tcx> LateLintPass<'tcx> for InvalidNoMangleItems {
fn check_item(&mut self, cx: &LateContext<'_>, it: &hir::Item<'_>) {
let attrs = cx.tcx.hir().attrs(it.hir_id());
let check_no_mangle_on_generic_fn = |no_mangle_attr: &ast::Attribute,
impl_generics: Option<&hir::Generics<'_>>,
generics: &hir::Generics<'_>,
span| {
for param in
generics.params.iter().chain(impl_generics.map(|g| g.params).into_iter().flatten())
{
match param.kind {
GenericParamKind::Lifetime { .. } => {}
GenericParamKind::Type { .. } | GenericParamKind::Const { .. } => {
cx.struct_span_lint(NO_MANGLE_GENERIC_ITEMS, span, |lint| {
lint.build("functions generic over types or consts must be mangled")
.span_suggestion_short(
no_mangle_attr.span,
"remove this attribute",
String::new(),
// Use of `#[no_mangle]` suggests FFI intent; correct
// fix may be to monomorphize source by hand
Applicability::MaybeIncorrect,
)
.emit();
});
break;
}
}
}
};
match it.kind {
hir::ItemKind::Fn(.., ref generics, _) => {
if let Some(no_mangle_attr) = cx.sess().find_by_name(attrs, sym::no_mangle) {
for param in generics.params {
match param.kind {
GenericParamKind::Lifetime { .. } => {}
GenericParamKind::Type { .. } | GenericParamKind::Const { .. } => {
cx.struct_span_lint(NO_MANGLE_GENERIC_ITEMS, it.span, |lint| {
lint.build(
"functions generic over types or consts must be mangled",
)
.span_suggestion_short(
no_mangle_attr.span,
"remove this attribute",
String::new(),
// Use of `#[no_mangle]` suggests FFI intent; correct
// fix may be to monomorphize source by hand
Applicability::MaybeIncorrect,
)
.emit();
});
break;
}
}
}
check_no_mangle_on_generic_fn(no_mangle_attr, None, generics, it.span);
}
}
hir::ItemKind::Const(..) => {
Expand Down Expand Up @@ -1170,6 +1195,23 @@ impl<'tcx> LateLintPass<'tcx> for InvalidNoMangleItems {
});
}
}
hir::ItemKind::Impl(hir::Impl { ref generics, items, .. }) => {
for it in items {
if let hir::AssocItemKind::Fn { .. } = it.kind {
if let Some(no_mangle_attr) = cx
.sess()
.find_by_name(cx.tcx.hir().attrs(it.id.hir_id()), sym::no_mangle)
{
check_no_mangle_on_generic_fn(
no_mangle_attr,
Some(generics),
cx.tcx.hir().get_generics(it.id.def_id.to_def_id()).unwrap(),
it.span,
);
}
}
}
}
_ => {}
}
}
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_lint/src/nonstandard_style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,9 +391,14 @@ impl<'tcx> LateLintPass<'tcx> for NonSnakeCase {
_: Span,
id: hir::HirId,
) {
let attrs = cx.tcx.hir().attrs(id);
match &fk {
FnKind::Method(ident, ..) => match method_context(cx, id) {
FnKind::Method(ident, sig, ..) => match method_context(cx, id) {
MethodLateContext::PlainImpl => {
if sig.header.abi != Abi::Rust && cx.sess().contains_name(attrs, sym::no_mangle)
{
return;
}
self.check_snake_case(cx, "method", ident);
}
MethodLateContext::TraitAutoImpl => {
Expand All @@ -402,7 +407,6 @@ impl<'tcx> LateLintPass<'tcx> for NonSnakeCase {
_ => (),
},
FnKind::ItemFn(ident, _, header, _) => {
let attrs = cx.tcx.hir().attrs(id);
// Skip foreign-ABI #[no_mangle] functions (Issue #31924)
if header.abi != Abi::Rust && cx.sess().contains_name(attrs, sym::no_mangle) {
return;
Expand Down
32 changes: 20 additions & 12 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,10 @@ impl CheckAttrVisitor<'tcx> {
}
}

fn is_impl_item(&self, hir_id: HirId) -> bool {
matches!(self.tcx.hir().get(hir_id), hir::Node::ImplItem(..))
}

/// Checks if `#[export_name]` is applied to a function or static. Returns `true` if valid.
fn check_export_name(
&self,
Expand All @@ -971,7 +975,8 @@ impl CheckAttrVisitor<'tcx> {
target: Target,
) -> bool {
match target {
Target::Static | Target::Fn | Target::Method(..) => true,
Target::Static | Target::Fn => true,
Target::Method(..) if self.is_impl_item(hir_id) => true,
// FIXME(#80564): We permit struct fields, match arms and macro defs to have an
// `#[export_name]` attribute with just a lint, because we previously
// erroneously allowed it and some crates used it accidentally, to to be compatible
Expand All @@ -985,9 +990,9 @@ impl CheckAttrVisitor<'tcx> {
.sess
.struct_span_err(
attr.span,
"attribute should be applied to a function or static",
"attribute should be applied to a free function, impl method or static",
)
.span_label(*span, "not a function or static")
.span_label(*span, "not a free function, impl method or static")
.emit();
false
}
Expand Down Expand Up @@ -1169,7 +1174,8 @@ impl CheckAttrVisitor<'tcx> {
/// Checks if `#[no_mangle]` is applied to a function or static.
fn check_no_mangle(&self, hir_id: HirId, attr: &Attribute, span: &Span, target: Target) {
match target {
Target::Static | Target::Fn | Target::Method(..) => {}
Target::Static | Target::Fn => {}
Target::Method(..) if self.is_impl_item(hir_id) => {}
// FIXME(#80564): We permit struct fields, match arms and macro defs to have an
// `#[no_mangle]` attribute with just a lint, because we previously
// erroneously allowed it and some crates used it accidentally, to to be compatible
Expand All @@ -1181,14 +1187,16 @@ impl CheckAttrVisitor<'tcx> {
// FIXME: #[no_mangle] was previously allowed on non-functions/statics and some
// crates used this, so only emit a warning.
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
lint.build("attribute should be applied to a function or static")
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
)
.span_label(*span, "not a function or static")
.emit();
lint.build(
"attribute should be applied to a free function, impl method or static",
)
.warn(
"this was previously accepted by the compiler but is \
being phased out; it will become a hard error in \
a future release!",
)
.span_label(*span, "not a free function, impl method or static")
.emit();
});
}
}
Expand Down
34 changes: 21 additions & 13 deletions compiler/rustc_passes/src/reachable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,13 +211,15 @@ impl<'tcx> ReachableContext<'tcx> {
if !self.any_library {
// If we are building an executable, only explicitly extern
// types need to be exported.
if let Node::Item(item) = *node {
let reachable = if let hir::ItemKind::Fn(ref sig, ..) = item.kind {
sig.header.abi != Abi::Rust
} else {
false
};
let codegen_attrs = self.tcx.codegen_fn_attrs(item.def_id);
if let Node::Item(hir::Item { kind: hir::ItemKind::Fn(sig, ..), def_id, .. })
| Node::ImplItem(hir::ImplItem {
kind: hir::ImplItemKind::Fn(sig, ..),
def_id,
..
}) = *node
{
let reachable = sig.header.abi != Abi::Rust;
let codegen_attrs = self.tcx.codegen_fn_attrs(*def_id);
Comment on lines -214 to +222
Copy link
Author

Choose a reason for hiding this comment

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

I think this change (unintentionally) made reachable_set no longer include any #[no_mangle] static item. Opened #88032 to fix this.

let is_extern = codegen_attrs.contains_extern_indicator();
let std_internal =
codegen_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL);
Expand Down Expand Up @@ -335,17 +337,23 @@ struct CollectPrivateImplItemsVisitor<'a, 'tcx> {
worklist: &'a mut Vec<LocalDefId>,
}

impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx> {
fn visit_item(&mut self, item: &hir::Item<'_>) {
impl CollectPrivateImplItemsVisitor<'_, '_> {
fn push_to_worklist_if_has_custom_linkage(&mut self, def_id: LocalDefId) {
// Anything which has custom linkage gets thrown on the worklist no
// matter where it is in the crate, along with "special std symbols"
// which are currently akin to allocator symbols.
let codegen_attrs = self.tcx.codegen_fn_attrs(item.def_id);
let codegen_attrs = self.tcx.codegen_fn_attrs(def_id);
if codegen_attrs.contains_extern_indicator()
|| codegen_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL)
{
self.worklist.push(item.def_id);
self.worklist.push(def_id);
}
}
}

impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx> {
fn visit_item(&mut self, item: &hir::Item<'_>) {
self.push_to_worklist_if_has_custom_linkage(item.def_id);

// We need only trait impls here, not inherent impls, and only non-exported ones
if let hir::ItemKind::Impl(hir::Impl { of_trait: Some(ref trait_ref), ref items, .. }) =
Expand Down Expand Up @@ -375,8 +383,8 @@ impl<'a, 'tcx> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, 'tcx

fn visit_trait_item(&mut self, _trait_item: &hir::TraitItem<'_>) {}

fn visit_impl_item(&mut self, _impl_item: &hir::ImplItem<'_>) {
// processed in visit_item above
fn visit_impl_item(&mut self, impl_item: &hir::ImplItem<'_>) {
self.push_to_worklist_if_has_custom_linkage(impl_item.def_id);
}

fn visit_foreign_item(&mut self, _foreign_item: &hir::ForeignItem<'_>) {
Expand Down
21 changes: 21 additions & 0 deletions src/test/ui/auxiliary/no-mangle-associated-fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#![crate_type = "lib"]

struct Bar;

impl Bar {
#[no_mangle]
fn bar() -> u8 {
2
}
}

trait Foo {
fn baz() -> u8;
}

impl Foo for Bar {
#[no_mangle]
fn baz() -> u8 {
3
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//~ NOTE: not an `extern crate` item
//~^ NOTE: not a function or static
//~^ NOTE: not a free function, impl method or static
//~^^ NOTE: not a function or closure
// This is testing whether various builtin attributes signals an
// error or warning when put in "weird" places.
Expand All @@ -25,7 +25,7 @@
#![no_link]
//~^ ERROR: attribute should be applied to an `extern crate` item
#![export_name = "2200"]
//~^ ERROR: attribute should be applied to a function or static
//~^ ERROR: attribute should be applied to a free function, impl method or static
#![inline]
//~^ ERROR: attribute should be applied to function or closure
#[inline]
Expand Down Expand Up @@ -83,27 +83,37 @@ mod no_link {
}

#[export_name = "2200"]
//~^ ERROR attribute should be applied to a function or static
//~^ ERROR attribute should be applied to a free function, impl method or static
mod export_name {
//~^ NOTE not a function or static
//~^ NOTE not a free function, impl method or static

mod inner { #![export_name="2200"] }
//~^ ERROR attribute should be applied to a function or static
//~| NOTE not a function or static
//~^ ERROR attribute should be applied to a free function, impl method or static
//~| NOTE not a free function, impl method or static

#[export_name = "2200"] fn f() { }

#[export_name = "2200"] struct S;
//~^ ERROR attribute should be applied to a function or static
//~| NOTE not a function or static
//~^ ERROR attribute should be applied to a free function, impl method or static
//~| NOTE not a free function, impl method or static

#[export_name = "2200"] type T = S;
//~^ ERROR attribute should be applied to a function or static
//~| NOTE not a function or static
//~^ ERROR attribute should be applied to a free function, impl method or static
//~| NOTE not a free function, impl method or static

#[export_name = "2200"] impl S { }
//~^ ERROR attribute should be applied to a function or static
//~| NOTE not a function or static
//~^ ERROR attribute should be applied to a free function, impl method or static
//~| NOTE not a free function, impl method or static

trait Tr {
#[export_name = "2200"] fn foo();
//~^ ERROR attribute should be applied to a free function, impl method or static
//~| NOTE not a free function, impl method or static

#[export_name = "2200"] fn bar() {}
//~^ ERROR attribute should be applied to a free function, impl method or static
//~| NOTE not a free function, impl method or static
}
}

#[start]
Expand Down
Loading