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

tests: Port split-debuginfo to rmake.rs #135572

Merged
merged 9 commits into from
Feb 6, 2025

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jan 16, 2025

Part of #121876.

This PR supersedes #128754 and is co-authored with @Oneirical.

Known limitations

  • In general, like the Makefile version, this test in its present form is also somewhat funny because for the most part it merely checks for existence/absence of output artifacts but makes no attempt to actually check if the debuginfo is at all usable.

Changes

This PR ports tests/run-make/split-debuginfo to rmake.rs. This is an initial port, and certainly could be cleaned up and/or enhanced.

The original Makefile version had several functional problems. I fixed some of them, but also left some existing issues as-is.

  1. The linux/non-linux final branch had a conditional interpolation of UNSTABLE_OPTIONS := -Zunstable-options. However, one of the use sites was -C $(UNSTABLE_OPTIONS) split-debuginfo. This indicates to me that this run-make test is not run in CI under a non-linux + non-windows + non-darwin environment, because that would've failed as this would expand to -C -Zunstable-options split-debuginfo. I fixed this in the rmake.rs version, but I'm not sure if this distinction is worth keeping at all if it's not tested in CI.

  2. There are several comments that were discovered to be wrong. I tried to fix them in the rmake.rs version as well.

  3. The check for path remapping / lack of path remapping through

    objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (! grep $(TMPDIR)) || exit 1

    is incorrect, because that looks at the single line of that contains DW_AT_GNU_dwo_name. This is unfortunately wrong because empirical evidence shows that with objdump1, the check actually needs to look at the attribute value of DW_AT_comp_dir on the previous line not DW_AT_GNU_dwo_name2. Example output of objdump:

    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
    

    In the rmake.rs version I used a 2-line sliding window to check for DW_AT_comp_dir and DW_AT_GNU_dwo_name, but to look at DW_AT_comp_dir specifically.

  4. I included a bunch of FIXMEs and ENHANCEMENTs I noticed regarding the test because I didn't want to fix them in this initial port3.

  5. The Makefile version didn't test anything on Windows (both windows-msvc and windows-gnu). I added some very basic and very sparse checks for windows-msvc, but I am not willing to spend the effort to expand test coverage to windows-gnu in this initial port.

  6. This run-make test is way too big. But I didn't want to expend the effort of breaking this up in this initial port.

Review advice

  • I'm sorry for how long the rmake.rs test ended up, but a lot of it is comments and just vertical space due to formatting. If there's any ways to make this test less long / convoluted, advice would be appreciated.
  • This PR intentionally introduces several intermediate commits for the Makefile, mostly to illustrate the problems I discovered when looking at the original Makefile version. This is intended to highlight the existing problems in the Makefile version for the reviewer4.
    • There are several intentional non-functional commits:
      1. Reindent the Makefile to make the platform conditional gating more obvious.
      2. Collapse nested if-else branches into an else if construct, which is not supported by GNU Make 3.80.
      3. Remove all redundant -C debuginfo=2 when -g is already specified.
  • This PR is best reviewed commit-by-commit.

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various

Footnotes

  1. the output format differs between objdump and llvm-objdump, but the same is true for llvm-objdump that this is looking at the wrong line.

  2. AFAICT that is a GNU DWARF attribute extension, since it isn't mentioned in DWARFv5 spec

  3. For instance, the previous path remapping check could in theory be precisely inspected by inspecting .debug_info section to look for attribute value of DW_AT_comp_dir. But that involves resolving the value of the indirect string, which means you have to: (1) look for offset into string offset table and (2) use that offset to find the string itself in the string table. The split part of "split-debuginfo" makes this murky for me, so I wasn't able to replace llvm-objdump textual output substring matches with more precise object + gimli inspections.

  4. I intend to squash these intermediate commits away after the reviewer concludes that the current form of the rmake.rs test is acceptable for merge. Before then, I'll keep them to help with review.

