Preserve the .debug_gdb_scripts section #143679
Conversation
rustbot
added
A-LLVM
|
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_gcc |
sebastianpoeplau
force-pushed
the
preserve-debug-gdb-scripts-section
branch
2 times, most recently
from
97ac05e to
07de96b
Compare
sebastianpoeplau
force-pushed
the
preserve-debug-gdb-scripts-section
branch
from
07de96b to
d54c3ea
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
…b-scripts-section, r=bjorn3 Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. r? `@bjorn3`
…b-scripts-section, r=bjorn3 Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. r? ``@bjorn3``
Rollup of 8 pull requests Successful merges: - #140136 (Add an aarch64-msvc build running on ARM64 Windows) - #143642 (stdarch subtree update) - #143679 (Preserve the .debug_gdb_scripts section) - #143707 (Fix `--skip-std-check-if-no-download-rustc`) - #143722 (Make some "safe" llvm ops actually sound) - #143728 (Resolve refactor: extraction of `finalize_module_binding` and `single_import_can_define_name`) - #143742 (Rework borrowing suggestions to use `Expr` instead of just `Span`) - #143744 (Properly track the depth when expanding free alias types) r? `@ghost` `@rustbot` modify labels: rollup
|
Looks like some GDB tests failed #143755 (comment) |
bors
added
S-waiting-on-author
…ection, r=<try> Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. r? `@bjorn3` try-job: dist-i586-gnu-i586-i686-musl
|
💔 Test failed
|
Interesting, I'm going to have a look. The test passes on my |
|
You might be able to get away with passing If that doesn't repro it for some reason you can replicate the CI setup pretty much exactly by running the docker tests https://rustc-dev-guide.rust-lang.org/tests/docker.html, with the downside that this takes about as long as CI does. |
|
Hmm, the full log of the CI job led me to believe that it's I noticed though that my GDB is version 15, whereas the test log shows GDB 12; maybe that's the reason for the discrepancy. In general, the |
|
You're right, it's i586. I must have been reading the wrong line. I assume there probably isn't any reason we need to keep a very old gdb version if that makes a difference |
|
Bors forgot to mention it, but now that #144738 has landed there is a minor conflict. |
Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to "llvm.used". The volatile load in the main shim is retained because "llvm.used", which translates to SHF_GNU_RETAIN on ELF targets, requires a reasonably recent linker; emitting the volatile load ensures compatibility with older linkers, at least when libstd is used. Pretty printers in dylib dependencies are now emitted by the main crate instead of the dylib; apart from matching how rlibs are handled, this approach has the advantage that `omit_gdb_pretty_printer_section` keeps working with dylib dependencies.
sebastianpoeplau
force-pushed
the
preserve-debug-gdb-scripts-section
branch
from
13f9889 to
cc2d4b0
Compare
| @@ -84,35 +95,5 @@ pub(crate) fn get_or_insert_gdb_debug_scripts_section_global<'ll>( | |||
| } | |||
There was a problem hiding this comment.
I just noticed that the section is created with LinkOnceODR linkage. This was fine when all global visualizers were merged together and a single section was created across all crates, but now that we create different sections for each crate, that would cause one of the sections to be chosen non-deterministically. I think it would be better to use regular linkage, but instead enable string merging. This is also what the GDB manual page for .debug_gdb_scripts suggests. I think the way to do that in LLVM is to put the \x01gdb_load_rust_pretty_printers.py\0 as well as every visualizer in a separate constant global variables with unnamed_addr but still in the same .debug_gdb_scripts section.
There was a problem hiding this comment.
To be honest, I don't think it matters whether we use linkonce linkage here. I agree that the attribute is unnecessary, but as far as I can see it's not harmful either: My understanding based on the LLVM Language Reference is that the attribute affects how the linker treats multiple globals of the same name (choosing one of them non-deterministically), but here we're creating globals with different names, unique per crate. Shouldn't each name exist exactly once? They're all in the same section, and we rely on the linker to merge the section values across object files, but there's no conflict between them.
What do you mean by "instead enable string merging"? Linkage type appending? It would be nice if we could use it, but it seems to work only internally in LLVM:
Unfortunately this doesn’t correspond to any feature in .o files, so it can only be used for variables like llvm.global_ctors which llvm interprets specially.
There was a problem hiding this comment.
I played with dropping linkonce, and actually it does have an effect when multiple codegen units are used per crate. However, the value should be the same in each codegen unit of the same crate because we collect pretty printers crate-wide 🤔 In this case the effect of choosing one of the globals with the same name (which all have the same value) is what we want, isn't it?
There was a problem hiding this comment.
My understanding based on the LLVM Language Reference is that the attribute affects how the linker treats multiple globals of the same name (choosing one of them non-deterministically), but here we're creating globals with different names, unique per crate.
Right missed that. In that case string merging would just be a binary size optimization that can be left for a future PR.
What do you mean by "instead enable string merging"? Linkage type appending?
No, most object file formats have a feature where you can mark a section as string merging, which causes the linker to interpret the section as a sequence of C strings and deduplicate identical strings. LLVM should automatically treat constant global variables marked as unnamed_addr as C strings if they end with a NUL byte and don't have any internal NUL bytes.
|
@bors r+ |
bors
added
S-waiting-on-bors
…ection, r=bjorn3 Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. Pretty printers in dylib dependencies are now emitted by the main crate instead of the dylib; apart from matching how rlibs are handled, this approach has the advantage that `omit_gdb_pretty_printer_section` keeps working with dylib dependencies. r? `@bjorn3`
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
There's a problem with old linkers again: We're emitting the volatile load in the main shim, but pretty printers are now collected per crate, meaning that the load only preserves the main crate's pretty printers. As far as I can see, this affects only a single test (which uses pretty printers from an rlib). Maybe it's okay to disable it, expecting that people generally have more recent linkers than the CI setup? |
Instead of collecting pretty printers transitively when building executables/staticlibs/cdylibs, let the debugger find each crate's pretty printers via its .debug_gdb_scripts section. This covers the case where libraries defining custom pretty printers are loaded dynamically.
sebastianpoeplau
force-pushed
the
preserve-debug-gdb-scripts-section
branch
from
cc2d4b0 to
b4d923c
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
…ection, r=bjorn3 Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. Pretty printers in dylib dependencies are now emitted by the main crate instead of the dylib; apart from matching how rlibs are handled, this approach has the advantage that `omit_gdb_pretty_printer_section` keeps working with dylib dependencies. r? `@bjorn3`
…b-scripts-section, r=bjorn3 Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. Pretty printers in dylib dependencies are now emitted by the main crate instead of the dylib; apart from matching how rlibs are handled, this approach has the advantage that `omit_gdb_pretty_printer_section` keeps working with dylib dependencies. r? `@bjorn3`
|
Yielding to enclosing rollup: @bors retry |
Rollup of 21 pull requests Successful merges: - #137831 (Tweak auto trait errors) - #138689 (add nvptx_target_feature) - #140267 (implement continue_ok and break_ok for ControlFlow) - #143679 (Preserve the .debug_gdb_scripts section) - #143857 (Port #[macro_export] to the new attribute parsing infrastructure) - #143929 (Mark all deprecation lints in name resolution as deny-by-default and report-in-deps) - #144133 (Stabilize const TypeId::of) - #144369 (Upgrade semicolon_in_expressions_from_macros from warn to deny) - #144473 (Address libunwind.a inconsistency issues in the bootstrap program) - #144498 (Add --print target-spec-json-schema) - #144552 (Rehome 33 `tests/ui/issues/` tests to other subdirectories under `tests/ui/`) - #144659 (bootstrap: refactor mingw dist and fix gnullvm) - #144676 (Add documentation for unstable_feature_bound) - #144794 (Port `#[coroutine]` to the new attribute system) - #144835 (Anonymize binders in tail call sig) - #144836 (Change visibility of Args new function) - #144861 (Stabilize `panic_payload_as_str` feature) - #144910 (Add regression tests for seemingly fixed issues) - #144913 ([rustdoc] Fix wrong `i` tooltip icon) - #144917 (Enforce tail call type is related to body return type in borrowck) - #144924 (compiletest: add hint for when a ui test produces no errors) r? `@ghost` `@rustbot` modify labels: rollup
…ection, r=bjorn3 Preserve the .debug_gdb_scripts section Make sure that compiler and linker don't optimize the section's contents away by adding the global holding the data to `llvm.used`. This eliminates the need for a volatile load in the main shim; since the LLVM codegen backend is the only implementer of the corresponding trait function, remove it entirely. Pretty printers in dylib dependencies are now emitted by the main crate instead of the dylib; apart from matching how rlibs are handled, this approach has the advantage that `omit_gdb_pretty_printer_section` keeps working with dylib dependencies. r? `@bjorn3`
Make sure that compiler and linker don't optimize the section's contents
away by adding the global holding the data to
llvm.used. Thiseliminates the need for a volatile load in the main shim; since the LLVM
codegen backend is the only implementer of the corresponding trait
function, remove it entirely.
Pretty printers in dylib dependencies are now emitted by the main crate
instead of the dylib; apart from matching how rlibs are handled, this
approach has the advantage that
omit_gdb_pretty_printer_sectionkeepsworking with dylib dependencies.
r? @bjorn3