Include (potentially remapped) working dir in crate hash

[Aaron1011]

Fixes #85019

A SourceFile created during compilation may have a relative
path (e.g. if rustc itself is invoked with a relative path).
When we write out crate metadata, we convert all relative paths
to absolute paths using the current working directory.

However, the working directory is not included in the crate hash.
This means that the crate metadata can change while the crate
hash remains the same. Among other problems, this can cause a
fingerprint mismatch ICE, since incremental compilation uses
the crate metadata hash to determine if a foreign query is green.

This commit moves the field holding the working directory from
Session to Options, including it as part of the crate hash.

cc @ohsayan

[rust-highfive]

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 8e82f8779b94c4f331f905d0ad1ee2ced86ddd42 with merge 00b86ce65e27b89f376907d04ba69c76ee8fa88f...

[bors]

☀️ Try build successful - checks-actions
Build commit: 00b86ce65e27b89f376907d04ba69c76ee8fa88f (00b86ce65e27b89f376907d04ba69c76ee8fa88f)

[rust-timer]

Queued 00b86ce65e27b89f376907d04ba69c76ee8fa88f with parent 0fa3190, future comparison URL.

[rust-timer]

Finished benchmarking try commit (00b86ce65e27b89f376907d04ba69c76ee8fa88f): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

• Small regression in instruction counts (up to 0.3% on incr-unchanged builds of helloworld)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[cjgillot]

r=me with a comment explaining why the new implementation is safe.

[Aaron1011]

The perf regressions are very small, and this is a correctness fix.

@rustbot label: +perf-regression-triaged

[Aaron1011]

@bors r=cjgillot

[bors]

📌 Commit 47daf829b1a0d2d499745a34cf4b1969929e5f7f has been approved by cjgillot

[bjorn3]

This will result in a different hash for /wd/foo.rs vs /baz/foo.rs with /baz remapped to /wd despite the filename after any remapping being equal.

[Aaron1011]

I don't think so - this field is initialized using the remapped working directory.

[bjorn3]

RealFileName is an enum defined as

#[derive(HashStable_Generic)]
enum RealFileName {
    LocalPath(PathBuf),
    Remapped {
        local_path: Option<PathBuf>,
        virtual_name: PathBuf,
    },
}

while the Hash implementation only looks at the remapped path, the HashStable implementation is derived and looks at which enum variant it is and in case of a remapped path also at the local path before remapping. The dep tracking hash is calculated using HashStable and not Hash, so this will depend if the path is remapped or not and more importantly the local path before remapping, which is even worse that I thought as it breaks reproducible builds.

[Aaron1011]

The dep tracking hash is calculated using HashStable and not Hash

That's incorrect - I'm using impl_dep_tracking_hash_via_hash for RealFileName: https://github.com/Aaron1011/rust/blob/47daf829b1a0d2d499745a34cf4b1969929e5f7f/compiler/rustc_session/src/config.rs#L2513

It looks like the HashStable implementations for FileName and RealFileName are unused, and can be removed. Having Hash, HashStable,andDepTrackingHash` all implemented for a single type is pretty confusing - I wonder if there's any kind of compile-time check we can do to prevent that.

[bors]

⌛ Testing commit 47daf829b1a0d2d499745a34cf4b1969929e5f7f with merge c6dcf24421076fedab2ade1ed496090bba3b2213...

[bors]

💔 Test failed - checks-actions

[Aaron1011]

@bors r=cjgillot

[bors]

📌 Commit 1ac7881 has been approved by cjgillot

[bors]

⌛ Testing commit 1ac7881 with merge bb758a338d556941407cde190135e1cdd948544a...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[Aaron1011]

@bors retry

[bors]

⌛ Testing commit 1ac7881 with merge a183141...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing a183141 to master...

[rust-highfive]

📣 Toolstate changed by #87990!

Tested on commit a183141.
Direct link to PR: #87990

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).