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

Track devirtualized filenames #72767

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented May 30, 2020

Split payload of FileName::Real to track both real and virtualized paths.

(Such splits arise from metadata refs into libstd; the virtualized paths look like /rustc/1.45.0/src/libstd/io/cursor.rs rather than /Users/felixklock/Dev/Mozilla/rust.git/src/libstd/io/cursor.rs)

This way, we can emit the virtual name into things like the like the StableSourceFileId (as was done back before PR #70642) that ends up in incremental build artifacts, while still using the devirtualized file path when we want to access the file.

Fix #70924

@pnkfelix
Copy link
Member Author

cc @eddyb @Mark-Simulacrum

@pnkfelix pnkfelix added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 30, 2020
pnkfelix added 3 commits May 29, 2020 23:34
(and I want to discourage further use of it if possible.)
…ths.

Such splits arise from metadata refs into libstd.

This way, we can (in a follow on commit) continue to emit the virtual name into
things like the like the StableSourceFileId that ends up in incremetnal build
artifacts, while still using the devirtualized file path when we want to access
the file.

Note that this commit is intended to be a refactoring; the actual fix to the bug
in question is in a follow-on commit.
@pnkfelix pnkfelix force-pushed the track-devirtualized-filenames-issue-70924 branch from bc92bbb to 5e5a3d5 Compare May 30, 2020 03:41
@pnkfelix
Copy link
Member Author

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov May 30, 2020
@pnkfelix
Copy link
Member Author

A note on the lack of a regression test for #70924: I don't have a regression test as part of the PR, because I'm not sure how to make one. my local replication relies on either adding/removing the rust-src component in rustup , or actually renaming the rustlib/src/rust subdirectory in my build directory.

(Either of these options don't seem to lend themselves to a test in our infrastructure, no?)

@pnkfelix
Copy link
Member Author

I'm going to unilaterally tag this as beta-accepted, based on the conversation in today's T-compiler meeting

However, as noted in my recent chat with @Mark-Simulacrum: please do wait until after this has been reviewed before backporting it into beta and/or stable. I don't want my unilateral beta-backport approval to inadvertently sidestep the review process entirely.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 30, 2020
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

r=me with or without extra long-term/low-priority FIXME (for removing the /rustc/.../ encoding into paths altogether)

@pnkfelix
Copy link
Member Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented May 30, 2020

📌 Commit a8e4236 has been approved by eddyb

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 30, 2020
@pnkfelix
Copy link
Member Author

@bors rollup=never

(this change is subtle and hard to test, and I want to maximize our ability to bisect to it if necessary.)

@pnkfelix
Copy link
Member Author

@bors p=1

this is a bugfix to a subtle incremental compilation bug that I have seen come up in my own workflow for building rustc itself (ironically, it happened while I was double-checking the intermediate commits of this PR itself were buildable). So I am upping the priority slightly, but not more than the PR's that have also had their priorities increased.

@bors
Copy link
Contributor

bors commented May 31, 2020

⌛ Testing commit a8e4236 with merge 5fd2f06...

@bors
Copy link
Contributor

bors commented May 31, 2020

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 5fd2f06 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 31, 2020
@bors bors merged commit 5fd2f06 into rust-lang:master May 31, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 1, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2020
…imulacrum

[stable] 1.44 release

This includes a release notes update as usual and a backport of rust-lang#72767.

r? @ghost
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 1, 2020

A note on the lack of a regression test for #70924: I don't have a regression test as part of the PR, because I'm not sure how to make one. my local replication relies on either adding/removing the rust-src component in rustup , or actually renaming the rustlib/src/rust subdirectory in my build directory.

(Either of these options don't seem to lend themselves to a test in our infrastructure, no?)

I just realized that I might be able to test this by invoking rustc while overriding the sysroot via --sysroot. I'll look into that, and if it bears fruit, then I'll make a regression test for #70924 in a separate PR.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 5, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 5, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 5, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 5, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 6, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 6, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
RalfJung added a commit to RalfJung/rust that referenced this pull request Jun 6, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 7, 2020
…70924, r=nikomatsakis

run-make regression test for issue rust-lang#70924.

Sometime after my PR rust-lang#72767 (to fix issue rust-lang#70924) landed, I realized that I *could* make a local regression test, thanks to `rustc --print sysroot`: I can make a fresh "copy" (really mostly symlinks) of the sysroot, and then modify it to recreate the terms of this bug.
Comment on lines +107 to +114
if let FileName::Real(real_name) = name {
// rust-lang/rust#70924: Use the stable (virtualized) name when
// available. (We do not want artifacts from transient file system
// paths for libstd to leak into our build artifacts.)
real_name.stable_name().hash(&mut hasher)
} else {
name.hash(&mut hasher);
}
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed a problem with this (although it's unlikely to cause issues in practice): the hashed data could overlap.
A better approach might be to hash tuples that are (0u8, ...) in the first case and (1u8, ...) in the second one.

If we had an Either enum around, producing a value of that and hashing it would be even better (since there would be a single hash call).

bors added a commit to rust-lang-ci/rust that referenced this pull request May 12, 2021
Fix `--remap-path-prefix` not correctly remapping `rust-src` component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#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 rust-lang#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 rust-lang#44940) and `name_was_remapped` (introduced by rust-lang#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
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2021
Fix `--remap-path-prefix` not correctly remapping `rust-src` component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#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 rust-lang#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 rust-lang#44940) and `name_was_remapped` (introduced by rust-lang#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
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request May 27, 2021
Fix `--remap-path-prefix` not correctly remapping `rust-src` component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#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 rust-lang#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 rust-lang#44940) and `name_was_remapped` (introduced by rust-lang#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'rustc' panicked at 'failed to lookup SourceFile in new context'
5 participants