@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-run-make Area: port run-make Makefiles to rmake.rs labels Jan 16, 2025
@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 16, 2025
@jieyouxu jieyouxu force-pushed the migrate-split-debuginfo branch 2 times, most recently from 78f39b7 to 11f2c9b Compare January 16, 2025 08:48
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not follow the unindented version. This is actually needed (as in no indents for the rules) because Makefiles are indent sensitive.

Comment on lines 34 to 37
# Windows only supports packed debuginfo - nothing to test.
off:
packed:
unpacked:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously, this won't work with GNU Make 3.80, which I suspect is what the CIs are probably using.

Comment on lines 115 to 116
$(RUSTC) $(UNSTABLEOPTS) -C split-debuginfo=packed -C debuginfo=2 \
-Z split-dwarf-kind=split --remap-path-prefix $(TMPDIR)=/a foo.rs -g
$(RUSTC) $(UNSTABLEOPTS) -C split-debuginfo=packed -g \
-Z split-dwarf-kind=split --remap-path-prefix $(TMPDIR)=/a foo.rs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-g is alias for -C debuginfo=2, specifying twice had me doing a double take

$(RUSTC) foo.rs -g -C $(UNSTABLEOPTS) split-debuginfo=off
$(RUSTC) foo.rs -g $(UNSTABLEOPTS) -C split-debuginfo=off
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was exercised in non-linux non-darwin non-windows env, this will fail because this expands to

$(RUSTC) foo.rs -g -C -Zunstable-options split-debuginfo=off

but it doesn't fail in CI, which means it's not even exercised in non-linux non-darwin non-windows under CI conditions.

# - `.o` and binary refer to remapped `.o` paths which do not exist
# - `.o` and binary refer to un-remapped `.o` because the remap path scope is for macros.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local testing reveals that the path is indeed not remapped, which is corroborated by the rule name substring "wrong-scope".

# - `.o` present (bitcode)
# - `.o` not present
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local testing reveals this comment is wrong, which is corroborated by the sentence immediately above it.

@@ -150,7 +150,7 @@ else # Some non-Windows, non-Darwin platforms are not stable, and some are.
rm $(TMPDIR)/$(call BIN,foo)

# - Debuginfo in `.o` files
# - `.o` and binary refer to remapped `.o` paths which do not exist
# - `.o` and binary refer to un-remapped `.o` because the remap path scope is for macros.
# - `.o` deleted
# - `.dwo` never created
# - `.dwp` present
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check below for the path is also wrong

objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (grep $(TMPDIR)) || exit 1

because

[03:00] Joe:rust (migrate-split-debuginfo *%) | objdump -Wi foo | grep DW_AT_GNU_dwo_name -B 5 -A 5
objdump: Warning: Unable to load dwo file: /home/joe/repos/rust/foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
   Abbrev Offset: 0x0
   Pointer Size:  8
 <0><b>: Abbrev Number: 1 (DW_TAG_compile_unit)
    <c>   DW_AT_stmt_list   : 0x0
    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
    <18>   DW_AT_GNU_dwo_id  : 0x17249573aa475dc7
    <20>   DW_AT_low_pc      : 0x0
    <28>   DW_AT_ranges      : 0x0
    <2c>   DW_AT_GNU_addr_base: 0x0
  Compilation Unit @ offset 0x30:

DW_AT_GNU_dwo_name's attribute value is the .dwo (or object) file name foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo, not the containing path /home/joe/repos/rust which is the attr value of DW_AT_comp_dir. So this check is not checking what it thinks it's checking.

Comment on lines +21 to +22
//! - The linux test coverage of cross-interactions between `-C split-debuginfo` and other flags are
//! significantly higher than the lack of such coverage for Windows and Darwin.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux targets are truly Tier 0.9 targets

Comment on lines +54 to +56
// FIXME(#135531): the `Makefile` version practically didn't test `-C split-debuginfo` on Windows
// at all, and lumped windows-msvc and windows-gnu together at that.
//@ ignore-windows-gnu
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't feel like trying to run this against windows-gnu, msys2 is very unpleasant to use IMO.

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2025
@jieyouxu

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
…<try>

tests: Port `split-debuginfo` to rmake.rs

Part of rust-lang#121876.

## Changes

This PR ports `tests/run-make/split-debuginfo` to rmake.rs. This is an **initial** port, and certainly could be cleaned up and/or enhanced.

The original Makefile version had several functional problems. I made

1. The linux/non-linux final branch had a conditional interpolation of `UNSTABLE_OPTIONS := -Zunstable-options`. However, one of the use sites was `-C $(UNSTABLE_OPTIONS) split-debuginfo`. This indicates to me that this run-make test is not run in CI under a non-linux + non-windows + non-darwin environment, because that would've failed as this would expand to `-C -Zunstable-options split-debuginfo`. I fixed this in the rmake.rs version, but I'm not sure if this distinction is worth keeping at all if it's not tested in CI.
2. There are several comments that were discovered to be wrong. I tried to fix them in the rmake.rs version as well.
3. The check for path remapping / lack of path remapping through

    ```make
    objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (! grep $(TMPDIR)) || exit 1
    ```

    are incorrect, because that looks at the single line of that contains `DW_AT_GNU_dwo_name`. This is unfortunately wrong because empirical evidence shows that with `objdump`[^objdump], the check actually needs to look at the attribute value of `DW_AT_comp_dir` on the previous line not `DW_AT_GNU_dwo_name`[^gnu-ext]. Example output of `objdump`:

	```text
    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
	```

	In the rmake.rs version
4. I included a bunch of FIXMEs and ENHANCEMENTs I noticed regarding the test because I didn't want to do them in this initial port[^enhancement].
5. The Makefile version didn't test *anything* on Windows (both windows-msvc and windows-gnu). I added some *very* basic and *very* sparse checks for windows-msvc, but I am not willing to spend the effort to expand test coverage to windows-gnu in this initial port.
6. This run-make test is way too big. But I didn't want to expend the effort of breaking this up in this initial port.

[^objdump]: the output format differs between `objdump` and `llvm-objdump`, but the same is true for `llvm-objdump` that this is looking at the wrong line.
[^gnu-ext]: AFAICT that is a GNU DWARF attribute extension, since it isn't mentioned in DWARFv5 spec
[^enhancement]: For instance, the previous path remapping check could in theory be precisely inspected by inspecting `.debug_info` section to look for attribute value of `DW_AT_comp_dir`. But that involves resolving the value of the indirect string, which means you have to: (1) look for offset into string offset table and (2) use *that* offset to find the string itself in the string table. The split part of "split-debuginfo" makes this murky for me, so I wasn't able to replace `llvm-objdump` textual output substring matches with more precise `object` + `gimli` inspections.

## Review advice

- I'm sorry for how long the rmake.rs test ended up, but a lot of it is comments and just vertical space due to formatting.
- This PR *intentionally* introduces several intermediate commits for the `Makefile`, mostly to illustrate the problems I discovered when looking at the original `Makefile` version. This is intended to highlight the existing problems in the `Makefile` version for the reviewer[^squash].
    - There are several intentional non-functional commits:
        1. Reindent the `Makefile` to make the platform conditional gating more obvious.
        2. Collapse nested if-else branches into an else if construct, which is not supported by GNU Make 3.80.
        3. Remove all redundant `-C debuginfo=2` when `-g` is already specified.
- This PR is best reviewed commit-by-commit.

[^squash]: I intend to squash these intermediate commits away after the reviewer concludes that the current form of the rmake.rs test is acceptable for merge. Before then, I'll keep them to help with review.

---

This PR supersedes rust-lang#128754 and is co-authored with `@Oneirical.`

r? `@ghost`

---

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
@bors

This comment was marked as outdated.

@jieyouxu jieyouxu added the rla-silenced Silences rust-log-analyzer postings to the PR it's added on. label Jan 16, 2025
@jieyouxu jieyouxu force-pushed the migrate-split-debuginfo branch from 11f2c9b to 9bf26e4 Compare January 16, 2025 09:01
@jieyouxu
Copy link
Member Author

Noticed I forgor input file on darwin branch.
@bors try

@bors
Copy link
Contributor

bors commented Jan 16, 2025

⌛ Trying commit 9bf26e4 with merge 2e99c8f...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
…<try>

tests: Port `split-debuginfo` to rmake.rs

Part of rust-lang#121876.

## Changes

This PR ports `tests/run-make/split-debuginfo` to rmake.rs. This is an **initial** port, and certainly could be cleaned up and/or enhanced.

The original Makefile version had several functional problems. I made

1. The linux/non-linux final branch had a conditional interpolation of `UNSTABLE_OPTIONS := -Zunstable-options`. However, one of the use sites was `-C $(UNSTABLE_OPTIONS) split-debuginfo`. This indicates to me that this run-make test is not run in CI under a non-linux + non-windows + non-darwin environment, because that would've failed as this would expand to `-C -Zunstable-options split-debuginfo`. I fixed this in the rmake.rs version, but I'm not sure if this distinction is worth keeping at all if it's not tested in CI.
2. There are several comments that were discovered to be wrong. I tried to fix them in the rmake.rs version as well.
3. The check for path remapping / lack of path remapping through

    ```make
    objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (! grep $(TMPDIR)) || exit 1
    ```

    are incorrect, because that looks at the single line of that contains `DW_AT_GNU_dwo_name`. This is unfortunately wrong because empirical evidence shows that with `objdump`[^objdump], the check actually needs to look at the attribute value of `DW_AT_comp_dir` on the previous line not `DW_AT_GNU_dwo_name`[^gnu-ext]. Example output of `objdump`:

	```text
    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
	```

	In the rmake.rs version
4. I included a bunch of FIXMEs and ENHANCEMENTs I noticed regarding the test because I didn't want to do them in this initial port[^enhancement].
5. The Makefile version didn't test *anything* on Windows (both windows-msvc and windows-gnu). I added some *very* basic and *very* sparse checks for windows-msvc, but I am not willing to spend the effort to expand test coverage to windows-gnu in this initial port.
6. This run-make test is way too big. But I didn't want to expend the effort of breaking this up in this initial port.

[^objdump]: the output format differs between `objdump` and `llvm-objdump`, but the same is true for `llvm-objdump` that this is looking at the wrong line.
[^gnu-ext]: AFAICT that is a GNU DWARF attribute extension, since it isn't mentioned in DWARFv5 spec
[^enhancement]: For instance, the previous path remapping check could in theory be precisely inspected by inspecting `.debug_info` section to look for attribute value of `DW_AT_comp_dir`. But that involves resolving the value of the indirect string, which means you have to: (1) look for offset into string offset table and (2) use *that* offset to find the string itself in the string table. The split part of "split-debuginfo" makes this murky for me, so I wasn't able to replace `llvm-objdump` textual output substring matches with more precise `object` + `gimli` inspections.

## Review advice

- I'm sorry for how long the rmake.rs test ended up, but a lot of it is comments and just vertical space due to formatting.
- This PR *intentionally* introduces several intermediate commits for the `Makefile`, mostly to illustrate the problems I discovered when looking at the original `Makefile` version. This is intended to highlight the existing problems in the `Makefile` version for the reviewer[^squash].
    - There are several intentional non-functional commits:
        1. Reindent the `Makefile` to make the platform conditional gating more obvious.
        2. Collapse nested if-else branches into an else if construct, which is not supported by GNU Make 3.80.
        3. Remove all redundant `-C debuginfo=2` when `-g` is already specified.
