Skip to content

Deduplicate libraries on hash instead of filename. #32317

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Deduplicate libraries on hash instead of filename. #32317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/librustc/hir/svh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@

use std::fmt;

#[derive(Clone, PartialEq, Debug)]
#[derive(Clone, Eq, Hash, PartialEq, Debug)]
pub struct Svh {
hash: String,
}
Expand Down
98 changes: 43 additions & 55 deletions src/librustc_metadata/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,20 +470,17 @@ impl<'a> Context<'a> {
// A Library candidate is created if the metadata for the set of
// libraries corresponds to the crate id and hash criteria that this
// search is being performed for.
let mut libraries = Vec::new();
let mut libraries = HashMap::new();
for (_hash, (rlibs, dylibs)) in candidates {
let mut metadata = None;
let rlib = self.extract_one(rlibs, CrateFlavor::Rlib, &mut metadata);
let dylib = self.extract_one(dylibs, CrateFlavor::Dylib, &mut metadata);
match metadata {
Some(metadata) => {
libraries.push(Library {
dylib: dylib,
rlib: rlib,
metadata: metadata,
})
}
None => {}
let mut slot = None;
let rlib = self.extract_one(rlibs, CrateFlavor::Rlib, &mut slot);
let dylib = self.extract_one(dylibs, CrateFlavor::Dylib, &mut slot);
if let Some((h, m)) = slot {
libraries.insert(h, Library {
dylib: dylib,
rlib: rlib,
metadata: m,
});
}
}

Expand All @@ -492,13 +489,13 @@ impl<'a> Context<'a> {
// libraries or not.
match libraries.len() {
0 => None,
1 => Some(libraries.into_iter().next().unwrap()),
1 => Some(libraries.into_iter().next().unwrap().1),
_ => {
let mut err = struct_span_err!(self.sess, self.span, E0464,
"multiple matching crates for `{}`",
self.crate_name);
err.note("candidates:");
for lib in &libraries {
for (_, lib) in libraries {
match lib.dylib {
Some((ref p, _)) => {
err.note(&format!("path: {}",
Expand Down Expand Up @@ -532,13 +529,13 @@ impl<'a> Context<'a> {
// be read, it is assumed that the file isn't a valid rust library (no
// errors are emitted).
fn extract_one(&mut self, m: HashMap<PathBuf, PathKind>, flavor: CrateFlavor,
slot: &mut Option<MetadataBlob>) -> Option<(PathBuf, PathKind)> {
let mut ret = None::<(PathBuf, PathKind)>;
slot: &mut Option<(Svh, MetadataBlob)>) -> Option<(PathBuf, PathKind)> {
let mut ret: Option<(PathBuf, PathKind)> = None;
let mut error = 0;

if slot.is_some() {
// FIXME(#10786): for an optimization, we only read one of the
// library's metadata sections. In theory we should
// libraries' metadata sections. In theory we should
// read both, but reading dylib metadata is quite
// slow.
if m.is_empty() {
Expand All @@ -551,10 +548,10 @@ impl<'a> Context<'a> {
let mut err: Option<DiagnosticBuilder> = None;
for (lib, kind) in m {
info!("{} reading metadata from: {}", flavor, lib.display());
let metadata = match get_metadata_section(self.target, flavor, &lib) {
let (hash, metadata) = match get_metadata_section(self.target, flavor, &lib) {
Ok(blob) => {
if self.crate_matches(blob.as_slice(), &lib) {
blob
if let Some(h) = self.crate_matches(blob.as_slice(), &lib) {
(h, blob)
} else {
info!("metadata mismatch");
continue
Expand All @@ -565,12 +562,8 @@ impl<'a> Context<'a> {
continue
}
};
// If we've already found a candidate and we're not matching hashes,
// emit an error about duplicate candidates found. If we're matching
// based on a hash, however, then if we've gotten this far both
// candidates have the same hash, so they're not actually
// duplicates that we should warn about.
if ret.is_some() && self.hash.is_none() {
// If we see multiple hashes, emit an error about duplicate candidates.
if slot.as_ref().map_or(false, |s| s.0 != hash) {
let mut e = struct_span_err!(self.sess, self.span, E0465,
"multiple {} candidates for `{}` found",
flavor, self.crate_name);
Expand All @@ -583,7 +576,7 @@ impl<'a> Context<'a> {
}
err = Some(e);
error = 1;
ret = None;
*slot = None;
}
if error > 0 {
error += 1;
Expand All @@ -592,7 +585,7 @@ impl<'a> Context<'a> {
lib.display()));
continue
}
*slot = Some(metadata);
*slot = Some((hash, metadata));
ret = Some((lib, kind));
}

Expand All @@ -604,22 +597,20 @@ impl<'a> Context<'a> {
}
}

fn crate_matches(&mut self, crate_data: &[u8], libpath: &Path) -> bool {
fn crate_matches(&mut self, crate_data: &[u8], libpath: &Path) -> Option<Svh> {
if self.should_match_name {
match decoder::maybe_get_crate_name(crate_data) {
Some(ref name) if self.crate_name == *name => {}
_ => { info!("Rejecting via crate name"); return false }
_ => { info!("Rejecting via crate name"); return None }
}
}
let hash = match decoder::maybe_get_crate_hash(crate_data) {
Some(hash) => hash, None => {
info!("Rejecting via lack of crate hash");
return false;
}
None => { info!("Rejecting via lack of crate hash"); return None; }
Some(h) => h,
};

let triple = match decoder::get_crate_triple(crate_data) {
None => { debug!("triple not present"); return false }
None => { debug!("triple not present"); return None }
Some(t) => t,
};
if triple != self.triple {
Expand All @@ -628,24 +619,21 @@ impl<'a> Context<'a> {
path: libpath.to_path_buf(),
got: triple.to_string()
});
return false;
return None;
}

match self.hash {
None => true,
Some(myhash) => {
if *myhash != hash {
info!("Rejecting via hash: expected {} got {}", *myhash, hash);
self.rejected_via_hash.push(CrateMismatch {
path: libpath.to_path_buf(),
got: myhash.as_str().to_string()
});
false
} else {
true
}
if let Some(myhash) = self.hash {
if *myhash != hash {
info!("Rejecting via hash: expected {} got {}", *myhash, hash);
self.rejected_via_hash.push(CrateMismatch {
path: libpath.to_path_buf(),
got: myhash.as_str().to_string()
});
return None;
}
}

Some(hash)
}


Expand Down Expand Up @@ -717,13 +705,13 @@ impl<'a> Context<'a> {
};

// Extract the rlib/dylib pair.
let mut metadata = None;
let rlib = self.extract_one(rlibs, CrateFlavor::Rlib, &mut metadata);
let dylib = self.extract_one(dylibs, CrateFlavor::Dylib, &mut metadata);
let mut slot = None;
let rlib = self.extract_one(rlibs, CrateFlavor::Rlib, &mut slot);
let dylib = self.extract_one(dylibs, CrateFlavor::Dylib, &mut slot);

if rlib.is_none() && dylib.is_none() { return None }
match metadata {
Some(metadata) => Some(Library {
match slot {
Some((_, metadata)) => Some(Library {
dylib: dylib,
rlib: rlib,
metadata: metadata,
Expand Down
8 changes: 8 additions & 0 deletions src/test/run-make/compiler-lookup-paths/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,21 @@ all: $(TMPDIR)/libnative.a
$(RUSTC) d.rs -L crate=$(TMPDIR)/native && exit 1 || exit 0
$(RUSTC) d.rs -L native=$(TMPDIR)/native
$(RUSTC) d.rs -L all=$(TMPDIR)/native
# Deduplication tests:
# Same hash, no errors.
mkdir -p $(TMPDIR)/e1
mkdir -p $(TMPDIR)/e2
$(RUSTC) e.rs -o $(TMPDIR)/e1/libe.rlib
$(RUSTC) e.rs -o $(TMPDIR)/e2/libe.rlib
$(RUSTC) f.rs -L $(TMPDIR)/e1 -L $(TMPDIR)/e2
$(RUSTC) f.rs -L crate=$(TMPDIR)/e1 -L $(TMPDIR)/e2
$(RUSTC) f.rs -L crate=$(TMPDIR)/e1 -L crate=$(TMPDIR)/e2
# Different hash, errors.
$(RUSTC) e2.rs -o $(TMPDIR)/e2/libe.rlib
$(RUSTC) f.rs -L $(TMPDIR)/e1 -L $(TMPDIR)/e2 && exit 1 || exit 0
$(RUSTC) f.rs -L crate=$(TMPDIR)/e1 -L $(TMPDIR)/e2 && exit 1 || exit 0
$(RUSTC) f.rs -L crate=$(TMPDIR)/e1 -L crate=$(TMPDIR)/e2 && exit 1 || exit 0
# Native/dependency paths don't cause errors.
$(RUSTC) f.rs -L native=$(TMPDIR)/e1 -L $(TMPDIR)/e2
$(RUSTC) f.rs -L dependency=$(TMPDIR)/e1 -L $(TMPDIR)/e2
$(RUSTC) f.rs -L dependency=$(TMPDIR)/e1 -L crate=$(TMPDIR)/e2
14 changes: 14 additions & 0 deletions src/test/run-make/compiler-lookup-paths/e2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// 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.

#![crate_name = "e"]
#![crate_type = "rlib"]

pub fn f() {}
5 changes: 3 additions & 2 deletions src/test/run-make/extern-flag-fun/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@
all:
$(RUSTC) bar.rs --crate-type=rlib
$(RUSTC) bar.rs --crate-type=rlib -C extra-filename=-a
$(RUSTC) bar-alt.rs --crate-type=rlib
$(RUSTC) foo.rs --extern hello && exit 1 || exit 0
$(RUSTC) foo.rs --extern bar=no-exist && exit 1 || exit 0
$(RUSTC) foo.rs --extern bar=foo.rs && exit 1 || exit 0
$(RUSTC) foo.rs \
--extern bar=$(TMPDIR)/libbar.rlib \
--extern bar=$(TMPDIR)/libbar-a.rlib \
--extern bar=$(TMPDIR)/libbar-alt.rlib \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed because two externs with the same hash don't conflict anymore.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should still be an error? Passing --extern bar=... to two distinct locations seems like it's surely an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Externs are processed as a single group, so we deduplicate on hash irrespective of the filename.

&& exit 1 || exit 0
$(RUSTC) foo.rs \
--extern bar=$(TMPDIR)/libbar.rlib \
--extern bar=$(TMPDIR)/libbar.rlib
--extern bar=$(TMPDIR)/libbar-a.rlib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enhanced to test that deduplication is based on hash and not only name.

$(RUSTC) foo.rs --extern bar=$(TMPDIR)/libbar.rlib
11 changes: 11 additions & 0 deletions src/test/run-make/extern-flag-fun/bar-alt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2014 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.

pub fn f() {}
10 changes: 4 additions & 6 deletions src/test/run-make/issue-11908/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@

all:
mkdir $(TMPDIR)/other
$(RUSTC) foo.rs --crate-type=dylib
$(RUSTC) foo.rs --crate-type=dylib -C prefer-dynamic
mv $(call DYLIB,foo) $(TMPDIR)/other
$(RUSTC) foo.rs --crate-type=dylib
$(RUSTC) bar.rs -L $(TMPDIR)/other 2>&1 | \
grep "multiple dylib candidates"
$(RUSTC) foo.rs --crate-type=dylib -C prefer-dynamic
$(RUSTC) bar.rs -L $(TMPDIR)/other
rm -rf $(TMPDIR)
mkdir -p $(TMPDIR)/other
$(RUSTC) foo.rs --crate-type=rlib
mv $(TMPDIR)/libfoo.rlib $(TMPDIR)/other
$(RUSTC) foo.rs --crate-type=rlib
$(RUSTC) bar.rs -L $(TMPDIR)/other 2>&1 | \
grep "multiple rlib candidates"
$(RUSTC) bar.rs -L $(TMPDIR)/other
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the primary purpose of this change -- this should not fail as the two crates are basically the same.

Copy link
Member

Choose a reason for hiding this comment

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

Can you change this test so it still exhibits the same error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's a good idea. Issue 11908 is about getting assertion failures, not a missing check for "multiple rlib candidates". I will change it to explicitly test for not "assertion failed", which is what the issue was about.