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

Only compile #[used] as llvm.compiler.used for ELF targets #93718

Merged
merged 4 commits into from
Jul 21, 2022
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
14 changes: 12 additions & 2 deletions compiler/rustc_codegen_llvm/src/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -535,10 +535,20 @@ impl<'ll> StaticMethods for CodegenCx<'ll, '_> {

// The semantics of #[used] in Rust only require the symbol to make it into the
// object file. It is explicitly allowed for the linker to strip the symbol if it
// is dead. As such, use llvm.compiler.used instead of llvm.used.
// is dead, which means we are allowed use `llvm.compiler.used` instead of
// `llvm.used` here.
//
// Additionally, https://reviews.llvm.org/D97448 in LLVM 13 started emitting unique
// sections with SHF_GNU_RETAIN flag for llvm.used symbols, which may trigger bugs
// in some versions of the gold linker.
// in the handling of `.init_array` (the static constructor list) in versions of
// the gold linker (prior to the one released with binutils 2.36).
//
// That said, we only ever emit these when compiling for ELF targets, unless
// `#[used(compiler)]` is explicitly requested. This is to avoid similar breakage
// on other targets, in particular MachO targets have *their* static constructor
// lists broken if `llvm.compiler.used` is emitted rather than llvm.used. However,
// that check happens when assigning the `CodegenFnAttrFlags` in `rustc_typeck`,
// so we don't need to take care of it here.
self.add_compiler_used_global(g);
}
if attrs.flags.contains(CodegenFnAttrFlags::USED_LINKER) {
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_ssa/src/traits/statics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ pub trait StaticMethods: BackendTypes {
/// Same as add_used_global(), but only prevent the compiler from potentially removing an
/// otherwise unused symbol. The linker is still permitted to drop it.
///
/// This corresponds to the semantics of the `#[used]` attribute.
/// This corresponds to the documented semantics of the `#[used]` attribute, although
/// on some targets (non-ELF), we may use `add_used_global` for `#[used]` statics
/// instead.
fn add_compiler_used_global(&self, global: Self::Value);
}

Expand Down
32 changes: 31 additions & 1 deletion compiler/rustc_typeck/src/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2850,7 +2850,37 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: DefId) -> CodegenFnAttrs {
)
.emit();
}
None => codegen_fn_attrs.flags |= CodegenFnAttrFlags::USED,
None => {
// Unfortunately, unconditionally using `llvm.used` causes
// issues in handling `.init_array` with the gold linker,
// but using `llvm.compiler.used` caused a nontrival amount
// of unintentional ecosystem breakage -- particularly on
// Mach-O targets.
//
// As a result, we emit `llvm.compiler.used` only on ELF
// targets. This is somewhat ad-hoc, but actually follows
// our pre-LLVM 13 behavior (prior to the ecosystem
// breakage), and seems to match `clang`'s behavior as well
// (both before and after LLVM 13), possibly because they
// have similar compatibility concerns to us. See
// https://github.com/rust-lang/rust/issues/47384#issuecomment-1019080146
// and following comments for some discussion of this, as
// well as the comments in `rustc_codegen_llvm` where these
// flags are handled.
//
// Anyway, to be clear: this is still up in the air
// somewhat, and is subject to change in the future (which
// is a good thing, because this would ideally be a bit
// more firmed up).
let is_like_elf = !(tcx.sess.target.is_like_osx
|| tcx.sess.target.is_like_windows
|| tcx.sess.target.is_like_wasm);
codegen_fn_attrs.flags = if is_like_elf {
tmiasko marked this conversation as resolved.
Show resolved Hide resolved
CodegenFnAttrFlags::USED
} else {
CodegenFnAttrFlags::USED_LINKER
Comment on lines +2879 to +2881
Copy link
Member

Choose a reason for hiding this comment

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

If I'm understanding all this correctly, it sounds like CodegenFnAttrFlags::USED corresponds to llvm.compiler.used, while CodegenFnAttrFlags::USED_LINKER corresponds to llvm.used.

If the above is true, then I personally would prefer USED to have a more specific name (e.g. USED_COMPILER.

But having said that, I don't want to delay this PR even longer than it already has been delayed based on a cleanup task like that. Its just a passing comment on how I found this code a little confusing as written.

};
}
}
} else if attr.has_name(sym::cmse_nonsecure_entry) {
if !matches!(tcx.fn_sig(did).abi(), abi::Abi::C { .. }) {
Expand Down
11 changes: 11 additions & 0 deletions src/test/run-make-fulldeps/used-cdylib-macos/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
-include ../tools.mk

# only-macos
#
# This checks that `#[used]` passes through to the linker on
# darwin. This is subject to change in the future, see
# https://github.com/rust-lang/rust/pull/93718 for discussion

all:
$(RUSTC) -Copt-level=3 dylib_used.rs
nm $(TMPDIR)/libdylib_used.dylib | $(CGREP) VERY_IMPORTANT_SYMBOL
4 changes: 4 additions & 0 deletions src/test/run-make-fulldeps/used-cdylib-macos/dylib_used.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#![crate_type = "cdylib"]

#[used]
static VERY_IMPORTANT_SYMBOL: u32 = 12345;