From a0efd3a3d99a98e3399a4f07abe6a67cf0660335 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 21 Jul 2015 17:31:35 -0700 Subject: [PATCH] trans: Be a little more picky about dllimport Currently you can hit a link error on MSVC by only referencing static items from a crate (no functions for example) and then link to the crate statically (as all Rust crates do 99% of the time). A detailed investigation can be found [on github][details], but the tl;dr is that we need to stop applying dllimport so aggressively. This commit alters the application of dllimport on constants to only cases where the crate the constant originated from will be linked as a dylib in some output crate type. That way if we're just linking rlibs (like the motivation for this issue) we won't use dllimport. For the compiler, however, (which has lots of dylibs) we'll use dllimport. [details]: https://github.com/rust-lang/rust/issues/26591#issuecomment-123513631 cc #26591 --- src/librustc_trans/trans/base.rs | 38 ++++++++++++++++++++++++++++- src/librustc_trans/trans/context.rs | 24 ++++++++++++++++++ src/test/auxiliary/xcrate-static.rs | 15 ++++++++++++ src/test/run-pass/xcrate-static.rs | 17 +++++++++++++ 4 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 src/test/auxiliary/xcrate-static.rs create mode 100644 src/test/run-pass/xcrate-static.rs diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 4aeba2fe06287..258051357b105 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -228,6 +228,7 @@ pub fn get_extern_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, did: ast::DefId, // FIXME(nagisa): perhaps the map of externs could be offloaded to llvm somehow? // FIXME(nagisa): investigate whether it can be changed into define_global let c = declare::declare_global(ccx, &name[..], ty); + // Thread-local statics in some other crate need to *always* be linked // against in a thread-local fashion, so we need to be sure to apply the // thread-local attribute locally if it was present remotely. If we @@ -239,7 +240,42 @@ pub fn get_extern_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, did: ast::DefId, llvm::set_thread_local(c, true); } } - if ccx.use_dll_storage_attrs() { + + // MSVC is a little ornery about how items are imported across dlls, and for + // lots more info on dllimport/dllexport see the large comment in + // SharedCrateContext::new. Unfortunately, unlike functions, statics + // imported from dlls *must* be tagged with dllimport (if you forget + // dllimport on a function then the linker fixes it up with an injected + // shim). This means that to link correctly to an upstream Rust dynamic + // library we need to make sure its statics are tagged with dllimport. + // + // Hence, if this translation is using dll storage attributes and the crate + // that this const originated from is being imported as a dylib at some + // point we tag this with dllimport. + // + // Note that this is not 100% correct for a variety of reasons: + // + // 1. If we are producing an rlib and linking to an upstream rlib, we'll + // omit the dllimport. It's a possibility, though, that some later + // downstream compilation will link the same upstream dependency as a + // dylib and use our rlib, causing linker errors because we didn't use + // dllimport. + // 2. We may have multiple crate output types. For example if we are + // emitting a statically linked binary as well as a dynamic library we'll + // want to omit dllimport for the binary but we need to have it for the + // dylib. + // + // For most every day uses, however, this should suffice. During the + // bootstrap we're almost always linking upstream to a dylib for some crate + // type output, so most imports will be tagged with dllimport (somewhat + // appropriately). Otherwise rust dylibs linking against rust dylibs is + // pretty rare in Rust so this will likely always be `false` and we'll never + // tag with dllimport. + // + // Note that we can't just blindly tag all constants with dllimport as can + // cause linkage errors when we're not actually linking against a dll. For + // more info on this see rust-lang/rust#26591. + if ccx.use_dll_storage_attrs() && ccx.upstream_dylib_used(did.krate) { llvm::SetDLLStorageClass(c, llvm::DLLImportStorageClass); } ccx.externs().borrow_mut().insert(name.to_string(), c); diff --git a/src/librustc_trans/trans/context.rs b/src/librustc_trans/trans/context.rs index 5a4bd7ff3a184..82428eebcce97 100644 --- a/src/librustc_trans/trans/context.rs +++ b/src/librustc_trans/trans/context.rs @@ -11,6 +11,7 @@ use llvm; use llvm::{ContextRef, ModuleRef, ValueRef, BuilderRef}; use metadata::common::LinkMeta; +use metadata::cstore; use middle::def::ExportMap; use middle::traits; use trans::adt; @@ -778,6 +779,29 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> { pub fn use_dll_storage_attrs(&self) -> bool { self.shared.use_dll_storage_attrs() } + + /// Tests whether the given `krate` (an upstream crate) is ever used as a + /// dynamic library for the final linkage of this crate. + pub fn upstream_dylib_used(&self, krate: ast::CrateNum) -> bool { + let tcx = self.tcx(); + let formats = tcx.dependency_formats.borrow(); + tcx.sess.crate_types.borrow().iter().any(|ct| { + match formats[ct].get(krate as usize - 1) { + // If a crate is explicitly linked dynamically then we're + // definitely using it dynamically. If it's not being linked + // then currently that means it's being included through another + // dynamic library, so we're including it dynamically. + Some(&Some(cstore::RequireDynamic)) | + Some(&None) => true, + + // Static linkage isn't included dynamically and if there's not + // an entry in the array then this crate type isn't actually + // doing much linkage so there's nothing dynamic going on. + Some(&Some(cstore::RequireStatic)) | + None => false, + } + }) + } } /// Declare any llvm intrinsics that you might need diff --git a/src/test/auxiliary/xcrate-static.rs b/src/test/auxiliary/xcrate-static.rs new file mode 100644 index 0000000000000..85093869ba21a --- /dev/null +++ b/src/test/auxiliary/xcrate-static.rs @@ -0,0 +1,15 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// no-prefer-dynamic + +#![crate_type = "rlib"] + +pub static FOO: u8 = 8; diff --git a/src/test/run-pass/xcrate-static.rs b/src/test/run-pass/xcrate-static.rs new file mode 100644 index 0000000000000..d1f08e726bc1f --- /dev/null +++ b/src/test/run-pass/xcrate-static.rs @@ -0,0 +1,17 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// aux-build:xcrate-static.rs + +extern crate xcrate_static; + +fn main() { + println!("{}", xcrate_static::FOO); +}