Apply simulate-remapped-rust-src-base even if remap-debuginfo is set in config.toml #110699

jyn514 · 2023-04-22T19:33:43Z

This is really a mess. Here is the situation before this change:

UI tests depend on not having rust-src available. In particular,

rust/tests/ui/tuple/wrong_argument_ice.stderr

Lines 7 to 8 in 3f37412

note: method defined here

--> $SRC_DIR/alloc/src/collections/vec_deque/mod.rs:LL:COL

is depending on the note being a single line and not showing the source code.
When download-rustc is disabled, we pass -Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX -Ztranslate-remapped-path-to-local-path=no, which changes the diagnostic to something like --> /rustc/FAKE_PREFIX/library/alloc/src/collections/vec_deque/mod.rs:1657:12
When download-rustc is enabled, we still pass those flags, but they no longer have an effect. Instead rustc emits diagnostic paths like this: --> /rustc/39c6804b92aa202369e402525cee329556bc1db0/library/alloc/src/collections/vec_deque/mod.rs:1657:12. Notice how there's a real commit and not FAKE_PREFIX. This happens because we set CFG_VIRTUAL_RUST_SOURCE_BASE_DIR during bootstrapping for CI artifacts, and rustc previously didn't allow for simulate-remapped to affect paths that had already been remapped.
Pietro noticed this and decided the right thing was to normalize /rustc/<commit> to $SRC_DIR in compiletest: 470423c
After my change to x test core, which rebuilds stage 2 std from source so build/stage2-std and build/stage2 use the same .rlib metadata, the compiler suddenly notices it has sources for std available and prints those in the diagnostic, causing the test to fail.

This changes simulate-remapped-rust-src-base to support remapping paths that have already been remapped, unblocking download-rustc.

Unfortunately, although this fixes the specific problem for
download-rustc, it doesn't seem to affect all the compiler's
diagnostics. In particular, various mir-opt tests are failing to
respect simulate-remapped-path-prefix (I looked into fixing this but
it seems non-trivial). As a result, we can't remove the normalization in
compiletest that maps /rustc/<commit> to $SRC_DIR, so this change is
currently untested anywhere except locally.

You can test this locally yourself by setting rust.remap-debuginfo = true, running any UI test with ERROR annotations, then rerunning the test manually with a dev toolchain to verify it prints /rustc/FAKE_PREFIX, not /rustc/1.71.0.

Helps with #110352.

… in config.toml This is really a mess. Here is the situation before this change: - UI tests depend on not having `rust-src` available. In particular, <https://github.com/rust-lang/rust/blob/3f374128ee3924514aacadf96479e17fee8f9903/tests/ui/tuple/wrong_argument_ice.stderr#L7-L8> is depending on the `note` being a single line and not showing the source code. - When `download-rustc` is disabled, we pass `-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX` `-Ztranslate-remapped-path-to-local-path=no`, which changes the diagnostic to something like ` --> /rustc/FAKE_PREFIX/library/alloc/src/collections/vec_deque/mod.rs:1657:12` - When `download-rustc` is enabled, we still pass those flags, but they no longer have an effect. Instead rustc emits diagnostic paths like this: ` --> /rustc/39c6804b92aa202369e402525cee329556bc1db0/library/alloc/src/collections/vec_deque/mod.rs:1657:12`. Notice how there's a real commit and not `FAKE_PREFIX`. This happens because we set `CFG_VIRTUAL_RUST_SOURCE_BASE_DIR` during bootstrapping for CI artifacts, and rustc previously didn't allow for `simulate-remapped` to affect paths that had already been remapped. - Pietro noticed this and decided the right thing was to normalize `/rustc/<commit>` to `$SRC_DIR` in compiletest: rust-lang@470423c - After my change to `x test core`, which rebuilds stage 2 std from source so `build/stage2-std` and `build/stage2` use the same `.rlib` metadata, the compiler suddenly notices it has sources for `std` available and prints those in the diagnostic, causing the test to fail. This changes `simulate-remapped-rust-src-base` to support remapping paths that have already been remapped, unblocking download-rustc. Unfortunately, although this fixes the specific problem for download-rustc, it doesn't seem to affect all the compiler's diagnostics. In particular, various `mir-opt` tests are failing to respect `simulate-remapped-path-prefix` (I looked into fixing this but it seems non-trivial). As a result, we can't remove the normalization in compiletest that maps `/rustc/<commit>` to `$SRC_DIR`, so this change is currently untested anywhere except locally.

rustbot · 2023-04-22T19:33:49Z