- This PR is best reviewed commit-by-commit.

[^squash]: I intend to squash these intermediate commits away after the reviewer concludes that the current form of the rmake.rs test is acceptable for merge. Before then, I'll keep them to help with review.

---

This PR supersedes rust-lang#128754 and is co-authored with `@Oneirical.`

r? `@ghost`

---

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
@rust-log-analyzer

This comment was marked as outdated.

@bors
Copy link
Contributor

bors commented Jan 16, 2025

☀️ Try build successful - checks-actions
Build commit: 2e99c8f (2e99c8f0321d1014942995e7592dfec012305983)

@jieyouxu jieyouxu marked this pull request as ready for review January 16, 2025 11:41
@jieyouxu jieyouxu removed the rla-silenced Silences rust-log-analyzer postings to the PR it's added on. label Jan 16, 2025
@jieyouxu
Copy link
Member Author

Based on git blame on the test, since you might vaguely recall some context about this specific test several years ago...

r? @davidtwco (or reroll)

@jieyouxu
Copy link
Member Author

The try job is successful so
@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 16, 2025
@bors

This comment was marked as outdated.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a huge improvement, it really goes to show just how subtle and fragile the original Makefiles are.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2025

📌 Commit 58c4b3c has been approved by davidtwco

It is now in the queue for this repository.

@bors 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 Feb 5, 2025
@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 5, 2025

Would you like me to squash the intermediate Makefile commits? Although it's probably fine (and might prove useful for git archaeology)

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#132547 (cg_gcc: Directly use rustc_abi instead of reexports)
 - rust-lang#135572 (tests: Port `split-debuginfo` to rmake.rs)
 - rust-lang#135964 (Make cenum_impl_drop_cast a hard error)
 - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe})
 - rust-lang#136304 (Reject negative literals for unsigned or char types in pattern ranges and literals)
 - rust-lang#136418 (uefi: process: Add support for command environment variables)
 - rust-lang#136449 (std: move network code into `sys`)
 - rust-lang#136517 (implement inherent str constructors)
 - rust-lang#136536 (Rename and Move some UI tests to more suitable subdirs)
 - rust-lang#136537 (Update `compiler-builtins` to 0.1.145)
 - rust-lang#136555 (Rename `slice::take...` methods to `split_off...`)
 - rust-lang#136567 (Arbitrary self types v2: recursion test)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#132547 (cg_gcc: Directly use rustc_abi instead of reexports)
 - rust-lang#135572 (tests: Port `split-debuginfo` to rmake.rs)
 - rust-lang#135964 (Make cenum_impl_drop_cast a hard error)
 - rust-lang#136154 (Use +secure-plt for powerpc-unknown-linux-gnu{,spe})
 - rust-lang#136304 (Reject negative literals for unsigned or char types in pattern ranges and literals)
 - rust-lang#136418 (uefi: process: Add support for command environment variables)
 - rust-lang#136449 (std: move network code into `sys`)
 - rust-lang#136517 (implement inherent str constructors)
 - rust-lang#136536 (Rename and Move some UI tests to more suitable subdirs)
 - rust-lang#136537 (Update `compiler-builtins` to 0.1.145)
 - rust-lang#136555 (Rename `slice::take...` methods to `split_off...`)
 - rust-lang#136567 (Arbitrary self types v2: recursion test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f75146f into rust-lang:master Feb 6, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 6, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2025
Rollup merge of rust-lang#135572 - jieyouxu:migrate-split-debuginfo, r=davidtwco

tests: Port `split-debuginfo` to rmake.rs

Part of rust-lang#121876.

This PR supersedes rust-lang#128754 and is co-authored with `@Oneirical.`

## Known limitations

- In general, like the `Makefile` version, this test in its present form is also somewhat funny because for the most part it merely checks for existence/absence of output artifacts but makes no attempt to actually check if the debuginfo is at all usable.

## Changes

This PR ports `tests/run-make/split-debuginfo` to rmake.rs. This is an **initial** port, and certainly could be cleaned up and/or enhanced.

The original Makefile version had several functional problems. I fixed some of them, but also left some existing issues as-is.

1. The linux/non-linux final branch had a conditional interpolation of `UNSTABLE_OPTIONS := -Zunstable-options`. However, one of the use sites was `-C $(UNSTABLE_OPTIONS) split-debuginfo`. This indicates to me that this run-make test is not run in CI under a non-linux + non-windows + non-darwin environment, because that would've failed as this would expand to `-C -Zunstable-options split-debuginfo`. I fixed this in the rmake.rs version, but I'm not sure if this distinction is worth keeping at all if it's not tested in CI.
2. There are several comments that were discovered to be wrong. I tried to fix them in the rmake.rs version as well.
3. The check for path remapping / lack of path remapping through

    ```make
    objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (! grep $(TMPDIR)) || exit 1
    ```

    is incorrect, because that looks at the single line of that contains `DW_AT_GNU_dwo_name`. This is unfortunately wrong because empirical evidence shows that with `objdump`[^objdump], the check actually needs to look at the attribute value of `DW_AT_comp_dir` on the previous line not `DW_AT_GNU_dwo_name`[^gnu-ext]. Example output of `objdump`:

	```text
    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
	```

	In the rmake.rs version I used a 2-line sliding window to check for `DW_AT_comp_dir` and `DW_AT_GNU_dwo_name`, but to look at `DW_AT_comp_dir` specifically.
4. I included a bunch of FIXMEs and ENHANCEMENTs I noticed regarding the test because I didn't want to fix them in this initial port[^enhancement].
5. The Makefile version didn't test *anything* on Windows (both windows-msvc and windows-gnu). I added some *very* basic and *very* sparse checks for windows-msvc, but I am not willing to spend the effort to expand test coverage to windows-gnu in this initial port.
6. This run-make test is way too big. But I didn't want to expend the effort of breaking this up in this initial port.

[^objdump]: the output format differs between `objdump` and `llvm-objdump`, but the same is true for `llvm-objdump` that this is looking at the wrong line.
[^gnu-ext]: AFAICT that is a GNU DWARF attribute extension, since it isn't mentioned in DWARFv5 spec
[^enhancement]: For instance, the previous path remapping check could in theory be precisely inspected by inspecting `.debug_info` section to look for attribute value of `DW_AT_comp_dir`. But that involves resolving the value of the indirect string, which means you have to: (1) look for offset into string offset table and (2) use *that* offset to find the string itself in the string table. The split part of "split-debuginfo" makes this murky for me, so I wasn't able to replace `llvm-objdump` textual output substring matches with more precise `object` + `gimli` inspections.

## Review advice

- I'm sorry for how long the rmake.rs test ended up, but a lot of it is comments and just vertical space due to formatting. If there's any ways to make this test less long / convoluted, advice would be appreciated.
- This PR *intentionally* introduces several intermediate commits for the `Makefile`, mostly to illustrate the problems I discovered when looking at the original `Makefile` version. This is intended to highlight the existing problems in the `Makefile` version for the reviewer[^squash].
    - There are several intentional non-functional commits:
        1. Reindent the `Makefile` to make the platform conditional gating more obvious.
        2. Collapse nested if-else branches into an else if construct, which is not supported by GNU Make 3.80.
        3. Remove all redundant `-C debuginfo=2` when `-g` is already specified.
- This PR is best reviewed commit-by-commit.

[^squash]: I intend to squash these intermediate commits away after the reviewer concludes that the current form of the rmake.rs test is acceptable for merge. Before then, I'll keep them to help with review.

---

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
@jieyouxu jieyouxu deleted the migrate-split-debuginfo branch February 6, 2025 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants