Include rustc version in `rustc_span::StableCrateId` #89836

pierwill · 2021-10-13T00:17:25Z

rustc_span::def_id::StableCrateId is a hash of various data about a crate during compilation. This PR includes the version of rustc in the input when computing this hash. From a cursory reading of RFC 2603, this appears to be acceptable within that design.

In order to pass the mir-opt and ui test suites, this adds new normalization for hashes and symbol names in compiletest. These are enabled by default, but we might prefer it to be configurable.

In the UI tests, I had to truncate a significant amount of error annotations in v0 symbols (and maybe some legacy) in order to get the normalization to work correctly. (See #90116.)

Closes #85142.

rust-highfive · 2021-10-13T00:17:28Z

r? @nagisa

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

pierwill · 2021-10-13T00:17:45Z

@bjorn3

src/test/ui/consts/miri_unleashed/tls.stderr

src/test/ui/symbol-names/basic.rs

nagisa

There are still a number of tests that seem like they would need some normalization to not require updating every time trains roll over?

nagisa · 2021-10-16T20:23:19Z

compiler/rustc_span/src/def_id.rs

        metadata.sort();
-        // Every distinct -C metadata value is only incorporated once:
        metadata.dedup();


Preexisting, but I feel like this logic is better placed in argument parsing, so that this is applied everywhere and consistently. As it is right now, it is very plausible that other places in the code won't dedup and consider -Cmetadata=a distinct from -Cmetadata=a -Cmetadata=a.

I believe this is the only code reading -Cmetadata. I would rather prefer to keep all logic for computing the stable crate id together like it is now.

src/test/mir-opt/retag.main.SimplifyCfg-elaborate-drops.after.mir

src/test/mir-opt/retag.array_casts.SimplifyCfg-elaborate-drops.after.mir

bors · 2021-12-15T17:35:38Z

💔 Test failed - checks-actions

pierwill · 2021-12-15T17:39:51Z

Grrr! 🤯 #89836 (comment):

- thread 'main' panicked at 'd was called', $DIR/panic-short-backtrace-windows-x86_64.rs:48:5
+ thread 'main' panicked at 'd was called', $DIR/panic-short-backtrace-windows-x86_64.rs:43:5
3    0: std::panicking::begin_panic
4    1: d

5    2: c
5    2: c
6    3: b
7    4: a
+    5: <T as core::any::Any>::type_id
8 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
9

pierwill · 2021-12-15T18:20:02Z

Guess we need to keep some normalization the panic-short-backtrace-windows-x86_64.rs test?

wesleywiser · 2021-12-15T20:24:55Z

It's the same target x86_64-pc-windows-msvc as before so I'd suggest just undoing that change. It's possible some other PR has landed in the mean time which rejiggers the layout of functions in the test binary enough that we (once again) have some function other than __rust_begin_short_backtrace as the 5th frame in the stack.

pierwill · 2021-12-15T21:22:37Z

Done. 👍

wesleywiser · 2021-12-15T21:46:42Z

@bors r+

bors · 2021-12-15T21:46:43Z

📌 Commit 535278a has been approved by wesleywiser

bors · 2021-12-16T02:21:39Z

⌛ Testing commit 535278a with merge 9e1aff8...

bors · 2021-12-16T05:17:21Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 9e1aff8 to master...

rust-timer · 2021-12-16T06:47:45Z

Finished benchmarking commit (9e1aff8): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -11.6% on incr-unchanged builds of tuple-stress)
Large regression in instruction counts (up to 4.7% on incr-unchanged builds of unicode_normalization)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-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 open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Since rust-lang/rust#89836, rustc stable crate IDs include a hash of the rustc version (including the git commit it's built from), which means that hashmaps or other structures have different behavior when comparing different rustc builds. This is bad for rustc-perf, as it means that comparing two commits has a source of noise that makes it harder to know what the actual change between two artifacts is.

rust-highfive assigned nagisa Oct 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2021