fix: explicitly remap current dir by using `.` by weihanglo · Pull Request #13114 · rust-lang/cargo

weihanglo · 2023-12-05T07:52:20Z

What does this PR try to resolve?

In https://github.com/rust-lang/rust/blob/87e1447aa/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs#L856
when the remap result of work_dir is an empty string,
LLVM won't generate some symbols for root debuginfo node.
For example, N_SO and N_OSO on macOS,
or DW_AT_comp_dir and DW_AT_GNU_dwo_name when debuginfo is splitted.
Precisely, it is observed that when the DIFile of a root DI node of a
compile unit DIFile was provied with an empty Directory string,
LLVM would not emit those symbols for the root DI node.

This behavior is not desired,
resulting in corrupted debuginfo and degrading debugging experience.

This is might not be a bug of --remap-path-prefix in rustc,
since -fdebug-prefix-map in clang 16 could have the same result
(DW_AT_comp_dir is gone when work_dir is remapped to an empty string).
However, in gcc 12 fdebug-prefix-map will return an absolute work_dir
when an empty string occurs.

To not bother whether this needs to be fixed in rustc or not,
let's fix it by always appending an explicit .
when --remap-path-prefix remaps to relative workspace root
a.k.a. where rustc is invoking.

How should we test and review this PR?

Build rustc

Without a possible fix in rustc like rust-lang/rust#118518, the debuginfo is not corrputed even we dont remap to . explicitly. Therefore, to test it manually you need to build rustc from rust-lang/rust#118518. Generally,

# in rust-lang/rust
./x b library --stage 1
rustup toolchain link stage1 build/host/stage1

Then cargo +stage1 build is available, and you can build package via

CARGO_PROFILE_DEV_TRIM_PATHS=object cargo +stage1 -Ztrim-paths b

After build a package, I would recommend playing with debuggers, either gdb or lldb.
After remapping, supposedly you need to launch the debugger from the same location the build kicked off, usually the workspace root.

Please help verify that source files still show, and breakpoints can pause/resume at either local or registry packages.

I don't want to build rustc

You can use either objdump -Wi or readelf -wi or dsymutil -s to inspect debug symbols, though it doesn't help much.

Additional information

It would be awesome if somebody can help verify the behavior on Windows 😆.

rustbot · 2023-12-05T07:52:26Z

r? @ehuss

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

bors · 2023-12-06T03:05:31Z

☔ The latest upstream changes (presumably #13118) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2023-12-06T04:04:23Z

src/cargo/core/compiler/mod.rs

        if is_local && pkg_root.strip_prefix(ws_root).is_ok() {
            remap.push(ws_root);
-            remap.push("="); // empty to remap to relative paths.
+            remap.push("=."); // explicitly remap to cwd (rustc working dir).


Actually I am not sure if we want to fix this in cargo or in rustc…

Run clang main.c -gdwarf -fdebug-prefix-map="$PWD"= -gsplit-dwarf

on Linux DW_AT_comp_dir is gone, as same as the result from rustc.

on macOS N_SO and N_OSO are gone, as same as the result from rustc.

Run gcc main.c -gdwarf -fdebug-prefix-map="$PWD"= -gsplit-dwarf

on Linux it generates empty DW_AT_comp_dir.

on macOS it generates N_SO with value /.

Without bothering whether this should be fixed in rustc or cargo, I would propose we add an . for remapping the current working directory.

For more on gcc/clang remap options, see: https://reproducible-builds.org/docs/build-path/

weihanglo · 2023-12-08T20:00:28Z

This is ready for review, but somehow a bit convoluted.

tests/testsuite/profile_trim_paths.rs

Also demonstarte that on Linux with split-debuginfo on the remap is broken

In https://github.com/rust-lang/rust/blob/87e1447aa/compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs#L856 when the remap result of `work_dir` is an empty string, LLVM won't generate some symbols for root debuginfo node. For example, `N_SO` and `N_OSO` on macOS, or `DW_AT_comp_dir` on Linux when debuginfo is splitted. Precisely, it is observed that when the `DIFile` of compile unit was provied with an empty compilation `Directory` string, LLVM would not emit those symbols for the root DI node. This behavior is not desired, resulting in corrupted debuginfo and degrading debugging experience. This is might not be a bug of `--remap-path-prefix` in rustc, since `-fdebug-prefix-map` in clang 16 could have the same result (`DW_AT_comp_dir` is gone when `work_dir` is remapped to an empty string). However, in gcc 12 `fdebug-prefix-map` will return an absolute work_dir when an empty string occurs. To not bother whether this needs to be fixed in rustc or not, let's fix it by always appending an explicit `.` when `--remap-path-prefix` remaps to relative workspace root a.k.a. where rustc is invoking. For more on gcc/clang remap options, see https://reproducible-builds.org/docs/build-path/

For refeference: https://sourceware.org/binutils/docs/binutils/readelf.html

epage · 2023-12-08T22:37:29Z

@bors r+

bors · 2023-12-08T22:37:31Z

📌 Commit fcd4221 has been approved by epage

It is now in the queue for this repository.

bors · 2023-12-08T22:38:39Z

⌛ Testing commit fcd4221 with merge 6feabf9...

bors · 2023-12-08T23:23:21Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 6feabf9 to master...

Update cargo 12 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..6feabf94773286928b7d73d0d313c0c5562b66af 2023-12-06 02:29:23 +0000 to 2023-12-08 22:38:37 +0000 - fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114) - Don't rely on mtime to test changes (rust-lang/cargo#13143) - refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128) - fix: Print rustc messages colored on wincon (rust-lang/cargo#13140) - Add a windows manifest file (rust-lang/cargo#13131) - Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132) - re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130) - fix bash completion in directory with spaces (rust-lang/cargo#13126) - test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129) - fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125) - re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117) - refactor: Clarify PackageId constructor names (rust-lang/cargo#13123) r? ghost

Update cargo 13 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..66ad359b408ccdf867cceb30cc2dff1ed4afd82d 2023-12-06 02:29:23 +0000 to 2023-12-09 12:30:01 +0000 - chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144) - fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114) - Don't rely on mtime to test changes (rust-lang/cargo#13143) - refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128) - fix: Print rustc messages colored on wincon (rust-lang/cargo#13140) - Add a windows manifest file (rust-lang/cargo#13131) - Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132) - re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130) - fix bash completion in directory with spaces (rust-lang/cargo#13126) - test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129) - fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125) - re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117) - refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)

