Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
japaric opened this issue Jun 3, 2022 · 5 comments · Fixed by #97786
Closed

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

japaric opened this issue Jun 3, 2022 · 5 comments · Fixed by #97786
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@japaric
Copy link
Member

japaric commented Jun 3, 2022

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
  1. 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
Copy link
Member

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

@JohnTitor JohnTitor added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jun 5, 2022
@JohnTitor JohnTitor removed the A-testsuite Area: The testsuite used to check the correctness of rustc label Jun 5, 2022
@JohnTitor JohnTitor changed the title ./x.py test src/test/ui now fails with remap-debuginfo = true -Z simulate-remapped-rust-src-base is not working as expected with remap-debuginfo = true Jun 5, 2022
@JohnTitor
Copy link
Member

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

@pietroalbini
Copy link
Member

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
Copy link
Member

pietroalbini commented Jun 6, 2022

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.

// 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:

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

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 6, 2022
These flags make UI test fail on some env.
See rust-lang#97682 for more details.
@pietroalbini pietroalbini changed the title -Z simulate-remapped-rust-src-base is not working as expected with remap-debuginfo = true -Z simulate-remapped-rust-src-base is not fully simulating remap-debuginfo = true Jun 6, 2022
@pietroalbini
Copy link
Member

Opened #97789 to fix the test, and #97786 to fix the behavior of -Z simulate-remapped-rust-src-base to be consistent.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 26, 2022
…fix, r=Mark-Simulacrum

Account for `-Z simulate-remapped-rust-src-base` when resolving remapped paths

Discovered in rust-lang#97682, `-Z simulate-remapped-rust-src-base` only partially simulated the behavior of `remap-debuginfo = true`. While the flag successfully simulates the remapping when stdlib's `rmeta` file is loaded, the simulated prefix was not accounted for when the remapped path's local path was being discovered. This caused the flag to not fully simulate the behavior of `remap-debuginfo = true`, leading to inconsistent behaviors.

This PR fixes rust-lang#97682 by also accounting for the simulated path.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 28, 2022
…fix, r=Mark-Simulacrum

Account for `-Z simulate-remapped-rust-src-base` when resolving remapped paths

Discovered in rust-lang#97682, `-Z simulate-remapped-rust-src-base` only partially simulated the behavior of `remap-debuginfo = true`. While the flag successfully simulates the remapping when stdlib's `rmeta` file is loaded, the simulated prefix was not accounted for when the remapped path's local path was being discovered. This caused the flag to not fully simulate the behavior of `remap-debuginfo = true`, leading to inconsistent behaviors.

This PR fixes rust-lang#97682 by also accounting for the simulated path.
@bors bors closed this as completed in c23add7 Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants