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

Binaries end up containing path to the rust-src component despite --remap-path-prefix #73167

Closed
nagisa opened this issue Jun 9, 2020 · 3 comments · Fixed by #83813
Closed
Labels
A-reproducibility Area: Reproducible / deterministic builds C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Jun 9, 2020

When the following code is built with rustc with rust-src component available to it, the binary will end up containing the reference to the libstd source in the rustc installation path. This reference is then possibly printed out if this program panics if an error ever occurs.

fn main() {
    std::thread::spawn(|| {
        println!("hello");
    });
}

Compile this program as such:

$ rustc src/main.rs

and inspect the executable with strings:

$ strings ./main | grep lib/rustlib
failed to spawn thread/nix/store/blyf5kcrmi9kciffk18qm9r6xr9k4c6v-rust-1.45.0-nightly-2020-06-04-47c3158c3/lib/rustlib/src/rust/src/libstd/thread/mod.rs
there is no such thing as a relaxed fence/nix/store/blyf5kcrmi9kciffk18qm9r6xr9k4c6v-rust-1.45.0-nightly-2020-06-04-47c3158c3/lib/rustlib/src/rust/src/libcore/macros/mod.rs

Then try to get rid of the source paths with the flag dedicated for exactly this purpose:

$ rustc src/main.rs --remap-path-prefix=/nix/store/blyf5kcrmi9kciffk18qm9r6xr9k4c6v-rust-1.45.0-nightly-2020-06-04-47c3158c3/lib/rustlib/src/=/rustc-src/

and notice that the binary contains the path to the original directory anyway:

$ strings ./main | grep lib/rustlib
failed to spawn thread/nix/store/blyf5kcrmi9kciffk18qm9r6xr9k4c6v-rust-1.45.0-nightly-2020-06-04-47c3158c3/lib/rustlib/src/rust/src/libstd/thread/mod.rs
there is no such thing as a relaxed fence/nix/store/blyf5kcrmi9kciffk18qm9r6xr9k4c6v-rust-1.45.0-nightly-2020-06-04-47c3158c3/lib/rustlib/src/rust/src/libcore/macros/mod.rs

The relevant logic appears to be here:

https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/rmeta/decoder.rs#L1450

However it is not clear to me that this is the right place to introduce more logic to handle --remap-path-prefix.

Version

nightly 2020-06-05

cc @eddyb who appears to have added this logic in #70642

@nagisa nagisa added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jun 9, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
@nagisa
Copy link
Member Author

nagisa commented Jun 9, 2020

This is technically a regression, but a unimportant enough one that it should not block a release.

@spastorino
Copy link
Member

Assigning P-medium as discussed as part of the Prioritization Working Group process and removing I-prioritize.

@spastorino spastorino added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 10, 2020
@nagisa nagisa added A-reproducibility Area: Reproducible / deterministic builds regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jun 11, 2020
@eddyb
Copy link
Member

eddyb commented Jun 11, 2020

I don't think --remap-path-prefix should remap paths that were already remapped, that feels like a bigger change that is necessary to fix the regression here.

Probably a good way to handle the /rustc/.../ "unmapping"/"devirtualization" is to only expose it to diagnostics, and leave file!() / panic::Location / debuginfo alone (i.e. use /rustc/.../ for it).

Since #72767 we track both paths, so the fix should be relatively simple: use RealFileName's stable_name method (could also rename it to stable_path) instead of local_path in most cases.

However, it looks like everything we want to change and also diagnostics, all go through formatting (i.e. impl std::fmt::Display for FileName) instead of calling any of those methods themselves.

So maybe the fmt::Display impl should have the old behavior, and then we change diagnostics specifically. What's funny is that for the purposes of extracting snippets, local_path is used directly, so we could have source snippets alongside /rustc/... paths, in diagnostics (but I don't think we want that, at least in terms of tooling consuming diagnostics in JSON form).

@bors bors closed this as completed in e1ff91f May 12, 2021
flip1995 pushed a commit to flip1995/rust that referenced this issue May 20, 2021
Fix `--remap-path-prefix` not correctly remapping `rust-src` component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#73167 ("Binaries end up containing path to the rust-src component despite `--remap-path-prefix`") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.

`RealFileName::Named` introduced in rust-lang#72767 is now renamed as `LocalPath`, because this variant wraps a (most likely) valid local filesystem path.

`RealFileName::Devirtualized` is renamed as `Remapped` to be used for remapped path from a real path via `--remap-path-prefix` argument, as well as real path inferred from a virtualized (during compiler bootstrapping) `/rustc/...` path. The `local_path` field is now an `Option<PathBuf>`, as it will be set to `None` before serialisation, so it never reaches any build output. Attempting to serialise a non-`None` `local_path` will cause an assertion faliure.

When a path is remapped, a `RealFileName::Remapped` variant is created. The original path is preserved in `local_path` field and the remapped path is saved in `virtual_name` field. Previously, the `local_path` is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".

`rustc_span::SourceFile`'s fields `unmapped_path` (introduced by rust-lang#44940) and `name_was_remapped` (introduced by rust-lang#41508 when `--remap-path-prefix` feature originally added) are removed, as these two pieces of information can be inferred from the `name` field: if it's anything other than a `FileName::Real(_)`, or if it is a `FileName::Real(RealFileName::LocalPath(_))`, then clearly `name_was_remapped` would've been false and `unmapped_path` would've been `None`. If it is a `FileName::Real(RealFileName::Remapped{local_path, virtual_name})`, then `name_was_remapped` would've been true and `unmapped_path` would've been `Some(local_path)`.

cc `@eddyb` who implemented `/rustc/...` path devirtualisation
bjorn3 pushed a commit to bjorn3/rust that referenced this issue May 27, 2021
Fix `--remap-path-prefix` not correctly remapping `rust-src` component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#73167 ("Binaries end up containing path to the rust-src component despite `--remap-path-prefix`") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.

`RealFileName::Named` introduced in rust-lang#72767 is now renamed as `LocalPath`, because this variant wraps a (most likely) valid local filesystem path.

`RealFileName::Devirtualized` is renamed as `Remapped` to be used for remapped path from a real path via `--remap-path-prefix` argument, as well as real path inferred from a virtualized (during compiler bootstrapping) `/rustc/...` path. The `local_path` field is now an `Option<PathBuf>`, as it will be set to `None` before serialisation, so it never reaches any build output. Attempting to serialise a non-`None` `local_path` will cause an assertion faliure.

When a path is remapped, a `RealFileName::Remapped` variant is created. The original path is preserved in `local_path` field and the remapped path is saved in `virtual_name` field. Previously, the `local_path` is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".

`rustc_span::SourceFile`'s fields `unmapped_path` (introduced by rust-lang#44940) and `name_was_remapped` (introduced by rust-lang#41508 when `--remap-path-prefix` feature originally added) are removed, as these two pieces of information can be inferred from the `name` field: if it's anything other than a `FileName::Real(_)`, or if it is a `FileName::Real(RealFileName::LocalPath(_))`, then clearly `name_was_remapped` would've been false and `unmapped_path` would've been `None`. If it is a `FileName::Real(RealFileName::Remapped{local_path, virtual_name})`, then `name_was_remapped` would've been true and `unmapped_path` would've been `Some(local_path)`.

cc `@eddyb` who implemented `/rustc/...` path devirtualisation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-reproducibility Area: Reproducible / deterministic builds C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
4 participants