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

Implement RFC 1717 #37973

Merged
merged 9 commits into from
Dec 6, 2016
72 changes: 50 additions & 22 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
@@ -262,7 +262,7 @@ top_level_options!(
// much sense: The search path can stay the same while the
// things discovered there might have changed on disk.
search_paths: SearchPaths [TRACKED],
libs: Vec<(String, cstore::NativeLibraryKind)> [TRACKED],
libs: Vec<(String, Option<String>, cstore::NativeLibraryKind)> [TRACKED],
maybe_sysroot: Option<PathBuf> [TRACKED],

target_triple: String [TRACKED],
@@ -1439,6 +1439,8 @@ pub fn build_session_options_and_crate_config(matches: &getopts::Matches)
}

let libs = matches.opt_strs("l").into_iter().map(|s| {
// Parse string of the form "[KIND=]lib[:new_name]",
// where KIND is one of "dylib", "framework", "static".
let mut parts = s.splitn(2, '=');
let kind = parts.next().unwrap();
let (name, kind) = match (parts.next(), kind) {
@@ -1452,7 +1454,10 @@ pub fn build_session_options_and_crate_config(matches: &getopts::Matches)
s));
}
};
(name.to_string(), kind)
let mut name_parts = name.splitn(2, ':');
let name = name_parts.next().unwrap();
let new_name = name_parts.next();
(name.to_string(), new_name.map(|n| n.to_string()), kind)
}).collect();

