Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 79fb1e8

Browse files
committedJan 16, 2025
Auto merge of rust-lang#135572 - jieyouxu:migrate-split-debuginfo, r=<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
2 parents 5cd16b7 + 11f2c9b commit 79fb1e8

File tree

7 files changed

+1650
-376
lines changed

7 files changed

+1650
-376
lines changed
 

‎src/tools/run-make-support/src/external_deps/rustc.rs

+12
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,18 @@ impl Rustc {
325325
self
326326
}
327327

328+
/// Specify `-C debuginfo=...`.
329+
pub fn debuginfo(&mut self, level: &str) -> &mut Self {
330+
self.cmd.arg(format!("-Cdebuginfo={level}"));
331+
self
332+
}
333+
334+
/// Specify `-C split-debuginfo={packed,unpacked,off}`.
335+
pub fn split_debuginfo(&mut self, split_kind: &str) -> &mut Self {
336+
self.cmd.arg(format!("-Csplit-debuginfo={split_kind}"));
337+
self
338+
}
339+
328340
/// Pass the `--verbose` flag.
329341
pub fn verbose(&mut self) -> &mut Self {
330342
self.cmd.arg("--verbose");

‎src/tools/run-make-support/src/fs.rs

+1-3
Original file line numberDiff line numberDiff line change
@@ -238,9 +238,7 @@ pub fn set_permissions<P: AsRef<Path>>(path: P, perm: std::fs::Permissions) {
238238
));
239239
}
240240

241-
/// A function which prints all file names in the directory `dir` similarly to Unix's `ls`.
242-
/// Useful for debugging.
243-
/// Usage: `eprintln!("{:#?}", shallow_find_dir_entries(some_dir));`
241+
/// List directory entries immediately under the given `dir`.
244242
#[track_caller]
245243
pub fn shallow_find_dir_entries<P: AsRef<Path>>(dir: P) -> Vec<PathBuf> {
246244
let paths = read_dir(dir);

‎src/tools/run-make-support/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ pub use artifact_names::{
9090
/// Path-related helpers.
9191
pub use path_helpers::{
9292
cwd, filename_contains, filename_not_in_denylist, has_extension, has_prefix, has_suffix,
93-
not_contains, path, shallow_find_files, source_root,
93+
not_contains, path, shallow_find_directories, shallow_find_files, source_root,
9494
};
9595

9696
/// Helpers for scoped test execution where certain properties are attempted to be maintained.

‎src/tools/run-make-support/src/path_helpers.rs

+19
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,25 @@ pub fn shallow_find_files<P: AsRef<Path>, F: Fn(&PathBuf) -> bool>(
5353
matching_files
5454
}
5555

56+
/// Browse the directory `path` non-recursively and return all directories which respect the
57+
/// parameters outlined by `closure`.
58+
#[track_caller]
59+
pub fn shallow_find_directories<P: AsRef<Path>, F: Fn(&PathBuf) -> bool>(
60+
path: P,
61+
filter: F,
62+
) -> Vec<PathBuf> {
63+
let mut matching_files = Vec::new();
64+
for entry in rfs::read_dir(path) {
65+
let entry = entry.expect("failed to read directory entry.");
66+
let path = entry.path();
67+
68+
if path.is_dir() && filter(&path) {
69+
matching_files.push(path);
70+
}
71+
}
72+
matching_files
73+
}
74+
5675
/// Returns true if the filename at `path` does not contain `expected`.
5776
pub fn not_contains<P: AsRef<Path>>(path: P, expected: &str) -> bool {
5877
!path.as_ref().file_name().is_some_and(|name| name.to_str().unwrap().contains(expected))
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
run-make/cat-and-grep-sanity-check/Makefile
22
run-make/jobserver-error/Makefile
3-
run-make/split-debuginfo/Makefile
43
run-make/symbol-mangling-hashed/Makefile
54
run-make/translation/Makefile
There was a problem loading the remainder of the diff.

0 commit comments

Comments
 (0)
Failed to load comments.