Migrate split-debuginfo run-make test to rmake
#128754
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-run-make
|
This PR modifies cc @jieyouxu The run-make-support library was changed cc @jieyouxu |
rustbot
added
S-blocked
Oneirical
force-pushed
the
master
branch
2 times, most recently
from
6c4d8bf to
07cef6f
Compare
|
☔ The latest upstream changes (presumably #128783) made this pull request unmergeable. Please resolve the merge conflicts. |
jieyouxu
reviewed
Aug 8, 2024
jieyouxu
added
S-waiting-on-review
|
I won't have time to review this before our meeting tomorrow, I'll try to get to it tomorrow. |
Rust maintainers? Sleeping? What is this? No rush, I am quite sure that there will not be any further test ports dependent on another one, so we can deal with each individual remaining case on its own. |
|
☔ The latest upstream changes (presumably #129143) made this pull request unmergeable. Please resolve the merge conflicts. |
jieyouxu
reviewed
Sep 16, 2024
Comment on lines
+405
to
+570
| // Some non-Windows, non-Darwin platforms are not stable, and some are. | ||
| fn unstable_rustc() -> Rustc { | ||
| let mut compiler = rustc(); | ||
| if !target().contains("linux") { | ||
| compiler.arg("-Zunstable-options"); | ||
| } |
There was a problem hiding this comment.
Question: the impl here is for non-linux, it doesn't quite match up with the comment?
There was a problem hiding this comment.
The comment is from the original Makefile:
# Some non-Windows, non-Darwin platforms are not stable, and some are.
ifeq ($(UNAME),Linux)
UNSTABLEOPTS :=
else
UNSTABLEOPTS := -Zunstable-options
endif
I am open to inverting the comment to read "Some non-Linux platforms are not stable, and some are."
jieyouxu
added
S-waiting-on-author
Oneirical
force-pushed
the
master
branch
2 times, most recently
from
7ae48a8 to
b37d0bf
Compare
|
@rustbot review |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #134844) made this pull request unmergeable. Please resolve the merge conflicts. |
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
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
|
Superseded by #135572 w/ attribution, thanks for the PR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #121876 and the associated Google Summer of Code project.
Possibly the most epic test rewrite yet in this project.
.dwo never created). Tell me if anything should get explanatory comments, especially in the massiverun_testfunction.Please try:
try-job: test-various
try-job: aarch64-apple