diff options
| author | bors <bors@rust-lang.org> | 2022-05-18 12:45:44 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2022-05-18 12:45:44 +0000 |
| commit | 936eba3b348e65b658b60218cc9237f02abdbeb4 (patch) | |
| tree | 170ce02aaa1fca72948775fc8adaccf2952f82e7 /compiler/rustc_span/src | |
| parent | e5732a21711e7cefa6eb22e1790406b269d6197a (diff) | |
| parent | 6411fef3aba5ba54a02b54b171b4e9bc83687ce9 (diff) | |
| download | rust-936eba3b348e65b658b60218cc9237f02abdbeb4.tar.gz rust-936eba3b348e65b658b60218cc9237f02abdbeb4.zip | |
Auto merge of #96867 - michaelwoerister:path-prefix-fixes-2, r=davidtwco
--remap-path-prefix: Fix duplicated path components in debuginfo This PR fixes an issue with `--remap-path-prefix` where path components could appear twice in the remapped version of the path (e.g. https://github.com/rust-lang/rust/issues/78479). The underlying problem was that `--remap-path-prefix` is often used to map an absolute path to something that looks like a relative path, e.g.: ``` --remap-path-prefix=/home/calvin/.cargo/registry/src/github.com-1ecc6299db9ec823=crates.io", ``` and relative paths in debuginfo are interpreted as being relative to the compilation directory. So if Cargo invokes the compiler with `/home/calvin/.cargo/registry/src/github.com-1ecc6299db9ec823/some_crate-0.1.0/src/lib.rs` as input and `/home/calvin/.cargo/registry/src/github.com-1ecc6299db9ec823/some_crate-0.1.0` as the compiler's working directory, then debuginfo will state that the working directory was `crates.io/some_crate-0.1.0` and the file is question was `crates.io/some_crate-0.1.0/src/lib.rs`, which combined gives the path: ``` crates.io/some_crate-0.1.0/crates.io/some_crate-0.1.0/src/lib.rs ``` With this PR the compiler will detect this situation and set up debuginfo in LLVM in a way that makes it strip the duplicated path components when emitting DWARF. The PR also extracts the logic for making remapped paths absolute into a common helper function that is now used by debuginfo too (instead of just during crate metadata generation).
Diffstat (limited to 'compiler/rustc_span/src')
| -rw-r--r-- | compiler/rustc_span/src/lib.rs | 6 | ||||
| -rw-r--r-- | compiler/rustc_span/src/source_map.rs | 134 | ||||
| -rw-r--r-- | compiler/rustc_span/src/source_map/tests.rs | 169 |
3 files changed, 246 insertions, 63 deletions
diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs index 7357cebf62e..1f4578c08a3 100644 --- a/compiler/rustc_span/src/lib.rs +++ b/compiler/rustc_span/src/lib.rs @@ -335,8 +335,8 @@ impl fmt::Display for FileNameDisplay<'_> { } } -impl FileNameDisplay<'_> { - pub fn to_string_lossy(&self) -> Cow<'_, str> { +impl<'a> FileNameDisplay<'a> { + pub fn to_string_lossy(&self) -> Cow<'a, str> { match self.inner { FileName::Real(ref inner) => inner.to_string_lossy(self.display_pref), _ => Cow::from(format!("{}", self)), @@ -1153,7 +1153,7 @@ impl FromStr for SourceFileHashAlgorithm { } /// The hash of the on-disk source file used for debug info. -#[derive(Copy, Clone, PartialEq, Eq, Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] #[derive(HashStable_Generic, Encodable, Decodable)] pub struct SourceFileHash { pub kind: SourceFileHashAlgorithm, diff --git a/compiler/rustc_span/src/source_map.rs b/compiler/rustc_span/src/source_map.rs index 505c0af953a..020ae3ad0c7 100644 --- a/compiler/rustc_span/src/source_map.rs +++ b/compiler/rustc_span/src/source_map.rs @@ -1098,28 +1098,45 @@ impl FilePathMapping { /// The return value is the remapped path and a boolean indicating whether /// the path was affected by the mapping. pub fn map_prefix(&self, path: PathBuf) -> (PathBuf, bool) { - // NOTE: We are iterating over the mapping entries from last to first - // because entries specified later on the command line should - // take precedence. - for &(ref from, ref to) in self.mapping.iter().rev() { - if let Ok(rest) = path.strip_prefix(from) { - let remapped = if rest.as_os_str().is_empty() { - // This is subtle, joining an empty path onto e.g. `foo/bar` will - // result in `foo/bar/`, that is, there'll be an additional directory - // separator at the end. This can lead to duplicated directory separators - // in remapped paths down the line. - // So, if we have an exact match, we just return that without a call - // to `Path::join()`. - to.clone() - } else { - to.join(rest) - }; + if path.as_os_str().is_empty() { + // Exit early if the path is empty and therefore there's nothing to remap. + // This is mostly to reduce spam for `RUSTC_LOG=[remap_path_prefix]`. + return (path, false); + } - return (remapped, true); + return remap_path_prefix(&self.mapping, path); + + #[instrument(level = "debug", skip(mapping))] + fn remap_path_prefix(mapping: &[(PathBuf, PathBuf)], path: PathBuf) -> (PathBuf, bool) { + // NOTE: We are iterating over the mapping entries from last to first + // because entries specified later on the command line should + // take precedence. + for &(ref from, ref to) in mapping.iter().rev() { + debug!("Trying to apply {:?} => {:?}", from, to); + + if let Ok(rest) = path.strip_prefix(from) { + let remapped = if rest.as_os_str().is_empty() { + // This is subtle, joining an empty path onto e.g. `foo/bar` will + // result in `foo/bar/`, that is, there'll be an additional directory + // separator at the end. This can lead to duplicated directory separators + // in remapped paths down the line. + // So, if we have an exact match, we just return that without a call + // to `Path::join()`. + to.clone() + } else { + to.join(rest) + }; + debug!("Match - remapped {:?} => {:?}", path, remapped); + + return (remapped, true); + } else { + debug!("No match - prefix {:?} does not match {:?}", from, path); + } } - } - (path, false) + debug!("Path {:?} was not remapped", path); + (path, false) + } } fn map_filename_prefix(&self, file: &FileName) -> (FileName, bool) { @@ -1140,4 +1157,83 @@ impl FilePathMapping { other => (other.clone(), false), } } + + /// Expand a relative path to an absolute path with remapping taken into account. + /// Use this when absolute paths are required (e.g. debuginfo or crate metadata). + /// + /// The resulting `RealFileName` will have its `local_path` portion erased if + /// possible (i.e. if there's also a remapped path). + pub fn to_embeddable_absolute_path( + &self, + file_path: RealFileName, + working_directory: &RealFileName, + ) -> RealFileName { + match file_path { + // Anything that's already remapped we don't modify, except for erasing + // the `local_path` portion. + RealFileName::Remapped { local_path: _, virtual_name } => { + RealFileName::Remapped { + // We do not want any local path to be exported into metadata + local_path: None, + // We use the remapped name verbatim, even if it looks like a relative + // path. The assumption is that the user doesn't want us to further + // process paths that have gone through remapping. + virtual_name, + } + } + + RealFileName::LocalPath(unmapped_file_path) => { + // If no remapping has been applied yet, try to do so + let (new_path, was_remapped) = self.map_prefix(unmapped_file_path); + if was_remapped { + // It was remapped, so don't modify further + return RealFileName::Remapped { local_path: None, virtual_name: new_path }; + } + + if new_path.is_absolute() { + // No remapping has applied to this path and it is absolute, + // so the working directory cannot influence it either, so + // we are done. + return RealFileName::LocalPath(new_path); + } + + debug_assert!(new_path.is_relative()); + let unmapped_file_path_rel = new_path; + + match working_directory { + RealFileName::LocalPath(unmapped_working_dir_abs) => { + let file_path_abs = unmapped_working_dir_abs.join(unmapped_file_path_rel); + + // Although neither `working_directory` nor the file name were subject + // to path remapping, the concatenation between the two may be. Hence + // we need to do a remapping here. + let (file_path_abs, was_remapped) = self.map_prefix(file_path_abs); + if was_remapped { + RealFileName::Remapped { + // Erase the actual path + local_path: None, + virtual_name: file_path_abs, + } + } else { + // No kind of remapping applied to this path, so + // we leave it as it is. + RealFileName::LocalPath(file_path_abs) + } + } + RealFileName::Remapped { + local_path: _, + virtual_name: remapped_working_dir_abs, + } => { + // If working_directory has been remapped, then we emit + // Remapped variant as the expanded path won't be valid + RealFileName::Remapped { + local_path: None, + virtual_name: Path::new(remapped_working_dir_abs) + .join(unmapped_file_path_rel), + } + } + } + } + } + } } diff --git a/compiler/rustc_span/src/source_map/tests.rs b/compiler/rustc_span/src/source_map/tests.rs index 481e015c66c..be827cea874 100644 --- a/compiler/rustc_span/src/source_map/tests.rs +++ b/compiler/rustc_span/src/source_map/tests.rs @@ -313,82 +313,169 @@ impl SourceMapExtension for SourceMap { } } -fn map_path_prefix(mapping: &FilePathMapping, path: &str) -> String { +// Takes a unix-style path and returns a platform specific path. +fn path(p: &str) -> PathBuf { + path_str(p).into() +} + +// Takes a unix-style path and returns a platform specific path. +fn path_str(p: &str) -> String { + #[cfg(not(windows))] + { + return p.into(); + } + + #[cfg(windows)] + { + let mut path = p.replace('/', "\\"); + if let Some(rest) = path.strip_prefix('\\') { + path = ["X:\\", rest].concat(); + } + + path + } +} + +fn map_path_prefix(mapping: &FilePathMapping, p: &str) -> String { // It's important that we convert to a string here because that's what // later stages do too (e.g. in the backend), and comparing `Path` values // won't catch some differences at the string level, e.g. "abc" and "abc/" // compare as equal. - mapping.map_prefix(path.into()).0.to_string_lossy().to_string() + mapping.map_prefix(path(p)).0.to_string_lossy().to_string() } -#[cfg(unix)] #[test] fn path_prefix_remapping() { // Relative to relative { - let mapping = &FilePathMapping::new(vec![("abc/def".into(), "foo".into())]); + let mapping = &FilePathMapping::new(vec![(path("abc/def"), path("foo"))]); - assert_eq!(map_path_prefix(mapping, "abc/def/src/main.rs"), "foo/src/main.rs"); - assert_eq!(map_path_prefix(mapping, "abc/def"), "foo"); + assert_eq!(map_path_prefix(mapping, "abc/def/src/main.rs"), path_str("foo/src/main.rs")); + assert_eq!(map_path_prefix(mapping, "abc/def"), path_str("foo")); } // Relative to absolute { - let mapping = &FilePathMapping::new(vec![("abc/def".into(), "/foo".into())]); + let mapping = &FilePathMapping::new(vec![(path("abc/def"), path("/foo"))]); - assert_eq!(map_path_prefix(mapping, "abc/def/src/main.rs"), "/foo/src/main.rs"); - assert_eq!(map_path_prefix(mapping, "abc/def"), "/foo"); + assert_eq!(map_path_prefix(mapping, "abc/def/src/main.rs"), path_str("/foo/src/main.rs")); + assert_eq!(map_path_prefix(mapping, "abc/def"), path_str("/foo")); } // Absolute to relative { - let mapping = &FilePathMapping::new(vec![("/abc/def".into(), "foo".into())]); + let mapping = &FilePathMapping::new(vec![(path("/abc/def"), path("foo"))]); - assert_eq!(map_path_prefix(mapping, "/abc/def/src/main.rs"), "foo/src/main.rs"); - assert_eq!(map_path_prefix(mapping, "/abc/def"), "foo"); + assert_eq!(map_path_prefix(mapping, "/abc/def/src/main.rs"), path_str("foo/src/main.rs")); + assert_eq!(map_path_prefix(mapping, "/abc/def"), path_str("foo")); } // Absolute to absolute { - let mapping = &FilePathMapping::new(vec![("/abc/def".into(), "/foo".into())]); + let mapping = &FilePathMapping::new(vec![(path("/abc/def"), path("/foo"))]); - assert_eq!(map_path_prefix(mapping, "/abc/def/src/main.rs"), "/foo/src/main.rs"); - assert_eq!(map_path_prefix(mapping, "/abc/def"), "/foo"); + assert_eq!(map_path_prefix(mapping, "/abc/def/src/main.rs"), path_str("/foo/src/main.rs")); + assert_eq!(map_path_prefix(mapping, "/abc/def"), path_str("/foo")); } } -#[cfg(windows)] #[test] -fn path_prefix_remapping_from_relative2() { - // Relative to relative - { - let mapping = &FilePathMapping::new(vec![("abc\\def".into(), "foo".into())]); +fn path_prefix_remapping_expand_to_absolute() { + // "virtual" working directory is relative path + let mapping = + &FilePathMapping::new(vec![(path("/foo"), path("FOO")), (path("/bar"), path("BAR"))]); + let working_directory = path("/foo"); + let working_directory = RealFileName::Remapped { + local_path: Some(working_directory.clone()), + virtual_name: mapping.map_prefix(working_directory).0, + }; + + assert_eq!(working_directory.remapped_path_if_available(), path("FOO")); + + // Unmapped absolute path + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::LocalPath(path("/foo/src/main.rs")), + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("FOO/src/main.rs") } + ); - assert_eq!(map_path_prefix(mapping, "abc\\def\\src\\main.rs"), "foo\\src\\main.rs"); - assert_eq!(map_path_prefix(mapping, "abc\\def"), "foo"); - } + // Unmapped absolute path with unrelated working directory + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::LocalPath(path("/bar/src/main.rs")), + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("BAR/src/main.rs") } + ); - // Relative to absolute - { - let mapping = &FilePathMapping::new(vec![("abc\\def".into(), "X:\\foo".into())]); + // Unmapped absolute path that does not match any prefix + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::LocalPath(path("/quux/src/main.rs")), + &working_directory + ), + RealFileName::LocalPath(path("/quux/src/main.rs")), + ); - assert_eq!(map_path_prefix(mapping, "abc\\def\\src\\main.rs"), "X:\\foo\\src\\main.rs"); - assert_eq!(map_path_prefix(mapping, "abc\\def"), "X:\\foo"); - } + // Unmapped relative path + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::LocalPath(path("src/main.rs")), + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("FOO/src/main.rs") } + ); - // Absolute to relative - { - let mapping = &FilePathMapping::new(vec![("X:\\abc\\def".into(), "foo".into())]); + // Unmapped relative path with `./` + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::LocalPath(path("./src/main.rs")), + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("FOO/src/main.rs") } + ); - assert_eq!(map_path_prefix(mapping, "X:\\abc\\def\\src\\main.rs"), "foo\\src\\main.rs"); - assert_eq!(map_path_prefix(mapping, "X:\\abc\\def"), "foo"); - } + // Unmapped relative path that does not match any prefix + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::LocalPath(path("quux/src/main.rs")), + &RealFileName::LocalPath(path("/abc")), + ), + RealFileName::LocalPath(path("/abc/quux/src/main.rs")), + ); - // Absolute to absolute - { - let mapping = &FilePathMapping::new(vec![("X:\\abc\\def".into(), "X:\\foo".into())]); + // Already remapped absolute path + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::Remapped { + local_path: Some(path("/foo/src/main.rs")), + virtual_name: path("FOO/src/main.rs"), + }, + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("FOO/src/main.rs") } + ); - assert_eq!(map_path_prefix(mapping, "X:\\abc\\def\\src\\main.rs"), "X:\\foo\\src\\main.rs"); - assert_eq!(map_path_prefix(mapping, "X:\\abc\\def"), "X:\\foo"); - } + // Already remapped absolute path, with unrelated working directory + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::Remapped { + local_path: Some(path("/bar/src/main.rs")), + virtual_name: path("BAR/src/main.rs"), + }, + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("BAR/src/main.rs") } + ); + + // Already remapped relative path + assert_eq!( + mapping.to_embeddable_absolute_path( + RealFileName::Remapped { local_path: None, virtual_name: path("XYZ/src/main.rs") }, + &working_directory + ), + RealFileName::Remapped { local_path: None, virtual_name: path("XYZ/src/main.rs") } + ); } |
