Add a mechanism to GC when building .dwps.#50
Conversation
|
Maybe split out the added tests into a new commit and put the gimli + hashbrown updates as first commit? Trying to see if you can split out the GC implementation into a couple of commits (eg first adapt the writer impls, but ask it to preserve all items and in a later commit add the actual GC algorithm) may also be useful. I doubt anyone would be able to accurately review a single ~3000 line commit (excl tests). Also just to check: Does the test harness automatically pickup on those added test files? |
|
Yes, the tests are picked up automatically, that's why CI is failing (the tests use DW_FORM_addrx3 and the llvm-dwarfdump used on CI is apparently too old to recognize it). I don't think splitting out the tests or the dependency changes into one or two separate commits helps to review this. Those are easy enough to distinguish simply by what file they're in. If I were to split this into smaller commits I would split out the rewriting of the supporting sections (.debug_loclists.dwo, .debug_rnglists.dwo, and .debug_str_offsets.dwo) first and then go from there. |
khuey
force-pushed
the
main
branch
2 times, most recently
from
0591da5 to
89477ca
Compare
|
I made a few minor changes to clean up error handling and use data structures more efficiently but I'm done now. I assume @davidtwco would be the one to review this. Happy to discuss splitting up the patch with him if it's too big to review. |
…e DIEs in the tree, and if there is no longer an eligible sibling, to point it at the correct DW_TAG_null.
…oned. gdb and lldb both treat an empty rangelist and a rangelist where all the entries are tombstoned identically.
|
I had Fable 5 review this and it raised the following issues that need to be fixed and/or are worth considering:
|
Using a generic doesn't buy us anything here because the rest of the API can't be Send/Sync anyways.
referenced_rnglists/referenced_loclists can still be something. Avoid pruning, which would catch any orphaned list entries and shift indices incorrectly. Apply the same rewritten.as_ref().and(...) guard already used for str_offsets to the rnglists, loclists, and debug_loc paths.
…f --gc is used without executables
Split it into functions, an outer emit_gc_sections() which always appends the sections regardless of whether the GC ran and an inner maybe_gc() that can safely return early. Add a test with a .dwo whose dwo_id doesn't match any executable to verify its str_offsets section is preserved in the output .dwp.
When a DWARF5 .debug_loclists.dwo section has offset_entry_count == 0 (spec-legal when DW_FORM_sec_offset is used instead of DW_FORM_loclistx), rewrite_loclists' offset-table loop never executed, so DW_OP_call4 operands kept stale pre-GC DIE offsets. Flat-scan the entry data after the header through patch_loclist_data, matching how patch_debug_loc handles DWARF4.
davidtwco
left a comment
davidtwco
left a comment
There was a problem hiding this comment.
I've had a high-level read through this and it makes sense to me. I'm open to merging it. I think I'd like it to be behind a Cargo feature flag or some similar mechanism to indicate that it's something we're still kicking the tyres on - it's a big change, and while everything here looks reasonable to me, I'm not 100% confident in it, it's hard to take in all the details from just reading the code a lot. We could still expose it through an unstable flag in rustc to make it easier to try out on bigger projects and get more confidence in it.
|
That sounds reasonable. My biggest concern with this was the memory allocation behavior. There's a bunch of transient stuff in gc.rs that's not using the |
|
(super glad to see work in this direction) - the reason I hadn't looked into this on the LLVM side was I had assumed the compute and memory use was going to be too high for my situation - any sense of how much this increases the memory usage and runtime? (maybe it manages to reduce it if it saves so much work) & may be of interest to compare/contrast performance and the resulting DWARF with llvm-dsymutil/dwarfutil and maybe llvm bolt, which recently got a DWARF-aware linking (implemented separately/differently from the dsymutil implementation, for better and worse) |
|
I haven't measured memory usage, but the runtime increase on the two cases I've tested is 10x (in the case where we shrink things substantially on the closed source project) and 25x (for rustc_driver.so, where the savings are minimal). That difference makes some amount of sense, you'd expect the runtime to be heavily influenced by how much of the DWARF tree is live. I haven't yet done any profiling to see if we could go faster, and this is all single threaded but in principle each .dwo could be processed independently in parallel. |
|
Alright, the cfg feature is added, CI is green, and this depends on a released version of gimli now. As long as you're fine with the memory management stuff I mentioned earlier I think this is ready to go. |
I'll be honest, I don't recall why I abstracted over the memory management like that. I'd probably prefer you leveraged it too for this, but that can happen as a follow-up given this is behind a feature flag. |
|
Filed #52 (comment) so we follow-up on that |
5 participants
Disclosure: this PR was written with the assistance of Claude. The following PR description was authored solely by me. The code changes have been either written by or reviewed by me. The tests have been reviewed by me to the extent practicable, and have been verified to fail without this PR where appropriate.
Background
It is well known that rustc relies on the linker to remove dead code via the equivalent of -ffunction-sections and --gc-sections. This approach works well for code, but while each function gets its own section for code, that is not true for DWARF (the resulting blow up in sizes if each function had to emit DWARF for all of its types, inlines, etc would surely be severe), and DWARF for dead code is retained. There have been multiple failed efforts to have lld GC the DWARF. To date, this issue remains unsolved and complaints about the size of Rust debuginfo remain common.
Split DWARF offers an opportunity to solve this at the DWARF package file generation stage. To produce a .dwp thorin has to touch all of the DWARF anyways (though it's currently largely just passed through). We are not bound by lld's legacy understanding of what --gc-sections means (where, based on the failure of the first attempt to do this in lld, its expected that DWARF for dead code will be passed through in some situations). Split DWARF (and DWARF 5 in general) restructures DWARF in ways that make garbage collecting DWARF more feasible (the introduction of .debug_addr, no cross-CU references in .debug_info, well defined offset tables for figuring out what is used in the supporting sections like .debug_loclists.dwo, .debug_rnglists.dwo, etc). And .dwp generation is downstream of linking the binary, so the linker has already identified all the dead code and tombstoned the appropriate .debug_addr table entries for us.
Mechanics
This PR introduces API surface for garbage collecting executables and input objects on addition to a DWARF package. Garbage collection only works if we can examine the .debug_addr tables, and since .debug_addr tables live in the executable, we can only GC input objects that belong to an executable. We also need to prescan the executable to find the relevant .debug_addr data before processing individual input objects. thorin-dwp-bin gets a --gc flag that drives the new API, and you can see there that the consumer changes are simple.
Conceptually GCing .debug_info.dwo is relatively straightforward. We identify roots based on subprograms and variables with non-tombstoned addresses, mark, and sweep. We always patch the .debug_locs.dwo/.debug_loclists.dwo sections to adjust the offsets of any references back into .debug_info.dwo. If circumstances permit, we attempt to shrink the .debug_loclists.dwo/.debug_rnglists.dwo/.debug_str_offsets.dwo sections by removing now-unused offset table entries and any corresponding data. We do not attempt the same for the DWARF 4 extension .debug_locs.dwo section. We do not attempt to modify the executable to shrink the .debug_addr table or any other data that lives there (e.g. the DWARF 4 .debug_ranges).
Results
The results here are highly workload dependent. On librustc_driver.so, for example, the size reduction is minimal.
However, on a large closed-source Rust project of mine that makes extensive use of third-party crates for things like interacting with AWS, the reduction is dramatic. --gc reduces the total size of the .dwp by 65% (from 1.35GB to just under 0.5GB) including a ~60% reduction in .debug_info, a ~75% reduction in .debug_rnglists, a ~83% reduction in .debug_loclists, a ~45% reduction in .debug_str_offsets, and a ~70% reduction in .debug_str.
I think that's compelling enough to merge.
Correctness Testing
There are a variety of unit tests included in this PR. It's difficult to measure programmatically whether useful debug info was lost/corrupted in real world applications but I have tested this on my project and not noticed any degradation in quality. I've also examined the diff in the .debug_str sections when --gc is used and the strings that are newly dropped generally comport with my expectations (e.g. for the aws-sdk-* crates we pull in I see lots of newly dropped strings corresponding to API calls we don't use).
To-dos before merging this PR.
To-dos either before or after merging this PR
To-dos after merging this PR.