let cfg = parse_cfgspecs(matches.opt_strs("cfg"));
@@ -1716,8 +1721,8 @@ mod dep_tracking {
impl_dep_tracking_hash_for_sortable_vec_of!(String);
impl_dep_tracking_hash_for_sortable_vec_of!(CrateType);
impl_dep_tracking_hash_for_sortable_vec_of!((String, lint::Level));
impl_dep_tracking_hash_for_sortable_vec_of!((String, cstore::NativeLibraryKind));

impl_dep_tracking_hash_for_sortable_vec_of!((String, Option<String>,
cstore::NativeLibraryKind));
impl DepTrackingHash for SearchPaths {
fn hash(&self, hasher: &mut DefaultHasher, _: ErrorOutputType) {
let mut elems: Vec<_> = self
@@ -1740,6 +1745,21 @@ mod dep_tracking {
}
}

impl<T1, T2, T3> DepTrackingHash for (T1, T2, T3)
where T1: DepTrackingHash,
T2: DepTrackingHash,
T3: DepTrackingHash
{
fn hash(&self, hasher: &mut DefaultHasher, error_format: ErrorOutputType) {
Hash::hash(&0, hasher);
DepTrackingHash::hash(&self.0, hasher, error_format);
Hash::hash(&1, hasher);
DepTrackingHash::hash(&self.1, hasher, error_format);
Hash::hash(&2, hasher);
DepTrackingHash::hash(&self.2, hasher, error_format);
}
}

// This is a stable hash because BTreeMap is a sorted container
pub fn stable_hash(sub_hashes: BTreeMap<&'static str, &DepTrackingHash>,
hasher: &mut DefaultHasher,
@@ -2143,29 +2163,37 @@ mod tests {
let mut v1 = super::basic_options();
let mut v2 = super::basic_options();
let mut v3 = super::basic_options();
let mut v4 = super::basic_options();

// Reference
v1.libs = vec![(String::from("a"), cstore::NativeStatic),
(String::from("b"), cstore::NativeFramework),
(String::from("c"), cstore::NativeUnknown)];
v1.libs = vec![(String::from("a"), None, cstore::NativeStatic),
(String::from("b"), None, cstore::NativeFramework),
(String::from("c"), None, cstore::NativeUnknown)];

// Change label
v2.libs = vec![(String::from("a"), cstore::NativeStatic),
(String::from("X"), cstore::NativeFramework),
(String::from("c"), cstore::NativeUnknown)];
v2.libs = vec![(String::from("a"), None, cstore::NativeStatic),
(String::from("X"), None, cstore::NativeFramework),
(String::from("c"), None, cstore::NativeUnknown)];

// Change kind
v3.libs = vec![(String::from("a"), cstore::NativeStatic),
(String::from("b"), cstore::NativeStatic),
(String::from("c"), cstore::NativeUnknown)];
v3.libs = vec![(String::from("a"), None, cstore::NativeStatic),
(String::from("b"), None, cstore::NativeStatic),
(String::from("c"), None, cstore::NativeUnknown)];

// Change new-name
v4.libs = vec![(String::from("a"), None, cstore::NativeStatic),
(String::from("b"), Some(String::from("X")), cstore::NativeFramework),
(String::from("c"), None, cstore::NativeUnknown)];

assert!(v1.dep_tracking_hash() != v2.dep_tracking_hash());
assert!(v1.dep_tracking_hash() != v3.dep_tracking_hash());
assert!(v1.dep_tracking_hash() != v4.dep_tracking_hash());

// Check clone
assert_eq!(v1.dep_tracking_hash(), v1.clone().dep_tracking_hash());
assert_eq!(v2.dep_tracking_hash(), v2.clone().dep_tracking_hash());
assert_eq!(v3.dep_tracking_hash(), v3.clone().dep_tracking_hash());
assert_eq!(v4.dep_tracking_hash(), v4.clone().dep_tracking_hash());
}

#[test]
@@ -2175,17 +2203,17 @@ mod tests {
let mut v3 = super::basic_options();

// Reference
v1.libs = vec![(String::from("a"), cstore::NativeStatic),
(String::from("b"), cstore::NativeFramework),
(String::from("c"), cstore::NativeUnknown)];
v1.libs = vec![(String::from("a"), None, cstore::NativeStatic),
(String::from("b"), None, cstore::NativeFramework),
(String::from("c"), None, cstore::NativeUnknown)];

v2.libs = vec![(String::from("b"), cstore::NativeFramework),
(String::from("a"), cstore::NativeStatic),
(String::from("c"), cstore::NativeUnknown)];
v2.libs = vec![(String::from("b"), None, cstore::NativeFramework),
(String::from("a"), None, cstore::NativeStatic),
(String::from("c"), None, cstore::NativeUnknown)];

v3.libs = vec![(String::from("c"), cstore::NativeUnknown),
(String::from("a"), cstore::NativeStatic),
(String::from("b"), cstore::NativeFramework)];
v3.libs = vec![(String::from("c"), None, cstore::NativeUnknown),
(String::from("a"), None, cstore::NativeStatic),
(String::from("b"), None, cstore::NativeFramework)];

assert!(v1.dep_tracking_hash() == v2.dep_tracking_hash());
assert!(v1.dep_tracking_hash() == v3.dep_tracking_hash());
22 changes: 14 additions & 8 deletions src/librustc_metadata/creader.rs
Original file line number Diff line number Diff line change
@@ -966,14 +966,20 @@ impl<'a> middle::cstore::CrateLoader for CrateLoader<'a> {
dump_crates(&self.cstore);
}

for &(ref name, kind) in &self.sess.opts.libs {
let lib = NativeLibrary {
name: Symbol::intern(name),
kind: kind,
cfg: None,
foreign_items: Vec::new(),
};
register_native_lib(self.sess, self.cstore, None, lib);
// Process libs passed on the command line
for &(ref name, ref new_name, kind) in &self.sess.opts.libs {
// First, try to update existing lib(s) added via #[link(...)]
let new_name = new_name.as_ref().map(|s| &**s); // &Option<String> -> Option<&str>
if !self.cstore.update_used_library(name, new_name, kind) {
// Add if not found
let lib = NativeLibrary {
name: Symbol::intern(name),
kind: kind,
cfg: None,
foreign_items: Vec::new(),
};
register_native_lib(self.sess, self.cstore, None, lib);
Copy link
Member

Choose a reason for hiding this comment

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

In this case the new_name is thrown away, but isn't that the name the library should be linked under?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Yes, I should use new_name here, if available.
Probably also emit a warning?

Copy link
Member

Choose a reason for hiding this comment

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

Hm actually given the option to do so let's make this a hard error to be conservative.

}
}
self.register_statically_included_foreign_items();
self.register_dllimport_foreign_items();
17 changes: 17 additions & 0 deletions src/librustc_metadata/cstore.rs
Original file line number Diff line number Diff line change
@@ -232,6 +232,23 @@ impl CStore {
self.used_libraries.borrow_mut().push(lib);
}

// Update kind and, optionally, the name of all native libaries (there may be more than one)
// with the specified name.
pub fn update_used_library(&self, name: &str, new_name: Option<&str>,
new_kind: NativeLibraryKind) -> bool {
let mut found = false;
for item in self.used_libraries.borrow_mut().iter_mut() {
if item.name == name {
item.kind = new_kind;
if let Some(new_name) = new_name {
item.name = Symbol::intern(new_name);
Copy link
Member

Choose a reason for hiding this comment

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

Could we take an extra conservative route here and avoid multiple indirections of libraries? Ideally each library would be renamed at most once and wouldn't have to worry about what order we process options in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by "multiple indirections"... Is this about the fact that there may be several NativeLibrary entries for the same library? I don't think we can just merge them,- because library ordering on linker's command line is important.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I mean something like:

rustc -l foo:bar -l bar:baz -l baz:real-name

We shouldn't allow something like that and a lib should be "renamed" at most once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Just want to note that this behavior is occasionally useful when you want to override something by appending to a pre-composed command line (usually comes up in makefiles and such).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton: How far did you want to take this checking?

Currently the following are allowed:
-l foo -l foo
-l static=foo -l dylib=foo
which would result in adding foo to the linker command line twice.

Under the new rules, this will merely change the kind of foo two times (assuming it's been defined in crate source, and if not... I am not sure).

It feels a bit weird to allow chaining for kind alterations, but not for names.

Perhaps this part of the RFC merits additional discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Hm that does sound like an unfortunate "regression", but I'd imagine that in practice that was doomed to never link correctly anyway?

It does seem like we can't strictly require that -lfoo isn't specified more than once, but perhaps we can still be strict about renamings where a lib is only renamed once?

}
found = true;
}
}
found
}

pub fn get_used_libraries(&self) -> &RefCell<Vec<NativeLibrary>> {
&self.used_libraries
}
15 changes: 15 additions & 0 deletions src/test/run-pass/rfc1717/auxiliary/clibrary.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2016 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// no-prefer-dynamic
#![crate_type = "staticlib"]

#[no_mangle]
pub extern "C" fn foo(x:i32) -> i32 { x }
23 changes: 23 additions & 0 deletions src/test/run-pass/rfc1717/library-override.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Copyright 2016 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// aux-build:clibrary.rs
// compile-flags: -lstatic=wronglibrary:clibrary

#[link(name = "wronglibrary", kind = "dylib")]
extern "C" {
pub fn foo(x:i32) -> i32;
}

fn main() {
unsafe {
foo(42);
}
}