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

Tracking Issue for trim-paths RFC 3127 #111540

Open
1 of 9 tasks
ehuss opened this issue May 13, 2023 · 26 comments
Open
1 of 9 tasks

Tracking Issue for trim-paths RFC 3127 #111540

ehuss opened this issue May 13, 2023 · 26 comments
Labels
A-backtrace Area: backtraces A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-reproducibility Area: Reproducible / Deterministic builds B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-trim-paths Feature: trim-paths T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented May 13, 2023

This is a tracking issue for the RFC 3127 (rust-lang/rfcs#3127).

This enhancement adds the --remap-path-scope command-line flag to control the scoping of how paths get remapped in the resulting binary.

Issues: F-trim-paths Feature: trim-paths
Documentation (rustc): https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/remap-path-scope.html
Documentation (cargo): https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#profile-trim-paths-option
Cargo tracking issue: rust-lang/cargo#12137

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@ehuss ehuss added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-trim-paths Feature: trim-paths labels May 13, 2023
@ehuss ehuss added the B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. label May 13, 2023
@jyn514 jyn514 added the A-reproducibility Area: Reproducible / Deterministic builds label Jun 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 3, 2023
…r-errors

Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 3, 2023
…r-errors

Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 7, 2023
…r-errors

Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 13, 2023
…r-errors

Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 19, 2023
…r-errors

Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
bors added a commit to rust-lang/miri that referenced this issue Oct 20, 2023
Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang/rust#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Oct 21, 2023
Implement rustc part of RFC 3127 trim-paths

This PR implements (or at least tries to) [RFC 3127 trim-paths](rust-lang/rust#111540), the rustc part. That is `-Zremap-path-scope` with all of it's components/scopes.

`@rustbot` label: +F-trim-paths
@weihanglo
Copy link
Member

I was trying to remove OSO symbols on macOS. Did a research and summarized in this issue: #116948 (comment). Now, I can see that there is no way for trim-paths to really trim all paths unless rustc does some dirty trick for older macOS platforms. Would it be a blocker if we want to stabilize this feature, say, in 2024 edition?

cc @Urgau you might be interested.

@michaelwoerister
Copy link
Member

What concerns are there regarding the use of relative paths causing issues with backtraces and debuggers?

I talked about this with @danielframpton the other day and he noticed that the current [package name]-[version] rule for remapping might make things unnecessarily complicated for debuggers. For making the debugging experience easy to set up, it would be beneficial to require as little configuration as possible. The RFC does not explicitly make file system paths (as used by Cargo) match with how things are renamed, so in the general case the debugger needs a separate source file search path for each crate involved.

However, if we prescribed that each crate from was remapped to the relative path Cargo uses inside its $CARGO_HOME/registry/src and $CARGO_HOME/git/checkouts directories, then we have a fixed number of source directories a debugger needs to know about. The renaming would then be [registry]/[package name]-[version] for registry crates and [package name]-[sha] for git dependencies.

The current [package name]-[version] rule is already pretty close, but that seems to be by accident. It would be good to make this something that can be relied on.

This might also help with backtraces?

I don't know if there is something similarly useful we can do for path dependencies.

Should we treat the current package the same as other packages?

This also came up. Unconditionally stripping the path to the current package might be too inflexible in some cases. E.g. when building a staticlib, one might want to make the paths coming from the current package identifiable in some way. If they all start with src/.. then they might clash with the paths of the codebase that uses the staticlib as a dependency.

@weihanglo
Copy link
Member

However, if we prescribed that each crate from was remapped to the relative path Cargo uses inside its $CARGO_HOME/registry/src and $CARGO_HOME/git/checkouts directories, then we have a fixed number of source directories a debugger needs to know about. The renaming would then be [registry]/[package name]-[version] for registry crates and [package name]-[sha] for git dependencies.

This plan sounds really tenable to me! Will do a change on cargo side for this, and see how people think about it.

For path dependencies, maybe we could have a common prefix for them, like cargo_deps@local/<pkg>-<version>. Might not be useful as others dep kinds, but at least people can have a single location for path dependencies?

then they might clash with the paths of the codebase that uses the staticlib as a dependency.

Sorry I don't fully understand. Could you give a more concrete example of this?

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 13, 2023
Implement RFC 3127 sysroot path handling changes

Fix rust-lang#105907
Fix rust-lang#85463

Implement parts of rust-lang#111540

Right now, backtraces into sysroot always shows /rustc/$hash in diagnostics, e.g.

```
thread 'main' panicked at 'hello world', map-panic.rs:2:50
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:616:12
   1: map_panic::main::{{closure}}
             at ./map-panic.rs:2:50
   2: core::option::Option<T>::map
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/option.rs:929:29
   3: map_panic::main
             at ./map-panic.rs:2:30
   4: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```

[RFC 3127 said](https://rust-lang.github.io/rfcs/3127-trim-paths.html#changing-handling-of-sysroot-path-in-rustc)

> We want to change this behaviour such that, when rust-src source files can be discovered, the virtual path is discarded and therefore the local path will be embedded, unless there is a --remap-path-prefix that causes this local path to be remapped in the usual way.

This PR implements this behaviour. When `rust-src` is present at compile time, rustc replaces /rustc/$hash with a real path into local rust-src with best effort. To sanitise this, users must explicitly supply `--remap-path-prefix=<path to rust-src>=foo`.
@michaelwoerister
Copy link
Member

For path dependencies, maybe we could have a common prefix for them, like cargo_deps@local/-. Might not be useful as others dep kinds, but at least people can have a single location for path dependencies?

Maybe, yes. It's definitely worth thinking about this some more (and collecting ideas from other people).

Sorry I don't fully understand. Could you give a more concrete example of this?

As I understand it, the RFC states that paths from the workspace being compiled are trimmed by simply making them relative to the workspace root, that is, all file paths from workspace crates would look like src/foo/bar.rs or some_crate_in_the_workspace/src/foo/bar.rs. Those paths don't contain anything that would allow disambiguating them from other paths that are purely relative. One example where this is sub-optimal is when one has multiple Rust projects, each of which gets compiled into a staticlib that is then consumed within another, non-Rust project. Each of these Rust projects would have debuginfo file paths that just start with src/ and the paths for some of the files might clash , e.g. src/lib.rs. The debugger would have no way to know which src/lib.rs file to pick, even if both are in its source search path.

Therefore it would be good, if there was some way to avoid this problem. Allowing to prepend a prefix to the relative paths might be an option.

@weihanglo
Copy link
Member

@michaelwoerister. Thank you for your feedback! I just opened a new issue rust-lang/cargo#13171 for further discussion about remap rules in Cargo.

@michaelwoerister
Copy link
Member

michaelwoerister commented Feb 9, 2024

For cargo install I wouldn't want trim paths. And ditto for precompiled opensource binaries as created using eg cargo-dist and downloaded using cargo-binstall.

To clarify: trim-paths will still give file paths of the form some_crate-1.0.1/src/lib.rs, so the information in a backtrace is still rather useful. If debuginfo is enabled (which currently is not the default for release builds), then the debugger should still be able to find the symbols, it just won't be able to find the source files currently.

EDIT: @bjorn3, would that be sufficient for your use case?

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 12, 2024
…=wesleywiser

link.exe: Don't embed full path to PDB file in binary.

This PR makes `rustc` unconditionally pass `/PDBALTPATH:%_PDB%` to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the [trim-paths RFC](rust-lang#111540) for `*-msvc` targets.

Passing `/PDBALTPATH:%_PDB%` to the linker is already done by many projects that need reproducible builds and [debugger's should still be able to find the PDB](https://learn.microsoft.com/cpp/build/reference/pdbpath) if it is in the same directory as the binary.

r? `@ghost`

Fixes rust-lang#87825
@michaelwoerister
Copy link
Member

Here is a table that shows the various combinations of the trim-paths and split-debuginfo Cargo options and (to the right side of the ) what rustc options these map to and what the consequences of this are.

trim-paths split-debuginfo --remap-path-scope Binary Separate Debuginfo Surprising? Useful? Remarks
none off   unsanitzed     Yes  
macro off macro semi-sanitized     ?  
object off macro + split-debuginfo + split-debuginfo-path + unsplit-debuginfo sanitized   Kind of ? Must sanitize debuginfo too, otherwise "object" won't be sanitized
all off macro + split-debuginfo + split-debuginfo-path + unsplit-debuginfo sanitized     Yes  
trim-paths split-debuginfo --remap-path-scope Binary Separate Debuginfo Surprising? Useful? Remarks
none packed   unsanitzed unsanitzed   Yes  
macro packed macro semi-sanitized unsanitzed   ?  
object packed macro + unsplit-debuginfo + split-debuginfo-path sanitized unsanitzed Yes ? Debugger won't find source files
all packed all sanitized sanitized   Yes (?) Debugger won't find source files
trim-paths split-debuginfo --remap-path-scope Binary Separate Debuginfo Surprising? Useful? Remarks
none unpacked   unsanitzed unsanitzed   Yes Fast builds with debuginfo
macro unpacked macro semi-sanitized unsanitzed   ?  
object unpacked macro + unsplit-debuginfo + split-debuginfo-path sanitized unsanitzed Yes ? Debugger won't find source files AND dwos
all unpacked all sanitized sanitized   ? Debugger won't find source files AND dwos
trim-paths split-debuginfo --remap-path-scope Binary Separate Debuginfo Surprising? Useful? Remarks
none post-link   unsanitzed unsanitzed   Kind of Not much use in splitting
macro post-link macro sanitized unsanitzed A little Kind of  
object post-link macro + unsplit-debuginfo + split-debuginfo-path + split-debuginfo sanitized unsanitzed   Kind of Might be useful when only distributing binary?
all post-link all sanitized sanitized   Yes  

Observations:

  • Some of the combinations don't have a real use case
  • Some of the combinations are surprising. They don't do what one this they would do. E.g. trim-paths=object, split-debuginfo=packed leads to semi-broken debuginfo because line-tables, contained in the binary, have to be sanitized. But it the combination sounds like "don't touch debuginfo".
  • Only two cases make a distinction between the split-debuginfo and unsplit-debuginfo remapping scopes -- and they fall into the surprising and useless category.
  • No case makes a distinction between unsplit-debuginfo and split-debuginfo-path

My conclusion:

  • We should only have a single debuginfo scope and not further subdivide that into split-debuginfo, unsplit-debuginfo, and split-debuginfo-path. The only cases this makes a difference for are trim-paths=object, split-debuginfo=packed and trim-paths=object, split-debuginfo=unpacked but these cases have no real use case to begin with.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 13, 2024
…=wesleywiser

link.exe: Don't embed full path to PDB file in binary.

This PR makes `rustc` unconditionally pass `/PDBALTPATH:%_PDB%` to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the [trim-paths RFC](rust-lang#111540) for `*-msvc` targets.

Passing `/PDBALTPATH:%_PDB%` to the linker is already done by many projects that need reproducible builds and [debugger's should still be able to find the PDB](https://learn.microsoft.com/cpp/build/reference/pdbpath) if it is in the same directory as the binary.

r? `@ghost`

Fixes rust-lang#87825
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 15, 2024
…=wesleywiser

link.exe: Don't embed full path to PDB file in binary.

This PR makes `rustc` unconditionally pass `/PDBALTPATH:%_PDB%` to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the [trim-paths RFC](rust-lang#111540) for `*-msvc` targets.

Passing `/PDBALTPATH:%_PDB%` to the linker is already done by many projects that need reproducible builds and [debugger's should still be able to find the PDB](https://learn.microsoft.com/cpp/build/reference/pdbpath) if it is in the same directory as the binary.

r? `@ghost`

Fixes rust-lang#87825
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 17, 2024
link.exe: Don't embed full path to PDB file in binary.

This PR makes `rustc` unconditionally pass `/PDBALTPATH:%_PDB%` to MSVC-style linkers, causing the linker to only embed the filename of the PDB in the binary instead of the full path. This will help implement the [trim-paths RFC](rust-lang/rust#111540) for `*-msvc` targets.

Passing `/PDBALTPATH:%_PDB%` to the linker is already done by many projects that need reproducible builds and [debugger's should still be able to find the PDB](https://learn.microsoft.com/cpp/build/reference/pdbpath) if it is in the same directory as the binary.

r? `@ghost`

Fixes rust-lang/rust#87825
@Zalathar
Copy link
Contributor

Via #122450, it’s come to my attention that this will affect coverage instrumentation, because the coverage mappings embedded in the binary usually contain at least one absolute path, and any adjustment to those paths is constrained by the capabilities of the llvm-cov tool.

@michaelwoerister
Copy link
Member

For reference, Clang allows to separately control path remapping for code coverage: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fcoverage-prefix-map

@michaelwoerister
Copy link
Member

FYI, @Urgau's PR #122450 will merge all debuginfo scopes into a single one, since splitting them did not actually solve the problem it was intended to solve (see #111540 (comment)).

I'll wait for a few days before approving the PR, as to give everyone here a chance to speak up in case they disagree with the change.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 29, 2024
…=michaelwoerister

Simplify trim-paths feature by merging all debuginfo options together

This PR simplifies the trim-paths feature by merging all debuginfo options together, as described in rust-lang#111540 (comment).

And also do some correctness fixes found during the review.

cc `@weihanglo`
r? `@michaelwoerister`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 30, 2024
…oerister

Simplify trim-paths feature by merging all debuginfo options together

This PR simplifies the trim-paths feature by merging all debuginfo options together, as described in rust-lang/rust#111540 (comment).

And also do some correctness fixes found during the review.

cc `@weihanglo`
r? `@michaelwoerister`
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Mar 30, 2024
…oerister

Simplify trim-paths feature by merging all debuginfo options together

This PR simplifies the trim-paths feature by merging all debuginfo options together, as described in rust-lang/rust#111540 (comment).

And also do some correctness fixes found during the review.

cc `@weihanglo`
r? `@michaelwoerister`
@redactedontop
Copy link

...updates?

@liciana24
Copy link

any movement here? still waiting on this and hoping scope didnt change from stripping all in release mode BY DEFAULT.

@weihanglo
Copy link
Member

Not entirely sure on rustc side of this, but I feel like it is waiting for Cargo to report back the integration status. See also: rust-lang/cargo#12137 (comment)

@estebank
Copy link
Contributor

estebank commented Aug 9, 2024

Random question: are these flags meant to affect the output of diagnostics as well? One could argue that they should.

@Urgau
Copy link
Member

Urgau commented Aug 9, 2024

Yes, that's the diagnostics scope, see the --remap-path-scope flag for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backtrace Area: backtraces A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-reproducibility Area: Reproducible / Deterministic builds B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-trim-paths Feature: trim-paths T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests