Fix --remap-path-prefix not correctly remapping rust-src component paths and unify handling of path mapping with virtualized paths

[cbeuw]

This PR fixes #73167 ("Binaries end up containing path to the rust-src component despite --remap-path-prefix") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.

RealFileName::Named introduced in #72767 is now renamed as LocalPath, because this variant wraps a (most likely) valid local filesystem path.

RealFileName::Devirtualized is renamed as Remapped to be used for remapped path from a real path via --remap-path-prefix argument, as well as real path inferred from a virtualized (during compiler bootstrapping) /rustc/... path. The local_path field is now an Option<PathBuf>, as it will be set to None before serialisation, so it never reaches any build output. Attempting to serialise a non-None local_path will cause an assertion faliure.

When a path is remapped, a RealFileName::Remapped variant is created. The original path is preserved in local_path field and the remapped path is saved in virtual_name field. Previously, the local_path is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".

rustc_span::SourceFile's fields unmapped_path (introduced by #44940) and name_was_remapped (introduced by #41508 when --remap-path-prefix feature originally added) are removed, as these two pieces of information can be inferred from the name field: if it's anything other than a FileName::Real(_), or if it is a FileName::Real(RealFileName::LocalPath(_)), then clearly name_was_remapped would've been false and unmapped_path would've been None. If it is a FileName::Real(RealFileName::Remapped{local_path, virtual_name}), then name_was_remapped would've been true and unmapped_path would've been Some(local_path).

cc @eddyb who implemented /rustc/... path devirtualisation

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eddyb]

cc @pnkfelix @michaelwoerister

[nagisa]

Could the history be simplified, please?

e.g. 1 commit for adding a test case, rather than 2, combine the Devirtualized changes into one, etc. As the history is right now, its not very straightforward to follow and makes reviewing difficult as the same area of code is changed multiple times.

Furthermore, am I correct that the last 2 commits are not required to fix the issue itself? Could those be split out into a separate PR?

[cbeuw]

@nagisa yeah I probably should've squashed some of them before the PR. I'll reorganise every change so far and force push. So further changes can still be kept as individual commits.

It's true that the bug can be solved without the last two commit and I did contemplate putting up two PRs. But the second PR would have to incorporate all the changes in the first one, so it seems I need to wait until the first PR merges into master first? Another reason was that the state before the last two commits has bits I'm unsure about (for instance, unmaped_path is supposed to be None for imported files, but we are remapping imported file paths, should it be set or left as None - it's better to just remove the field entirely), so I decided to put all changes in one PR to be reviewed together

[michaelwoerister]

Nice! Thanks for the PR, @cbeuw!

@nagisa, @varkor, let me know if you'd like me to take a closer look at the fix.

[nagisa]

Please do! My primary concern is basically that this implements repeated remapping, whereas in the linked issue @eddyb said:

I don't think --remap-path-prefix should remap paths that were already remapped

I think implementation wise this is fine. Perhaps what we need is a fcp merge?

[cbeuw]

@nagisa Regarding repeated remapping, I think there are two senarios:

1. Remapping a /rustc/$hash/... path which has been remapped during compiler bootstraping. Although it doesn't seem very useful, I don't see why we shouldn't allow this. It might be used (in a very contrived manner) to manually remap it back to a real path on the system for a sysroot crate without installing rust-src.
2. Remapping paths embedded in the compilation output of another crate, which may have already been remapped. This is what src/test/codegen/remap_path_prefix/double-remap-imported.rs is testing. This shouldn't happen if rustc is invoked by cargo, as it passes the same RUSTFLAGS to all invocation to rustc - that is unless the user specifies multiple --remap-path-prefixes with the to path of one matching from path of another, e.g. --remap-path-prefix=a=b --remap-path-prefix=b=c. It shouldn't break anything and I don't think we should place artificial restrictions on this.

[varkor]

@nagisa, @varkor, let me know if you'd like me to take a closer look at the fix.

I'm very busy at the moment, so if you'd be happy to complete the review, that would be great :)

r? @michaelwoerister

[michaelwoerister]

OK, I'll take over the review 👍

[michaelwoerister]

@eddyb Do you think that the functionality in #70642 could also be implemented by making Rustup's rustc wrapper binary inject an appropriate --remap-path-prefix option?

[eddyb]

@eddyb Do you think that the functionality in #70642 could also be implemented by making Rustup's rustc wrapper binary inject an appropriate --remap-path-prefix option?

