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

Take the original extra-filename passed to a crate into account when resolving it as a dependency #49253

Merged
merged 2 commits into from
Apr 5, 2018
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
1 change: 1 addition & 0 deletions src/librustc/dep_graph/dep_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,7 @@ define_dep_nodes!( <'tcx>
[input] CrateDisambiguator(CrateNum),
[input] CrateHash(CrateNum),
[input] OriginalCrateName(CrateNum),
[input] ExtraFileName(CrateNum),

[] ImplementationsOfTrait { krate: CrateNum, trait_id: DefId },
[] AllTraitImplementations(CrateNum),
Expand Down
6 changes: 6 additions & 0 deletions src/librustc/ty/maps/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,12 @@ impl<'tcx> QueryDescription<'tcx> for queries::original_crate_name<'tcx> {
}
}

impl<'tcx> QueryDescription<'tcx> for queries::extra_filename<'tcx> {
fn describe(_tcx: TyCtxt, _: CrateNum) -> String {
format!("looking up the extra filename for a crate")
}
}

impl<'tcx> QueryDescription<'tcx> for queries::implementations_of_trait<'tcx> {
fn describe(_tcx: TyCtxt, _: (CrateNum, DefId)) -> String {
format!("looking up implementations of a trait in a crate")
Expand Down
1 change: 1 addition & 0 deletions src/librustc/ty/maps/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ define_maps! { <'tcx>
[] fn crate_disambiguator: CrateDisambiguator(CrateNum) -> CrateDisambiguator,
[] fn crate_hash: CrateHash(CrateNum) -> Svh,
[] fn original_crate_name: OriginalCrateName(CrateNum) -> Symbol,
[] fn extra_filename: ExtraFileName(CrateNum) -> String,

[] fn implementations_of_trait: implementations_of_trait_node((CrateNum, DefId))
-> Lrc<Vec<DefId>>,
Expand Down
1 change: 1 addition & 0 deletions src/librustc/ty/maps/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
DepKind::CrateDisambiguator => { force!(crate_disambiguator, krate!()); }
DepKind::CrateHash => { force!(crate_hash, krate!()); }
DepKind::OriginalCrateName => { force!(original_crate_name, krate!()); }
DepKind::ExtraFileName => { force!(extra_filename, krate!()); }

DepKind::AllTraitImplementations => {
force!(all_trait_implementations, krate!());
Expand Down
22 changes: 15 additions & 7 deletions src/librustc_metadata/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ impl<'a> CrateLoader<'a> {
ident: Symbol,
name: Symbol,
hash: Option<&Svh>,
extra_filename: Option<&str>,
span: Span,
path_kind: PathKind,
mut dep_kind: DepKind)
Expand All @@ -277,6 +278,7 @@ impl<'a> CrateLoader<'a> {
ident,
crate_name: name,
hash: hash.map(|a| &*a),
extra_filename: extra_filename,
filesearch: self.sess.target_filesearch(path_kind),
target: &self.sess.target.target,
triple: &self.sess.opts.target_triple,
Expand Down Expand Up @@ -409,7 +411,8 @@ impl<'a> CrateLoader<'a> {
::std::iter::once(krate).chain(crate_root.crate_deps
.decode(metadata)
.map(|dep| {
debug!("resolving dep crate {} hash: `{}`", dep.name, dep.hash);
info!("resolving dep crate {} hash: `{}` extra filename: `{}`", dep.name, dep.hash,
dep.extra_filename);
if dep.kind == DepKind::UnexportedMacrosOnly {
return krate;
}
Expand All @@ -418,7 +421,8 @@ impl<'a> CrateLoader<'a> {
_ => dep.kind,
};
let (local_cnum, ..) = self.resolve_crate(
root, dep.name, dep.name, Some(&dep.hash), span, PathKind::Dependency, dep_kind,
root, dep.name, dep.name, Some(&dep.hash), Some(&dep.extra_filename), span,
PathKind::Dependency, dep_kind,
);
local_cnum
})).collect()
Expand All @@ -437,6 +441,7 @@ impl<'a> CrateLoader<'a> {
ident: orig_name,
crate_name: rename,
hash: None,
extra_filename: None,
filesearch: self.sess.host_filesearch(PathKind::Crate),
target: &self.sess.host,
triple: &host_triple,
Expand Down Expand Up @@ -664,7 +669,7 @@ impl<'a> CrateLoader<'a> {

let dep_kind = DepKind::Implicit;
let (cnum, data) =
self.resolve_crate(&None, name, name, None, DUMMY_SP, PathKind::Crate, dep_kind);
self.resolve_crate(&None, name, name, None, None, DUMMY_SP, PathKind::Crate, dep_kind);

// Sanity check the loaded crate to ensure it is indeed a panic runtime
// and the panic strategy is indeed what we thought it was.
Expand Down Expand Up @@ -771,7 +776,7 @@ impl<'a> CrateLoader<'a> {
let symbol = Symbol::intern(name);
let dep_kind = DepKind::Explicit;
let (_, data) =
self.resolve_crate(&None, symbol, symbol, None, DUMMY_SP,
self.resolve_crate(&None, symbol, symbol, None, None, DUMMY_SP,
PathKind::Crate, dep_kind);

// Sanity check the loaded crate to ensure it is indeed a sanitizer runtime
Expand All @@ -794,7 +799,7 @@ impl<'a> CrateLoader<'a> {
let symbol = Symbol::intern("profiler_builtins");
let dep_kind = DepKind::Implicit;
let (_, data) =
self.resolve_crate(&None, symbol, symbol, None, DUMMY_SP,
self.resolve_crate(&None, symbol, symbol, None, None, DUMMY_SP,
PathKind::Crate, dep_kind);

// Sanity check the loaded crate to ensure it is indeed a profiler runtime
Expand Down Expand Up @@ -909,6 +914,7 @@ impl<'a> CrateLoader<'a> {
name,
name,
None,
None,
DUMMY_SP,
PathKind::Crate,
DepKind::Implicit);
Expand Down Expand Up @@ -1059,7 +1065,8 @@ impl<'a> middle::cstore::CrateLoader for CrateLoader<'a> {
};

let (cnum, ..) = self.resolve_crate(
&None, item.ident.name, orig_name, None, item.span, PathKind::Crate, dep_kind,
&None, item.ident.name, orig_name, None, None,
item.span, PathKind::Crate, dep_kind,
);

let def_id = definitions.opt_local_def_id(item.id).unwrap();
Expand All @@ -1074,6 +1081,7 @@ impl<'a> middle::cstore::CrateLoader for CrateLoader<'a> {
}

fn resolve_crate_from_path(&mut self, name: Symbol, span: Span) -> CrateNum {
self.resolve_crate(&None, name, name, None, span, PathKind::Crate, DepKind::Explicit).0
self.resolve_crate(&None, name, name, None, None, span, PathKind::Crate,
DepKind::Explicit).0
}
}
3 changes: 3 additions & 0 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,9 @@ provide! { <'tcx> tcx, def_id, other, cdata,
crate_hash => { cdata.hash() }
original_crate_name => { cdata.name() }

extra_filename => { cdata.root.extra_filename.clone() }


implementations_of_trait => {
let mut result = vec![];
let filter = Some(other);
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_metadata/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
let has_global_allocator = tcx.sess.has_global_allocator.get();
let root = self.lazy(&CrateRoot {
name: tcx.crate_name(LOCAL_CRATE),
extra_filename: tcx.sess.opts.cg.extra_filename.clone(),
triple: tcx.sess.opts.target_triple.clone(),
hash: link_meta.crate_hash,
disambiguator: tcx.sess.local_crate_disambiguator(),
Expand Down Expand Up @@ -1357,6 +1358,7 @@ impl<'a, 'b: 'a, 'tcx: 'b> IsolatedEncoder<'a, 'b, 'tcx> {
name: self.tcx.original_crate_name(cnum),
hash: self.tcx.crate_hash(cnum),
kind: self.tcx.dep_kind(cnum),
extra_filename: self.tcx.extra_filename(cnum),
};
(cnum, dep)
})
Expand Down
30 changes: 24 additions & 6 deletions src/librustc_metadata/locator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@
//! 1. Does the filename match an rlib/dylib pattern? That is to say, does the
//! filename have the right prefix/suffix?
//! 2. Does the filename have the right prefix for the crate name being queried?
//! This is filtering for files like `libfoo*.rlib` and such.
//! This is filtering for files like `libfoo*.rlib` and such. If the crate
//! we're looking for was originally compiled with -C extra-filename, the
//! extra filename will be included in this prefix to reduce reading
//! metadata from crates that would otherwise share our prefix.
//! 3. Is the file an actual rust library? This is done by loading the metadata
//! from the library and making sure it's actually there.
//! 4. Does the name in the metadata agree with the name of the library?
Expand Down Expand Up @@ -236,6 +239,7 @@ use syntax_pos::Span;
use rustc_back::target::{Target, TargetTriple};

use std::cmp;
use std::collections::HashSet;
use std::fmt;
use std::fs;
use std::io::{self, Read};
Expand All @@ -256,6 +260,7 @@ pub struct Context<'a> {
pub ident: Symbol,
pub crate_name: Symbol,
pub hash: Option<&'a Svh>,
pub extra_filename: Option<&'a str>,
// points to either self.sess.target.target or self.sess.host, must match triple
pub target: &'a Target,
pub triple: &'a TargetTriple,
Expand Down Expand Up @@ -303,7 +308,12 @@ impl CratePaths {

impl<'a> Context<'a> {
pub fn maybe_load_library_crate(&mut self) -> Option<Library> {
self.find_library_crate()
let mut seen_paths = HashSet::new();
match self.extra_filename {
Some(s) => self.find_library_crate(s, &mut seen_paths)
.or_else(|| self.find_library_crate("", &mut seen_paths)),
None => self.find_library_crate("", &mut seen_paths)
}
}

pub fn report_errs(&mut self) -> ! {
Expand Down Expand Up @@ -419,7 +429,10 @@ impl<'a> Context<'a> {
unreachable!();
}

fn find_library_crate(&mut self) -> Option<Library> {
fn find_library_crate(&mut self,
extra_prefix: &str,
seen_paths: &mut HashSet<PathBuf>)
-> Option<Library> {
// If an SVH is specified, then this is a transitive dependency that
// must be loaded via -L plus some filtering.
if self.hash.is_none() {
Expand All @@ -434,9 +447,9 @@ impl<'a> Context<'a> {
let staticpair = self.staticlibname();

// want: crate_name.dir_part() + prefix + crate_name.file_part + "-"
let dylib_prefix = format!("{}{}", dypair.0, self.crate_name);
let rlib_prefix = format!("lib{}", self.crate_name);
let staticlib_prefix = format!("{}{}", staticpair.0, self.crate_name);
let dylib_prefix = format!("{}{}{}", dypair.0, self.crate_name, extra_prefix);
let rlib_prefix = format!("lib{}{}", self.crate_name, extra_prefix);
let staticlib_prefix = format!("{}{}{}", staticpair.0, self.crate_name, extra_prefix);

let mut candidates = FxHashMap();
let mut staticlibs = vec![];
Expand Down Expand Up @@ -476,6 +489,7 @@ impl<'a> Context<'a> {
}
return FileDoesntMatch;
};

info!("lib candidate: {}", path.display());

let hash_str = hash.to_string();
Expand All @@ -484,6 +498,10 @@ impl<'a> Context<'a> {
let (ref mut rlibs, ref mut rmetas, ref mut dylibs) = *slot;
fs::canonicalize(path)
.map(|p| {
if seen_paths.contains(&p) {
return FileDoesntMatch
};
seen_paths.insert(p.clone());
match found_kind {
CrateFlavor::Rlib => { rlibs.insert(p, kind); }
CrateFlavor::Rmeta => { rmetas.insert(p, kind); }
Expand Down
5 changes: 4 additions & 1 deletion src/librustc_metadata/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ pub enum LazyState {
pub struct CrateRoot {
pub name: Symbol,
pub triple: TargetTriple,
pub extra_filename: String,
pub hash: hir::svh::Svh,
pub disambiguator: CrateDisambiguator,
pub panic_strategy: PanicStrategy,
Expand Down Expand Up @@ -216,12 +217,14 @@ pub struct CrateDep {
pub name: ast::Name,
pub hash: hir::svh::Svh,
pub kind: DepKind,
pub extra_filename: String,
}

impl_stable_hash_for!(struct CrateDep {
name,
hash,
kind
kind,
extra_filename
});

#[derive(RustcEncodable, RustcDecodable)]
Expand Down
7 changes: 7 additions & 0 deletions src/test/run-make-fulldeps/resolve-rename/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-include ../tools.mk

all:
$(RUSTC) -C extra-filename=-hash foo.rs
$(RUSTC) bar.rs
Copy link
Member

Choose a reason for hiding this comment

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

If you pass --crate-name foo here, does that make the test fail on today's compiler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, but because it's identical to one of its dependencies (same crate name and metadata), which is also a failure with this patch.

Copy link
Member

Choose a reason for hiding this comment

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

Could a different metadata be passed to bar.rs here so that error path wouldn't get hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but then we fail because baz.rs can't tell which version of foo to use, which is still an issue with this patch because we don't yet have an extra-filename for foo, because we're searching for it as a direct dependency of baz. This seemed plausible:

$(RUSTC) -C extra-filename=-hash foo.rs
$(RUSTC) --crate-name foo -C metadata=baz -C extra-filename=-another-hash bar.rs
$(RUSTC) --extern foo=$(TMPDIR)/libfoo-another-hash.rlib baz.rs
$(RUSTC) qux.rs

where qux depends on baz, but even then locator.rs doesn't produce an error because it can differentiate between foo crates by the SVH it finds in the metadata for deps of baz, and doesn't consider the foo found in foo.rs when resolving dependencies for baz.rs.

After trying this and a few more things I'm sort of convinced we're not going to get further in this direction, because whenever extra_filename is present in the locate context, hash will be too, and in this case locator.rs doesn't produce the kind of errors we're trying to induce.

Should we consider landing this without the test?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I think you're right, that makes sense to me and means that it may be quite hard to add a test. In that case I think it's fine, do you want to remove this one as it's not testing what we thought it was testing?

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 test may still be interesting, because it's a case that fails with an earlier version of this patch (which didn't fall back to an imprecise match if the match considering extra-filename failed), but I'd be ok with leaving it out for now as well. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok! If it catches a previous mistake sounds good to leave in

mv $(TMPDIR)/libfoo-hash.rlib $(TMPDIR)/libfoo-another-hash.rlib
$(RUSTC) baz.rs
15 changes: 15 additions & 0 deletions src/test/run-make-fulldeps/resolve-rename/bar.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// 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.

#![crate_type = "rlib"]

extern crate foo;

pub fn bar() { foo::foo() }
15 changes: 15 additions & 0 deletions src/test/run-make-fulldeps/resolve-rename/baz.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// 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.

#![crate_type = "rlib"]

extern crate bar;

pub fn baz() { bar::bar() }
13 changes: 13 additions & 0 deletions src/test/run-make-fulldeps/resolve-rename/foo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// 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.

#![crate_type = "rlib"]

pub fn foo() {}