From f916f3a36b21629d58e1b8ea4dbddc0db1316903 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 1 Feb 2022 16:18:39 +1100 Subject: [PATCH 1/7] Remove rlib special-casing in `FileSearch::search`. This code and comment appear to be out of date. `CrateLocator::find_library_crate` is the only caller of this function and it handles rlib vs dylib overlap itself (see `CrateLocator::extract_lib`) after inspecting all the files present, so it doesn't need to see them in any particular order. --- compiler/rustc_session/src/filesearch.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/compiler/rustc_session/src/filesearch.rs b/compiler/rustc_session/src/filesearch.rs index 357190178ce01..9687d0bd8a701 100644 --- a/compiler/rustc_session/src/filesearch.rs +++ b/compiler/rustc_session/src/filesearch.rs @@ -49,16 +49,7 @@ impl<'a> FileSearch<'a> { { for search_path in self.search_paths() { debug!("searching {}", search_path.dir.display()); - fn is_rlib(spf: &SearchPathFile) -> bool { - if let Some(f) = &spf.file_name_str { f.ends_with(".rlib") } else { false } - } - // Reading metadata out of rlibs is faster, and if we find both - // an rlib and a dylib we only read one of the files of - // metadata, so in the name of speed, bring all rlib files to - // the front of the search list. - let files1 = search_path.files.iter().filter(|spf| is_rlib(&spf)); - let files2 = search_path.files.iter().filter(|spf| !is_rlib(&spf)); - for spf in files1.chain(files2) { + for spf in search_path.files.iter() { debug!("testing {}", spf.path.display()); let maybe_picked = pick(spf, search_path.kind); match maybe_picked { From 47b5d95db8abaaf4fdad878ec3b06dfaa2a1d74f Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 1 Feb 2022 16:24:48 +1100 Subject: [PATCH 2/7] Remove `FileMatch`. It's returned from `FileSearch::search` but it's only used to print some debug info. --- compiler/rustc_metadata/src/locator.rs | 9 ++++----- compiler/rustc_session/src/filesearch.rs | 14 ++------------ 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index 13ea089e245a6..8db65a10c1386 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -223,7 +223,7 @@ use rustc_data_structures::sync::MetadataRef; use rustc_errors::{struct_span_err, FatalError}; use rustc_session::config::{self, CrateType}; use rustc_session::cstore::{CrateSource, MetadataLoader}; -use rustc_session::filesearch::{FileDoesntMatch, FileMatches, FileSearch}; +use rustc_session::filesearch::FileSearch; use rustc_session::search_paths::PathKind; use rustc_session::utils::CanonicalizedPath; use rustc_session::Session; @@ -396,7 +396,7 @@ impl<'a> CrateLocator<'a> { // The goal of this step is to look at as little metadata as possible. self.filesearch.search(|spf, kind| { let file = match &spf.file_name_str { - None => return FileDoesntMatch, + None => return, Some(file) => file, }; let (hash, found_kind) = if file.starts_with(&rlib_prefix) && file.ends_with(".rlib") { @@ -415,7 +415,7 @@ impl<'a> CrateLocator<'a> { staticlibs .push(CrateMismatch { path: spf.path.clone(), got: "static".to_string() }); } - return FileDoesntMatch; + return; }; info!("lib candidate: {}", spf.path.display()); @@ -423,7 +423,7 @@ impl<'a> CrateLocator<'a> { let (rlibs, rmetas, dylibs) = candidates.entry(hash.to_string()).or_default(); let path = fs::canonicalize(&spf.path).unwrap_or_else(|_| spf.path.clone()); if seen_paths.contains(&path) { - return FileDoesntMatch; + return; }; seen_paths.insert(path.clone()); match found_kind { @@ -431,7 +431,6 @@ impl<'a> CrateLocator<'a> { CrateFlavor::Rmeta => rmetas.insert(path, kind), CrateFlavor::Dylib => dylibs.insert(path, kind), }; - FileMatches }); self.crate_rejections.via_kind.extend(staticlibs); diff --git a/compiler/rustc_session/src/filesearch.rs b/compiler/rustc_session/src/filesearch.rs index 9687d0bd8a701..e6ec16b393b2f 100644 --- a/compiler/rustc_session/src/filesearch.rs +++ b/compiler/rustc_session/src/filesearch.rs @@ -1,7 +1,5 @@ //! A module for searching for libraries -pub use self::FileMatch::*; - use std::env; use std::fs; use std::iter::FromIterator; @@ -45,21 +43,13 @@ impl<'a> FileSearch<'a> { pub fn search(&self, mut pick: F) where - F: FnMut(&SearchPathFile, PathKind) -> FileMatch, + F: FnMut(&SearchPathFile, PathKind), { for search_path in self.search_paths() { debug!("searching {}", search_path.dir.display()); for spf in search_path.files.iter() { debug!("testing {}", spf.path.display()); - let maybe_picked = pick(spf, search_path.kind); - match maybe_picked { - FileMatches => { - debug!("picked {}", spf.path.display()); - } - FileDoesntMatch => { - debug!("rejected {}", spf.path.display()); - } - } + pick(spf, search_path.kind); } } } From 89b61ea09f03060ceac3d0a1779dbb4152b6fddd Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 1 Feb 2022 16:32:13 +1100 Subject: [PATCH 3/7] Inline and remove `FileSearch::search`. It has only a single callsite, and having all the code in one place will make it possible to optimize the search. --- compiler/rustc_metadata/src/locator.rs | 81 ++++++++++++++---------- compiler/rustc_session/src/filesearch.rs | 15 +---- 2 files changed, 47 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index 8db65a10c1386..f65cd73007b34 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -394,44 +394,55 @@ impl<'a> CrateLocator<'a> { // of the crate id (path/name/id). // // The goal of this step is to look at as little metadata as possible. - self.filesearch.search(|spf, kind| { - let file = match &spf.file_name_str { - None => return, - Some(file) => file, - }; - let (hash, found_kind) = if file.starts_with(&rlib_prefix) && file.ends_with(".rlib") { - (&file[(rlib_prefix.len())..(file.len() - ".rlib".len())], CrateFlavor::Rlib) - } else if file.starts_with(&rlib_prefix) && file.ends_with(".rmeta") { - (&file[(rlib_prefix.len())..(file.len() - ".rmeta".len())], CrateFlavor::Rmeta) - } else if file.starts_with(&dylib_prefix) && file.ends_with(&self.target.dll_suffix) { - ( - &file[(dylib_prefix.len())..(file.len() - self.target.dll_suffix.len())], - CrateFlavor::Dylib, - ) - } else { - if file.starts_with(&staticlib_prefix) - && file.ends_with(&self.target.staticlib_suffix) + for search_path in self.filesearch.search_paths() { + debug!("searching {}", search_path.dir.display()); + for spf in search_path.files.iter() { + debug!("testing {}", spf.path.display()); + + let file = match &spf.file_name_str { + None => continue, + Some(file) => file, + }; + let (hash, found_kind) = if file.starts_with(&rlib_prefix) + && file.ends_with(".rlib") { - staticlibs - .push(CrateMismatch { path: spf.path.clone(), got: "static".to_string() }); - } - return; - }; + (&file[(rlib_prefix.len())..(file.len() - ".rlib".len())], CrateFlavor::Rlib) + } else if file.starts_with(&rlib_prefix) && file.ends_with(".rmeta") { + (&file[(rlib_prefix.len())..(file.len() - ".rmeta".len())], CrateFlavor::Rmeta) + } else if file.starts_with(&dylib_prefix) && file.ends_with(&self.target.dll_suffix) + { + ( + &file[(dylib_prefix.len())..(file.len() - self.target.dll_suffix.len())], + CrateFlavor::Dylib, + ) + } else { + if file.starts_with(&staticlib_prefix) + && file.ends_with(&self.target.staticlib_suffix) + { + staticlibs.push(CrateMismatch { + path: spf.path.clone(), + got: "static".to_string(), + }); + } + continue; + }; - info!("lib candidate: {}", spf.path.display()); + info!("lib candidate: {}", spf.path.display()); + + let (rlibs, rmetas, dylibs) = candidates.entry(hash.to_string()).or_default(); + let path = fs::canonicalize(&spf.path).unwrap_or_else(|_| spf.path.clone()); + if seen_paths.contains(&path) { + continue; + }; + seen_paths.insert(path.clone()); + match found_kind { + CrateFlavor::Rlib => rlibs.insert(path, search_path.kind), + CrateFlavor::Rmeta => rmetas.insert(path, search_path.kind), + CrateFlavor::Dylib => dylibs.insert(path, search_path.kind), + }; + } + } - let (rlibs, rmetas, dylibs) = candidates.entry(hash.to_string()).or_default(); - let path = fs::canonicalize(&spf.path).unwrap_or_else(|_| spf.path.clone()); - if seen_paths.contains(&path) { - return; - }; - seen_paths.insert(path.clone()); - match found_kind { - CrateFlavor::Rlib => rlibs.insert(path, kind), - CrateFlavor::Rmeta => rmetas.insert(path, kind), - CrateFlavor::Dylib => dylibs.insert(path, kind), - }; - }); self.crate_rejections.via_kind.extend(staticlibs); // We have now collected all known libraries into a set of candidates diff --git a/compiler/rustc_session/src/filesearch.rs b/compiler/rustc_session/src/filesearch.rs index e6ec16b393b2f..9200be363adde 100644 --- a/compiler/rustc_session/src/filesearch.rs +++ b/compiler/rustc_session/src/filesearch.rs @@ -5,7 +5,7 @@ use std::fs; use std::iter::FromIterator; use std::path::{Path, PathBuf}; -use crate::search_paths::{PathKind, SearchPath, SearchPathFile}; +use crate::search_paths::{PathKind, SearchPath}; use rustc_fs_util::fix_windows_verbatim_for_gcc; use tracing::debug; @@ -41,19 +41,6 @@ impl<'a> FileSearch<'a> { self.get_lib_path().join("self-contained") } - pub fn search(&self, mut pick: F) - where - F: FnMut(&SearchPathFile, PathKind), - { - for search_path in self.search_paths() { - debug!("searching {}", search_path.dir.display()); - for spf in search_path.files.iter() { - debug!("testing {}", spf.path.display()); - pick(spf, search_path.kind); - } - } - } - pub fn new( sysroot: &'a Path, triple: &'a str, From 0ba47f32160d4c51717b4da6449f40079a5eb317 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 2 Feb 2022 13:16:25 +1100 Subject: [PATCH 4/7] Make `SearchPathFile::file_name_str` non-optional. Currently, it can be `None` if the conversion from `OsString` fails, in which case all searches will skip over the `SearchPathFile`. The commit changes things so that the `SearchPathFile` just doesn't get created in the first place. Same behaviour, but slightly simpler code. --- compiler/rustc_metadata/src/locator.rs | 5 +---- compiler/rustc_session/src/search_paths.rs | 26 +++++++++++----------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index f65cd73007b34..caa544284c914 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -399,10 +399,7 @@ impl<'a> CrateLocator<'a> { for spf in search_path.files.iter() { debug!("testing {}", spf.path.display()); - let file = match &spf.file_name_str { - None => continue, - Some(file) => file, - }; + let file = &spf.file_name_str; let (hash, found_kind) = if file.starts_with(&rlib_prefix) && file.ends_with(".rlib") { diff --git a/compiler/rustc_session/src/search_paths.rs b/compiler/rustc_session/src/search_paths.rs index acb6c735e051e..b6bde28233d24 100644 --- a/compiler/rustc_session/src/search_paths.rs +++ b/compiler/rustc_session/src/search_paths.rs @@ -15,22 +15,15 @@ pub struct SearchPath { /// doable, but very slow, because it involves calls to `file_name` and /// `extension` that are themselves slow. /// -/// This type augments the `PathBuf` with an `Option` containing the +/// This type augments the `PathBuf` with an `String` containing the /// `PathBuf`'s filename. The prefix and suffix checking is much faster on the -/// `Option` than the `PathBuf`. (It's an `Option` because -/// `Path::file_name` can fail; if that happens then all subsequent checking -/// will also fail, which is fine.) +/// `String` than the `PathBuf`. (The filename must be valid UTF-8. If it's +/// not, the entry should be skipped, because all Rust output files are valid +/// UTF-8, and so a non-UTF-8 filename couldn't be one we're looking for.) #[derive(Clone, Debug)] pub struct SearchPathFile { pub path: PathBuf, - pub file_name_str: Option, -} - -impl SearchPathFile { - fn new(path: PathBuf) -> SearchPathFile { - let file_name_str = path.file_name().and_then(|f| f.to_str()).map(|s| s.to_string()); - SearchPathFile { path, file_name_str } - } + pub file_name_str: String, } #[derive(PartialEq, Clone, Copy, Debug, Hash, Eq, Encodable, Decodable)] @@ -85,7 +78,14 @@ impl SearchPath { // Get the files within the directory. let files = match std::fs::read_dir(&dir) { Ok(files) => files - .filter_map(|e| e.ok().map(|e| SearchPathFile::new(e.path()))) + .filter_map(|e| { + e.ok().and_then(|e| { + e.file_name().to_str().map(|s| SearchPathFile { + path: e.path(), + file_name_str: s.to_string(), + }) + }) + }) .collect::>(), Err(..) => vec![], }; From 6dcda2aaec79f499f2d515e680a67f9f4b0d2bf9 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Feb 2022 09:36:17 +1100 Subject: [PATCH 5/7] Clean up `find_library_crate`. By introducing prefix and suffix variables for all file types, and renaming some variables. --- compiler/rustc_metadata/src/locator.rs | 40 ++++++++++++-------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index caa544284c914..69725b1377ce7 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -371,11 +371,17 @@ impl<'a> CrateLocator<'a> { extra_prefix: &str, seen_paths: &mut FxHashSet, ) -> Result, CrateError> { - // want: crate_name.dir_part() + prefix + crate_name.file_part + "-" - let dylib_prefix = format!("{}{}{}", self.target.dll_prefix, self.crate_name, extra_prefix); - let rlib_prefix = format!("lib{}{}", self.crate_name, extra_prefix); + let rmeta_prefix = &format!("lib{}{}", self.crate_name, extra_prefix); + let rlib_prefix = rmeta_prefix; + let dylib_prefix = + &format!("{}{}{}", self.target.dll_prefix, self.crate_name, extra_prefix); let staticlib_prefix = - format!("{}{}{}", self.target.staticlib_prefix, self.crate_name, extra_prefix); + &format!("{}{}{}", self.target.staticlib_prefix, self.crate_name, extra_prefix); + + let rmeta_suffix = ".rmeta"; + let rlib_suffix = ".rlib"; + let dylib_suffix = &self.target.dll_suffix; + let staticlib_suffix = &self.target.staticlib_suffix; let mut candidates: FxHashMap<_, (FxHashMap<_, _>, FxHashMap<_, _>, FxHashMap<_, _>)> = Default::default(); @@ -399,23 +405,15 @@ impl<'a> CrateLocator<'a> { for spf in search_path.files.iter() { debug!("testing {}", spf.path.display()); - let file = &spf.file_name_str; - let (hash, found_kind) = if file.starts_with(&rlib_prefix) - && file.ends_with(".rlib") - { - (&file[(rlib_prefix.len())..(file.len() - ".rlib".len())], CrateFlavor::Rlib) - } else if file.starts_with(&rlib_prefix) && file.ends_with(".rmeta") { - (&file[(rlib_prefix.len())..(file.len() - ".rmeta".len())], CrateFlavor::Rmeta) - } else if file.starts_with(&dylib_prefix) && file.ends_with(&self.target.dll_suffix) - { - ( - &file[(dylib_prefix.len())..(file.len() - self.target.dll_suffix.len())], - CrateFlavor::Dylib, - ) + let f = &spf.file_name_str; + let (hash, kind) = if f.starts_with(rlib_prefix) && f.ends_with(rlib_suffix) { + (&f[rlib_prefix.len()..(f.len() - rlib_suffix.len())], CrateFlavor::Rlib) + } else if f.starts_with(rmeta_prefix) && f.ends_with(rmeta_suffix) { + (&f[rmeta_prefix.len()..(f.len() - rmeta_suffix.len())], CrateFlavor::Rmeta) + } else if f.starts_with(dylib_prefix) && f.ends_with(dylib_suffix) { + (&f[dylib_prefix.len()..(f.len() - dylib_suffix.len())], CrateFlavor::Dylib) } else { - if file.starts_with(&staticlib_prefix) - && file.ends_with(&self.target.staticlib_suffix) - { + if f.starts_with(staticlib_prefix) && f.ends_with(staticlib_suffix) { staticlibs.push(CrateMismatch { path: spf.path.clone(), got: "static".to_string(), @@ -432,7 +430,7 @@ impl<'a> CrateLocator<'a> { continue; }; seen_paths.insert(path.clone()); - match found_kind { + match kind { CrateFlavor::Rlib => rlibs.insert(path, search_path.kind), CrateFlavor::Rmeta => rmetas.insert(path, search_path.kind), CrateFlavor::Dylib => dylibs.insert(path, search_path.kind), From 2b8d3dea63f74867d512a9ff50c695512b95a6ba Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Feb 2022 09:56:18 +1100 Subject: [PATCH 6/7] Remove `staticlibs` local variable. --- compiler/rustc_metadata/src/locator.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index 69725b1377ce7..5c81917fc0a82 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -385,7 +385,6 @@ impl<'a> CrateLocator<'a> { let mut candidates: FxHashMap<_, (FxHashMap<_, _>, FxHashMap<_, _>, FxHashMap<_, _>)> = Default::default(); - let mut staticlibs = vec![]; // First, find all possible candidate rlibs and dylibs purely based on // the name of the files themselves. We're trying to match against an @@ -414,7 +413,7 @@ impl<'a> CrateLocator<'a> { (&f[dylib_prefix.len()..(f.len() - dylib_suffix.len())], CrateFlavor::Dylib) } else { if f.starts_with(staticlib_prefix) && f.ends_with(staticlib_suffix) { - staticlibs.push(CrateMismatch { + self.crate_rejections.via_kind.push(CrateMismatch { path: spf.path.clone(), got: "static".to_string(), }); @@ -438,8 +437,6 @@ impl<'a> CrateLocator<'a> { } } - self.crate_rejections.via_kind.extend(staticlibs); - // We have now collected all known libraries into a set of candidates // keyed of the filename hash listed. For each filename, we also have a // list of rlibs/dylibs that apply. Here, we map each of these lists From 2826586b91977ce39eb6f82e68abcf7eb0bc6754 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 4 Feb 2022 13:08:21 +1100 Subject: [PATCH 7/7] Add a comment about possible mismatches. --- compiler/rustc_metadata/src/locator.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index 5c81917fc0a82..550b22a2a3c65 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -399,6 +399,13 @@ impl<'a> CrateLocator<'a> { // of the crate id (path/name/id). // // The goal of this step is to look at as little metadata as possible. + // Unfortunately, the prefix-based matching sometimes is over-eager. + // E.g. if `rlib_suffix` is `libstd` it'll match the file + // `libstd_detect-8d6701fb958915ad.rlib` (incorrect) as well as + // `libstd-f3ab5b1dea981f17.rlib` (correct). But this is hard to avoid + // given that `extra_filename` comes from the `-C extra-filename` + // option and thus can be anything, and the incorrect match will be + // handled safely in `extract_one`. for search_path in self.filesearch.search_paths() { debug!("searching {}", search_path.dir.display()); for spf in search_path.files.iter() {