r? @lcnr

(rustbot has picked a reviewer for you, use r? to override)

compiler/rustc_metadata/src/rmeta/decoder.rs

jyn514 · 2023-04-22T20:08:37Z

@bors try

bors · 2023-04-22T20:08:47Z

⌛ Trying commit 8785615 with merge 0f46b049864f2d971a84a3c92be916b1bbb1c842...

bors · 2023-04-22T21:49:12Z

☀️ Try build successful - checks-actions
Build commit: 0f46b049864f2d971a84a3c92be916b1bbb1c842 (0f46b049864f2d971a84a3c92be916b1bbb1c842)

jyn514 · 2023-04-23T22:43:43Z

I tried to test locally that this fixes x test ui on #110701. Unfortunately, I can't actually reproduce the original error this was fixing :/ I'm now correct seeing /rustc/FAKE_PREFIX, which wasn't what happened originally. I'm not sure if this is non-deterministic somehow or whether it was fixed when I rebased over master (somehow??).

lcnr · 2023-04-24T07:02:22Z

r? compiler

cjgillot · 2023-05-01T17:58:20Z

Half this function looks like a gigantic hack to ease ui testing.
I don't see any other reason to remap filenames when decoding them. That should be the exclusive responsibility of the metadata encoder to chose the information to expose to crate consumers.

I'm under the impression that there are multiple crates that attempt to do this manipulation. In metadata decoder, but also the path remapping logic in rustc_span::source_map.

Should the downloaded rustc output FAKE_PREFIX instead of its commit hash?
Is there a way to test that this solves the ui testing issue?

jyn514 · 2023-05-01T18:04:22Z

Should the downloaded rustc output FAKE_PREFIX instead of its commit hash?

This is the way to get it to output FAKE_PREFIX. The downloaded rustc is exactly identical to a nightly, it can't unconditionally output FAKE_PREFIX so it has to be configured through simulate-remapped-rust-src-base.

Is there a way to test that this solves the ui testing issue?

See my comment above:

Unfortunately, although this fixes the specific problem for
download-rustc, it doesn't seem to affect all the compiler's
diagnostics. In particular, various mir-opt tests are failing to
respect simulate-remapped-path-prefix (I looked into fixing this but
it seems non-trivial). As a result, we can't remove the normalization in
compiletest that maps /rustc/<commit> to $SRC_DIR, so this change is
currently untested anywhere except locally.

jyn514 · 2023-05-01T18:10:05Z

I don't see any other reason to remap filenames when decoding them. That should be the exclusive responsibility of the metadata encoder to chose the information to expose to crate consumers.

How should this handle files in the standard library? If we require passing simulate-remapped during the initial compile, we'd have to build the standard library twice - once for UI testing with the flag and once without.

cjgillot · 2023-05-07T12:53:22Z

Thanks for the explanations.
@bors r+

bors · 2023-05-07T12:53:24Z

📌 Commit 8785615 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-05-10T13:20:53Z

⌛ Testing commit 8785615 with merge 2b3db67e9cf0b4503cd5464639832198a27f00fb...

bors · 2023-05-10T13:25:07Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-05-10T13:38:37Z

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

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

jyn514 · 2023-05-10T14:15:18Z

Looks spurious? Only failures are in arm-android and armhf-gnu, and they failed in the first 30 seconds without logs.

@bors retry

bors · 2023-05-13T10:11:02Z

⌛ Testing commit 8785615 with merge ebf2b37...

bors · 2023-05-13T12:47:02Z

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

rust-timer · 2023-05-13T14:06:07Z

Finished benchmarking commit (ebf2b37): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	2
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-4.7%, -1.0%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 659.229s -> 660.401s (0.18%)

Stop normalizing so many different prefixes Previously, we would normalize *all* of - the absolute path to the repository checkout - the /rustc/$sha for stage1 (if `remap-debuginfo` was enabled) - the /rustc/$sha for download-rustc - the sysroot for download-rustc Now, we consistently only normalize /rustc/FAKE_PREFIX. Not only is this much simpler, but it also avoids ongoing maintenance for download-rustc and makes it much less likely that tests break by accident. - Change `tests/ui/track-diagnostics/track6.rs` to use a relative path instead of an absolute one. I am not actually sure why `track_caller` works here, but it does seem to work 🤷 - Pass `-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX` to all suites, not just UI. In particular, mir-opt tests emit /rustc/ paths in their output. r? `@cjgillot` since you reviewed rust-lang#110699 - this is the test that it doesn't regress :)

