debuginfo: Make names of types in debuginfo reliable and omit source locations from debug info type descriptions. #15199
Conversation
|
Forgot to mention: This PR is in response to issue #15045. |
|
@michaelwoerister What do you mean you've reverted the change in LLVM? I don't see any submodule changes. |
|
I just reverted the change just locally on my machine for testing. This PR does not include any change to LLVM. On June 28, 2014 9:51:32 AM CEST, Luqman Aden notifications@github.com wrote:
Sent from my Android device with K-9 Mail. Please excuse my brevity. |
|
@michaelwoerister So then, just to be clear, these changes also fix that issue (#15156)? |
|
No, this PR doesn't fix issue #15156. Sorry for phrasing that so ambiguously. I just had to circumvent the segfault, so I could properly test that LTO + debuginfo now correctly work together. Otherwise the segfaulting issue is not related to this PR or the issue it fixes. |
| fn push_debuginfo_type_name(cx: &CrateContext, | ||
| t: ty::t, | ||
| qualified: bool, | ||
| output:&mut String) { |
There was a problem hiding this comment.
nit: line these 2 up with the other arguments.
|
I'll add a test case for checking type names. It's not good that these |
|
I've added test cases for type names in |
…locations from debug info type descriptions. So far, type names generated for debuginfo where a bit sketchy. It was not clearly defined when a name should be fully qualified and when not, if region parameters should be shown or not, and other things like that. This commit makes the debuginfo module responsible for creating type names instead of using ppaux::ty_to_str() and brings type names, as they show up in the DWARF information, in line with GCC and Clang: * The name of the type being described is unqualified. It's path is defined by its position in the namespace hierarchy. * Type arguments are always fully qualified, no matter if they would actually be in scope at the type definition location. Care is also taken to reliably make type names consistent across crate boundaries. That is, the code now tries make the type name the same, regardless if the type is in the local crate or reconstructed from metadata. Otherwise LLVM will complain about violating the one-definition-rule when using link-time-optimization. This commit also removes all source location information from type descriptions because these cannot be reconstructed for types instantiated from metadata. Again, with LTO enabled, this can lead to two versions of the debuginfo type description, one with and one without source location information, which then triggers the LLVM ODR assertion. Fortunately, source location information about types is rarely used, so this has little impact. Once source location information is preserved in metadata (rust-lang#1972) it can also be reenabled for type descriptions.
|
OK, this should be done now. I've added some much needed test cases and fixed an issue with traits in the process. I've also addressed the problems pointed out by @luqmana. |
| .find_metadata_for_unique_id(unique_type_id) { | ||
| Some(metadata) => return metadata, | ||
| None => { /* proceed normally */ } | ||
| }; |
There was a problem hiding this comment.
Isn't this what return_if_created_in_meantime! does?
There was a problem hiding this comment.
Yes, it is but I removed the macro in the form that is needed here (returning a DIType) and added another form returning a MetadataCreationResult, because it is used much more often in the other functions. Only two occurences of the "raw metadata" form were left and it competed for a good name with the more often used form, so I decided to get rid of it.
|
@michaelwoerister Just that one last nit, otherwise r=me. Thanks for all your work on this! |
…=luqmana So far, type names generated for debuginfo where a bit sketchy. It was not clearly defined when a name should be fully qualified and when not, if region parameters should be shown or not, and other things like that. This commit makes the debuginfo module responsible for creating type names instead of using `ppaux::ty_to_str()` and brings type names (as they show up in the DWARF information) in line with GCC and Clang: * The name of the type being described is unqualified. It's path is defined by its position in the namespace hierarchy. * Type arguments are always fully qualified, no matter if they would actually be in scope at the type definition location. Care is also taken to make type names consistent across crate boundaries. That is, the code now tries make the type name the same, regardless if the type is in the local crate or reconstructed from metadata. Otherwise LLVM will complain about violating the one-definition-rule when using link-time-optimization. This commit also removes all source location information from type descriptions because these cannot be reconstructed for types instantiated from metadata. Again, with LTO enabled, this can lead to two versions of the debuginfo type description, one with and one without source location information, which then triggers the LLVM ODR assertion. Fortunately, source location information about types is rarely used, so this has little impact. Once source location information is preserved in metadata (#1972) it can also be re-enabled for type descriptions. `RUSTFLAGS=-g make check` no works again for me locally, including the LTO test cases (note that I've taken care of #15156 by reverting the change in LLVM that @luqmana identified as the culprit for that issue).
Revert "Support `#[rustc_coinductive]`" Reverts rust-lang#15125, addresses rust-lang/rust-analyzer#15125 (comment) I'll add the support again once I figure out the problem.
So far, type names generated for debuginfo where a bit sketchy. It was not clearly defined when a name should be fully qualified and when not, if region parameters should be shown or not, and other things like that.
This commit makes the debuginfo module responsible for creating type names instead of using
ppaux::ty_to_str()and brings type names (as they show up in the DWARF information) in line with GCC and Clang:Care is also taken to make type names consistent across crate boundaries. That is, the code now tries make the type name the same, regardless if the type is in the local crate or reconstructed from metadata. Otherwise LLVM will complain about violating the one-definition-rule when using link-time-optimization.
This commit also removes all source location information from type descriptions because these cannot be reconstructed for types instantiated from metadata. Again, with LTO enabled, this can lead to two versions of the debuginfo type description, one with and one without source location information, which then triggers the LLVM ODR assertion.
Fortunately, source location information about types is rarely used, so this has little impact. Once source location information is preserved in metadata (#1972) it can also be re-enabled for type descriptions.
RUSTFLAGS=-g make checkno works again for me locally, including the LTO test cases (note that I've taken care of #15156 by reverting the change in LLVM that @luqmana identified as the culprit for that issue).