Track devirtualized filenames #72767

pnkfelix · 2020-05-30T03:25:33Z

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 · 2020-05-30T03:25:48Z

cc @eddyb @Mark-Simulacrum

src/librustc_interface/passes.rs

(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.

…in the encoded build artifacts. Fix rust-lang#70924.

pnkfelix · 2020-05-30T03:43:07Z

r? @eddyb

pnkfelix · 2020-05-30T03:48:33Z

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 · 2020-05-30T03:53:43Z

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.

src/librustc_span/lib.rs

eddyb

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

pnkfelix · 2020-05-30T12:20:24Z

@bors r=eddyb

bors · 2020-05-30T12:20:25Z

📌 Commit a8e4236 has been approved by eddyb

pnkfelix · 2020-05-30T12:21:35Z

@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 · 2020-05-30T12:24:52Z

@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 · 2020-05-31T17:14:50Z

⌛ Testing commit a8e4236 with merge 5fd2f06...

bors · 2020-05-31T20:58:49Z

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

@ghost

…imulacrum [stable] 1.44 release This includes a release notes update as usual and a backport of rust-lang#72767. r? @ghost

pnkfelix · 2020-06-01T19:04:06Z

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.

…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.

eddyb · 2020-06-11T15:12:32Z

src/librustc_span/source_map.rs

+        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);
+        }


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).

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

…-component, r=wesleywiser Migrate `incr-add-rust-src-component` to rmake This PR partially supersedes rust-lang#128562, and ports the Makefile-based `tests/run-make/incr-add-rust-src-component` to use rmake.rs infra. Part of rust-lang#121876. This run-make test is a regression test for rust-lang#70924. It (tries to) checks that if we add the `rust-src` component in between two incremental compiles, that the compiler doesn't ICE on the second invocation. - Original issue:rust-lang#70924 - Fix PR: rust-lang#72767 - PR adding this regression test: rust-lang#72952 However, the Makefile version of this used `$SYSROOT/lib/rustlib/src/rust/src/libstd/lib.rs`, but that actually got moved around and reorganized over the years. As of Dec 2024, the `rust-src` component is more like (specific for our purposes): ``` $SYSROOT/lib/rustlib/src/rust/ library/std/src/lib.rs src/ ``` However, this run-make test is ancient and it exercises incr-comp system logic. I'm not sure if this test would actually catch the original regression. This PR was co-authored with `@Oneirical.` r? incremental try-job: i686-msvc try-job: x86_64-mingw-1 try-job: x86_64-msvc try-job: aarch64-apple

Rollup merge of rust-lang#134656 - jieyouxu:migrate-incr-add-rust-src-component, r=wesleywiser Migrate `incr-add-rust-src-component` to rmake This PR partially supersedes rust-lang#128562, and ports the Makefile-based `tests/run-make/incr-add-rust-src-component` to use rmake.rs infra. Part of rust-lang#121876. This run-make test is a regression test for rust-lang#70924. It (tries to) checks that if we add the `rust-src` component in between two incremental compiles, that the compiler doesn't ICE on the second invocation. - Original issue:rust-lang#70924 - Fix PR: rust-lang#72767 - PR adding this regression test: rust-lang#72952 However, the Makefile version of this used `$SYSROOT/lib/rustlib/src/rust/src/libstd/lib.rs`, but that actually got moved around and reorganized over the years. As of Dec 2024, the `rust-src` component is more like (specific for our purposes): ``` $SYSROOT/lib/rustlib/src/rust/ library/std/src/lib.rs src/ ``` However, this run-make test is ancient and it exercises incr-comp system logic. I'm not sure if this test would actually catch the original regression. This PR was co-authored with `@Oneirical.` r? incremental try-job: i686-msvc try-job: x86_64-mingw-1 try-job: x86_64-msvc try-job: aarch64-apple

rust-highfive assigned petrochenkov May 30, 2020

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 commented May 30, 2020

View reviewed changes

src/librustc_interface/passes.rs Outdated Show resolved Hide resolved

pnkfelix added 3 commits May 29, 2020 23:34

StableSourceFileId::new_from_pieces does not need to be public.

905d738

(and I want to discourage further use of it if possible.)

Use the virtual name for libstd files in StableSourceFileId and also …

5e5a3d5

…in the encoded build artifacts. Fix rust-lang#70924.

pnkfelix force-pushed the track-devirtualized-filenames-issue-70924 branch from bc92bbb to 5e5a3d5 Compare May 30, 2020 03:41

rust-highfive assigned eddyb and unassigned petrochenkov May 30, 2020

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

eddyb reviewed May 30, 2020

View reviewed changes

src/librustc_span/lib.rs Show resolved Hide resolved

eddyb approved these changes May 30, 2020

View reviewed changes

add fixme suggested by eddyb

a8e4236

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

bors added the merged-by-bors This PR was explicitly merged by bors. label May 31, 2020

bors merged commit 5fd2f06 into rust-lang:master May 31, 2020

pnkfelix mentioned this pull request May 31, 2020

Beta backport PR #72767 #72842

Closed

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 1, 2020

Mark-Simulacrum mentioned this pull request Jun 1, 2020

[stable] 1.44 release #72883

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 1, 2020

Auto merge of rust-lang#72883 - Mark-Simulacrum:stable-next, r=Mark-S…

49cae55

…imulacrum [stable] 1.44 release This includes a release notes update as usual and a backport of rust-lang#72767. r? @ghost

JohnTitor mentioned this pull request Jun 2, 2020

Rustup to the latest nightly rust-lang/rust-semverver#129

Merged

This was referenced Jun 3, 2020

run-make regression test for issue #70924. #72952

Merged

Get rid of /rustc/$hash hack #72953

Open

eddyb reviewed Jun 11, 2020

View reviewed changes

eddyb mentioned this pull request Jun 11, 2020

Binaries end up containing path to the rust-src component despite --remap-path-prefix #73167

Closed

cbeuw mentioned this pull request Apr 3, 2021

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

Merged

jieyouxu mentioned this pull request Dec 23, 2024

Migrate incr-add-rust-src-component to rmake #134656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track devirtualized filenames #72767

Track devirtualized filenames #72767

pnkfelix commented May 30, 2020 •

edited

Loading

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

eddyb left a comment

pnkfelix commented May 30, 2020

bors commented May 30, 2020

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

bors commented May 31, 2020

bors commented May 31, 2020

pnkfelix commented Jun 1, 2020

eddyb Jun 11, 2020

Track devirtualized filenames #72767

Track devirtualized filenames #72767

Conversation

pnkfelix commented May 30, 2020 • edited Loading

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

eddyb left a comment

Choose a reason for hiding this comment

pnkfelix commented May 30, 2020

bors commented May 30, 2020

pnkfelix commented May 30, 2020

pnkfelix commented May 30, 2020

bors commented May 31, 2020

bors commented May 31, 2020

pnkfelix commented Jun 1, 2020

eddyb Jun 11, 2020

Choose a reason for hiding this comment

pnkfelix commented May 30, 2020 •

edited

Loading