Update cargo 20 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..1aa9df1a5be205cce621f0bc0ea6062a5e22a98c 2023-12-06 02:29:23 +0000 to 2023-12-12 14:52:31 +0000 - crates-io: Add support for other 2xx HTTP status codes (rust-lang/cargo#13158) - Remove the deleted feature test_2018_feature from the test (rust-lang/cargo#13156) - refactor(schema): Remove reliance on cargo types (rust-lang/cargo#13154) - fix(toml)!: Disallow `[lints]` in virtual workspaces (rust-lang/cargo#13155) - Limit exported-private-dependencies lints to libraries (rust-lang/cargo#13135) - chore: update to gix-index@0.27.1 (rust-lang/cargo#13148) - Update curl-sys to bring in curl 8.5.0 (rust-lang/cargo#13147) - chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144) - fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114) - Don't rely on mtime to test changes (rust-lang/cargo#13143) - refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128) - fix: Print rustc messages colored on wincon (rust-lang/cargo#13140) - Add a windows manifest file (rust-lang/cargo#13131) - Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132) - re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130) - fix bash completion in directory with spaces (rust-lang/cargo#13126) - test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129) - fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125) - re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117) - refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)

rustbot assigned ehuss Dec 5, 2023

rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2023

weihanglo mentioned this pull request Dec 5, 2023

test(trim-paths): assert OSO and SO cannot be trimmed #13118

Merged

weihanglo force-pushed the trim-paths-explicit-cwd branch from a714f44 to 9b6cc88 Compare December 6, 2023 04:03

weihanglo commented Dec 6, 2023

View reviewed changes

weihanglo force-pushed the trim-paths-explicit-cwd branch 3 times, most recently from dd6b7c7 to 39c6aa5 Compare December 8, 2023 18:54

test(trim-paths): use --config CLI to change options

985d49b

weihanglo force-pushed the trim-paths-explicit-cwd branch 2 times, most recently from 9c7a261 to c424d8f Compare December 8, 2023 19:18

weihanglo marked this pull request as ready for review December 8, 2023 19:59

weihanglo added the Z-trim-paths Nightly: path sanitization label Dec 8, 2023

epage reviewed Dec 8, 2023

View reviewed changes

tests/testsuite/profile_trim_paths.rs Show resolved Hide resolved

epage reviewed Dec 8, 2023

View reviewed changes

tests/testsuite/profile_trim_paths.rs Show resolved Hide resolved

epage reviewed Dec 8, 2023

View reviewed changes

tests/testsuite/profile_trim_paths.rs Show resolved Hide resolved

weihanglo force-pushed the trim-paths-explicit-cwd branch from c424d8f to de33737 Compare December 8, 2023 20:50

weihanglo added 2 commits December 8, 2023 15:51

test(trim-paths): add test for each split-debuginfo options

7cb7b47

Also demonstarte that on Linux with split-debuginfo on the remap is broken

weihanglo force-pushed the trim-paths-explicit-cwd branch from de33737 to bb86adf Compare December 8, 2023 20:52

test(trim-paths): don't follow links to separate debuginfo files

fcd4221

For refeference: https://sourceware.org/binutils/docs/binutils/readelf.html

weihanglo force-pushed the trim-paths-explicit-cwd branch from 515f3cd to fcd4221 Compare December 8, 2023 21:34

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 Dec 8, 2023

bors merged commit 6feabf9 into rust-lang:master Dec 8, 2023

weihanglo deleted the trim-paths-explicit-cwd branch December 8, 2023 23:41

weihanglo mentioned this pull request Dec 9, 2023

Update cargo rust-lang/rust#118765

Merged

ehuss added this to the 1.76.0 milestone Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: explicitly remap current dir by using `.`#13114

fix: explicitly remap current dir by using `.`#13114
bors merged 4 commits intorust-lang:masterfrom
weihanglo:trim-paths-explicit-cwd

weihanglo commented Dec 5, 2023 •

edited

Loading

Uh oh!

rustbot commented Dec 5, 2023

Uh oh!

bors commented Dec 6, 2023

Uh oh!

weihanglo Dec 6, 2023

Uh oh!

weihanglo Dec 8, 2023

Uh oh!

weihanglo commented Dec 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

epage commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

weihanglo commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Build rustc

I don't want to build rustc

Additional information

Uh oh!

rustbot commented Dec 5, 2023

Uh oh!

bors commented Dec 6, 2023

Uh oh!

weihanglo Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

weihanglo Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

weihanglo commented Dec 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

epage commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

bors commented Dec 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

weihanglo commented Dec 5, 2023 •

edited

Loading