Stop normalizing so many different prefixes Previously, we would normalize *all* of - the absolute path to the repository checkout - the /rustc/$sha for stage1 (if `remap-debuginfo` was enabled) - the /rustc/$sha for download-rustc - the sysroot for download-rustc Now, we consistently only normalize /rustc/FAKE_PREFIX. Not only is this much simpler, but it also avoids ongoing maintenance for download-rustc and makes it much less likely that tests break by accident. - Change `tests/ui/track-diagnostics/track6.rs` to use a relative path instead of an absolute one. I am not actually sure why `track_caller` works here, but it does seem to work 🤷 - Pass `-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX` to all suites, not just UI. In particular, mir-opt tests emit /rustc/ paths in their output. r? ``@cjgillot`` since you reviewed rust-lang#110699 - this is the test that it doesn't regress :)

Stop normalizing so many different prefixes Previously, we would normalize *all* of - the absolute path to the repository checkout - the /rustc/$sha for stage1 (if `remap-debuginfo` was enabled) - the /rustc/$sha for download-rustc - the sysroot for download-rustc Now, we consistently only normalize /rustc/FAKE_PREFIX. Not only is this much simpler, but it also avoids ongoing maintenance for download-rustc and makes it much less likely that tests break by accident. - Change `tests/ui/track-diagnostics/track6.rs` to use a relative path instead of an absolute one. I am not actually sure why `track_caller` works here, but it does seem to work 🤷 - Pass `-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX` to all suites, not just UI. In particular, mir-opt tests emit /rustc/ paths in their output. r? ```@cjgillot``` since you reviewed rust-lang#110699 - this is the test that it doesn't regress :)

jyn514 added 2 commits April 22, 2023 11:56

Decrease the indentation in imported_source_file

919b391

rustbot assigned lcnr Apr 22, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 22, 2023

jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-testsuite Area: The testsuite used to check the correctness of rustc labels Apr 22, 2023

jyn514 mentioned this pull request Apr 22, 2023

Fix x test core when download-rustc is enabled #110701

Merged

jyn514 commented Apr 22, 2023

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs Show resolved Hide resolved

rustbot assigned cjgillot and unassigned lcnr Apr 24, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 13, 2023

bors merged commit ebf2b37 into rust-lang:master May 13, 2023

rustbot added this to the 1.71.0 milestone May 13, 2023

jyn514 deleted the simulate-remapped-already-remapped branch May 18, 2023 05:39

jyn514 mentioned this pull request May 26, 2023

Stop normalizing so many different prefixes #111975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply simulate-remapped-rust-src-base even if remap-debuginfo is set in config.toml #110699

Apply simulate-remapped-rust-src-base even if remap-debuginfo is set in config.toml #110699

jyn514 commented Apr 22, 2023 •

edited

Loading

rustbot commented Apr 22, 2023

jyn514 commented Apr 22, 2023

bors commented Apr 22, 2023

bors commented Apr 22, 2023

jyn514 commented Apr 23, 2023

lcnr commented Apr 24, 2023

cjgillot commented May 1, 2023

jyn514 commented May 1, 2023

jyn514 commented May 1, 2023

cjgillot commented May 7, 2023

bors commented May 7, 2023

bors commented May 10, 2023

bors commented May 10, 2023

rust-log-analyzer commented May 10, 2023

jyn514 commented May 10, 2023

bors commented May 13, 2023

bors commented May 13, 2023

rust-timer commented May 13, 2023

	note: method defined here
	--> $SRC_DIR/alloc/src/collections/vec_deque/mod.rs:LL:COL

Apply simulate-remapped-rust-src-base even if remap-debuginfo is set in config.toml #110699

Apply simulate-remapped-rust-src-base even if remap-debuginfo is set in config.toml #110699

Conversation

jyn514 commented Apr 22, 2023 • edited Loading

rustbot commented Apr 22, 2023

jyn514 commented Apr 22, 2023

bors commented Apr 22, 2023

bors commented Apr 22, 2023

jyn514 commented Apr 23, 2023

lcnr commented Apr 24, 2023

cjgillot commented May 1, 2023

jyn514 commented May 1, 2023

jyn514 commented May 1, 2023

cjgillot commented May 7, 2023

bors commented May 7, 2023

bors commented May 10, 2023

bors commented May 10, 2023

rust-log-analyzer commented May 10, 2023

jyn514 commented May 10, 2023

bors commented May 13, 2023

bors commented May 13, 2023

rust-timer commented May 13, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

jyn514 commented Apr 22, 2023 •

edited

Loading