`-Z simulate-remapped-rust-src-base` is not fully simulating `remap-debuginfo = true`

[japaric]

alternative title: -Z simulate-remapped-rust-src-base is not working as expected with remap-debuginfo = true

Hello! We are running the compiler test with a compiler configured with remap-debuginfo = true and after a recent PR merge we observed that the src/test/ui test suite is no longer passing. After looking into the issue I think a (test only?) -Z compiler flag may not be working as intended. Details below and apologies for not using one of the issue templates!

Observations

./x.py test src/test/ui worked fine before PR #97504 with remap-debuginfo = true set in config.toml

$ git clone https://github.com/rust-lang/rust
$ cd rust
$ git checkout 946a88a # before PR 97504
$ git submodule update

$ cp config.toml.example config.toml
$ # enable remap-debuginfo
$ sed -i '/remap-debuginfo/c\remap-debuginfo = true' config.toml

$ ./x.py test src/test/ui && echo OK
(..)
OK

./x.py test src/test/ui stopped working after #97504 was merged.

$ git checkout e810f75 # after PR 97504
$ git submodule update

$ cp config.toml.example config.toml
$ # enable remap-debuginfo
$ sed -i '/remap-debuginfo/c\remap-debuginfo = true' config.toml

$ ./x.py test src/test/ui
(..)
---- [ui] src/test/ui/span/issue-71363.rs stdout ----
diff of stderr:

1	error[E0277]: `MyError` doesn't implement `std::fmt::Display`
-	 --> $DIR/issue-71363.rs:6:6
-	  |
-	6 | impl std::error::Error for MyError {}
-	  |      ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted with the default formatter
-	  |
-	  = help: the trait `std::fmt::Display` is not implemented for `MyError`
-	  = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
+	   --> $DIR/issue-71363.rs:6:6
+	    |
+	6   | impl std::error::Error for MyError {}
+	    |      ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted with the default formatter
+	    |
+	    = help: the trait `std::fmt::Display` is not implemented for `MyError`
+	    = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
9	note: required by a bound in `std::error::Error`
+	   --> $SRC_DIR/std/src/error.rs:LL:COL
+	    |
+	193 | pub trait Error: Debug + Display {
+	    |                          ^^^^^^^ required by this bound in `std::error::Error`
10
11	error[E0277]: `MyError` doesn't implement `Debug`
-	 --> $DIR/issue-71363.rs:6:6
-	  |
-	6 | impl std::error::Error for MyError {}
-	  |      ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted using `{:?}`
-	  |
-	  = help: the trait `Debug` is not implemented for `MyError`
-	  = note: add `#[derive(Debug)]` to `MyError` or manually `impl Debug for MyError`
+	   --> $DIR/issue-71363.rs:6:6
+	    |
+	6   | impl std::error::Error for MyError {}
+	    |      ^^^^^^^^^^^^^^^^^ `MyError` cannot be formatted using `{:?}`
+	    |
+	    = help: the trait `Debug` is not implemented for `MyError`
+	    = note: add `#[derive(Debug)]` to `MyError` or manually `impl Debug for MyError`
19	note: required by a bound in `std::error::Error`
+	   --> $SRC_DIR/std/src/error.rs:LL:COL
+	    |
+	193 | pub trait Error: Debug + Display {
+	    |                  ^^^^^ required by this bound in `std::error::Error`
20	help: consider annotating `MyError` with `#[derive(Debug)]`
-	  |
-	5 | #[derive(Debug)]
-	  |
+	    |
+	5   | #[derive(Debug)]
+	    |
24
25	error: aborting due to 2 previous errors
26
(..)

more concretely, the UI test issue-71363 added in PR #97504 fails. the error message looks like the one that was reported by CI in the original PR #89268. see rust-log-analyzer

disabling remap-debuginfo makes test issue-71363 and the test suite pass:

$ # disable remap-debuginfo
$ sed -i '/remap-debuginfo/c\remap-debuginfo = false' config.toml

$ ./x.py test src/test/ui && echo OK
(..)
OK

Hypotheses

1. -Z simulate-remapped-rust-src-base is not working as intended with remap-debuginfo = true.

AIUI, the goal of -Z simulate-remapped-rust-src-base=/rustc/xyz is to make it as if there was no rust-src component installed but it's not working when the compiler is built with remap-debuginfo = true. This can be observed in the above test failure: the location of trait Error is reported. It can also be observed on nightly (shown below):

$ rustup default nightly-2022-06-02
$ rustup component add rust-src

$ cd src/test/ui/span # within rust-lang/rust repo
$ rustc -Z ui-testing=no -Z simulate-remapped-rust-src-base=/rustc/xyz issue-71363.rs
(..)
note: required by a bound in `std::error::Error`
   --> $RUST_SRC/library/std/src/error.rs:193:18
    |
193 | pub trait Error: Debug + Display {

only removing the rust-src component makes that pub trait note disappear.

$ rustup component remove rust-src

$ rustc -Z ui-testing=no -Z simulate-remapped-rust-src-base=/rustc/xyz issue-71363.rs 2>&1 | rg 'pub trait' || echo not found
not found

$ rustc issue-71363.rs 2>&1 | rg 'pub trait' || echo not found
not found

2. issue-71363 is not being exercised with remap-debuginfo = true in CI

PR #97504 rebased PR #89268 but added // only-x86_64-unknown-linux-gnu. with that change the issue-71363 test is run in the auto (x86_64-gnu, ubuntu-20.04-xl) builder which is configured with the default of remap-debuginfo = false. (see temporary logs)

I couldn't figure where in the source code remap-debuginfo is set for CI builders but for example auto (dist-x86_64-linux, ubuntu-20.04-xl) is configured with remap-debuginfo = true (see temporary logs) but that builder does not run the src/test/ui tests.

OTOH, auto (dist-i586-gnu-i586-i686-musl, ubuntu-20.04-xl) is configured with remap-debuginfo = true (see temporary logs) and does run the src/test/ui tests. However, it will not run issue-71363 (see temp logs) because the test has a // only-x86_64-unknown-linux-gnu directive. This dist-i586 builder is the one that failed in the original PR #89268 (again, see rust-log-analyzer); that original PR did not have the // only- directive.

Conclusions

1. -Z simulate-remapped-rust-src-base is not working as intended with remap-debuginfo = true.
2. because of (1), issue-71363 is a hazard because it will fail if the remap-debuginfo configuration of the CI builders is tweaked in the future

Suggestions

I would suggest:

1. remove / disable the UI test issue-71363
2. re-open issue Regression in spacing of left margin in diagnostics #71363 and label it with needs-test. note that the actual issue is already fixed
3. open an issue about fixing -Z simulate-remapped-rust-src-base (or re-purporse this issue for that)
4. re-add / re-enable test issue-71363 once (3) is sorted out

cc @JohnTitor (author of PR #97504)

[JohnTitor]

Thanks for reporting! Totally agree with your suggestions, I'm going to open a PR to disable that UI test later.

[JohnTitor]

Opened #97750, and retitled this issue as the issue is not limited to UI test, I think.

[pietroalbini]

I couldn't figure where in the source code remap-debuginfo is set for CI builders

remap-debuginfo is set when the builder deploys dist artifacts.

[pietroalbini]

Found the cause of the mismatch between remap-debuginfo = true and remap-debuginfo = false!

When -Z simulate-remapped-rust-src-base is passed to the compiler, when loading the standard library's rlib/rmeta file paths are remapped on the fly, reproducing the same behavior of remap-debuginfo = true. This is correct.

rust/compiler/rustc_metadata/src/rmeta/decoder.rs

Lines 1578 to 1601 in 9a74608

	// If this file is under $sysroot/lib/rustlib/src/ but has not been remapped
	// during rust bootstrapping by `remap-debuginfo = true`, and the user
	// wish to simulate that behaviour by -Z simulate-remapped-rust-src-base,
	// then we change `name` to a similar state as if the rust was bootstrapped
	// with `remap-debuginfo = true`.
	// This is useful for testing so that tests about the effects of
	// `try_to_translate_virtual_to_real` don't have to worry about how the
	// compiler is bootstrapped.
	if let Some(virtual_dir) =
	&sess.opts.debugging_opts.simulate_remapped_rust_src_base
	{
	if let Some(real_dir) = &sess.opts.real_rust_source_base_dir {
	if let rustc_span::FileName::Real(ref mut old_name) = name {
	if let rustc_span::RealFileName::LocalPath(local) = old_name {
	if let Ok(rest) = local.strip_prefix(real_dir) {
	*old_name = rustc_span::RealFileName::Remapped {
	local_path: None,
	virtual_name: virtual_dir.join(rest),
	};
	}
	}
	}
	}
	}

The problem is, below in the same file, another piece of code tries to determine the local path of a remapped path if available:

rust/compiler/rustc_metadata/src/rmeta/decoder.rs

Lines 1479 to 1556 in 9a74608

	// Translate the virtual `/rustc/$hash` prefix back to a real directory
	// that should hold actual sources, where possible.
	//
	// NOTE: if you update this, you might need to also update bootstrap's code for generating
	// the `rust-src` component in `Src::run` in `src/bootstrap/dist.rs`.
	let virtual_rust_source_base_dir = option_env!("CFG_VIRTUAL_RUST_SOURCE_BASE_DIR")
	.map(Path::new)
	.filter(|_| {
	// Only spend time on further checks if we have what to translate *to*.
	sess.opts.real_rust_source_base_dir.is_some()
	})
	.filter(|virtual_dir| {
	// Don't translate away `/rustc/$hash` if we're still remapping to it,
	// since that means we're still building `std`/`rustc` that need it,
	// and we don't want the real path to leak into codegen/debuginfo.
	!sess.opts.remap_path_prefix.iter().any(|(_from, to)| to == virtual_dir)
	});
	let try_to_translate_virtual_to_real = |name: &mut rustc_span::FileName| {
	debug!(
	"try_to_translate_virtual_to_real(name={:?}): \
	virtual_rust_source_base_dir={:?}, real_rust_source_base_dir={:?}",
	name, virtual_rust_source_base_dir, sess.opts.real_rust_source_base_dir,
	);
	if let Some(virtual_dir) = virtual_rust_source_base_dir {
	if let Some(real_dir) = &sess.opts.real_rust_source_base_dir {
	if let rustc_span::FileName::Real(old_name) = name {
	if let rustc_span::RealFileName::Remapped { local_path: _, virtual_name } =
	old_name
	{
	if let Ok(rest) = virtual_name.strip_prefix(virtual_dir) {
	let virtual_name = virtual_name.clone();
	// The std library crates are in
	// `$sysroot/lib/rustlib/src/rust/library`, whereas other crates
	// may be in `$sysroot/lib/rustlib/src/rust/` directly. So we
	// detect crates from the std libs and handle them specially.
	const STD_LIBS: &[&str] = &[
	"core",
	"alloc",
	"std",
	"test",
	"term",
	"unwind",
	"proc_macro",
	"panic_abort",
	"panic_unwind",
	"profiler_builtins",
	"rtstartup",
	"rustc-std-workspace-core",
	"rustc-std-workspace-alloc",
	"rustc-std-workspace-std",
	"backtrace",
	];
	let is_std_lib = STD_LIBS.iter().any(|l| rest.starts_with(l));
	let new_path = if is_std_lib {
	real_dir.join("library").join(rest)
	} else {
	real_dir.join(rest)
	};
	debug!(
	"try_to_translate_virtual_to_real: `{}` -> `{}`",
	virtual_name.display(),
	new_path.display(),
	);
	let new_name = rustc_span::RealFileName::Remapped {
	local_path: Some(new_path),
	virtual_name,
	};
	*old_name = new_name;
	}
	}
	}
	}
	}
	};

That code is not correct, as it only looks at the prefix set by bootstrap when remap-debuginfo = true, not at the simulated prefix added with the -Z flag. This is the cause of the mismatch in behavior, and that code should be updated to also check for the simulated prefix (as the purpose of that -Z flag is to fully simulate remap-debuginfo = true).

This also explains the difference @japaric identified depending on whether the rust-src component was installed: when it's not installed the second piece of code is skipped, as there is no local path to determine, causing the consistent behavior.