Don't merge vtables when full debuginfo is enabled. #107373
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
|
@michaelwoerister are we at all concerned that this makes it more likely that enabling debug info changes behavior, e.g., if investigating a bug related to vtable deduplication (such as when hashing pointers)? |
|
Interesting question -- I don't think we can guarantee that the difference in behavior won't ever make debugging a vtable-related compiler bug a bit harder. But it also doesn't seem like an obvious footgun to me. Do you have a concrete scenario in mind? |
|
Not in particular, more of a vague worry when we make debug differ from optimized builds. It's probably unlikely to be a huge problem. |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#107022 (Implement `SpecOptionPartialEq` for `cmp::Ordering`) - rust-lang#107100 (Use proper `InferCtxt` when probing for associated types in astconv) - rust-lang#107103 (Use new solver in `evaluate_obligation` query (when new solver is enabled)) - rust-lang#107190 (Recover from more const arguments that are not wrapped in curly braces) - rust-lang#107306 (Correct suggestions for closure arguments that need a borrow) - rust-lang#107339 (internally change regions to be covariant) - rust-lang#107344 (Minor tweaks in the new solver) - rust-lang#107373 (Don't merge vtables when full debuginfo is enabled.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
I understand the concern. The behavior can easily be changed if it turns out to be a problem. |
This PR makes the compiler not emit the
unnamed_addrattribute for vtables when full debuginfo is enabled, so that they don't get merged even if they have the same contents. This allows debuggers to more reliably map from a dyn pointer to the self-type of a trait object by looking at the vtable's debuginfo.The PR only changes the behavior of the LLVM backend as other backends don't emit vtable debuginfo (as far as I can tell).
The performance impact of this change should be small as measured in a previous PR.