No, I think that's way too invasive. I'm not sure if this was clear to everyone but part of the reason for making it sysroot-relative is that distro packages can also supply it, as long as they follow the same path hierarchy that rust-src has (AFAIK make install / x.py install should be enough?).

Maybe the hash and/or version should be part of the src paths in rust-src itself? Then the whole /rustc/<hash> hack could be replaced with just something like $sysroot - ofc this would break a bunch of tools, but it would also make it more clear what is going on.

Anyway I'm not super sure why the whole virtualized path thing is even relevant.
Its purpose is to allow diagnostics to point to an actual file to improve UX (we could even hide the path and only use it to show the code, really, but then the user wouldn't have access to wider context). The "devirtualized local path" should never end up in file!()/#[track_caller]/debuginfo, and if it does that's a bug IMO.

[michaelwoerister]

I'm still in the process of finding out what the status quo is and how this PR modifies it. My findings so far are:

• Translate the virtual /rustc/$hash prefix back to a real directory. #70642 (Translate the virtual /rustc/$hash prefix back to a real directory) in conjunction with Track devirtualized filenames #72767 (Track devirtualized filenames) seem to do the right thing in that they preserve the stable version of imported file names, so in theory it's still available for emitting to panic messages, debuginfo, etc.
• Track devirtualized filenames #72767, however, does not seem to adapt debuginfo generation code for file names. If I read this correctly, Filename::to_string() will fall back on the Display implementation which gives the local_path. I would expect that this causes Binaries end up containing path to the rust-src component despite --remap-path-prefix #73167 for debuginfo. I don't know yet if there are other such cases, such as the file!() macro.
• The current reliance on Display for Filename makes it easy to accidentally end up using the wrong version.
• SourceFile::unmapped_path and FileName::Devirtualized::local_path seem to overlap in functionality.
• The term "virtual" seems to be used with two different meanings in this context, once for remapped/stable paths and once for compiler-generated source files.

Currently it does not look to me like we need remapping of imported paths (i.e. "double remapping") to fix #73167. The compiler has the stable name of upstream file names available. I would opt for trying to fix the existing bugs without introducing new functionality (which would probably need a major change proposal).

That being said, from the description of the PR it sounds like it contains some nice cleanup refactorings. I'll take a closer look soon.

[michaelwoerister]

No, I think that's way too invasive. I'm not sure if this was clear to everyone but part of the reason for making it sysroot-relative is that distro packages can also supply it ...

Yes, that makes sense to me.

[cbeuw]

Currently there isn't a impl ToString for FileName, hence it falls back to impl Display for FileName which returns the local_name for (de)virtualised variant:

rust/compiler/rustc_span/src/lib.rs

Lines 196 to 201 in 1c158b6

	// FIXME: might be nice to display both components of Devirtualized.
	// But for now (to backport fix for issue #70924), best to not
	// perturb diagnostics so its obvious test suite still works.
	Real(RealFileName::Devirtualized { ref local_path, virtual_name: _ }) => {
	write!(fmt, "{}", local_path.display())
	}

There's one place where FileName::to_string() is used for debuginfo:

rust/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

Lines 761 to 774 in 1c158b6

	pub fn file_metadata(cx: &CodegenCx<'ll, '_>, source_file: &SourceFile) -> &'ll DIFile {
	debug!("file_metadata: file_name: {}", source_file.name);
	let hash = Some(&source_file.src_hash);
	let file_name = Some(source_file.name.to_string());
	let directory = if source_file.is_real_file() && !source_file.is_imported() {
	Some(cx.sess().working_dir.0.to_string_lossy().to_string())
	} else {
	// If the path comes from an upstream crate we assume it has been made
	// independent of the compiler's working directory one way or another.
	None
	};
	file_metadata_raw(cx, file_name, directory, hash)
	}

And used for panic location:

rust/compiler/rustc_codegen_ssa/src/mir/block.rs

Lines 1150 to 1154 in 1c158b6

	let const_loc = tcx.const_caller_location((
	Symbol::intern(&caller.file.name.to_string()),
	caller.line as u32,
	caller.col_display as u32 + 1,
	));

It's probably appropirate to manualy do impl ToString for FileName and leave impl Display for FileName exclusively responsible for diagnostics.

[michaelwoerister]

Great find about the panic locations, @cbeuw! These are exactly the things we need to look for. I wonder if we can remove the Display implementation for FileName and require users to explicitly choose if they want the local or the stable version. It's really easy to get this wrong at the moment.

[michaelwoerister]

Thank you for all the hard work you have been putting into this, @cbeuw!