Uh oh!
There was an error while loading. Please reload this page.
debuginfo: Make sure that type names for closure and generator environments are unique in debuginfo. - #93154
Conversation
rust-highfive
commented
Jan 21, 2022
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
Uh oh!
There was an error while loading. Please reload this page.
michaelwoerister
commented
Jan 21, 2022
There might be some indeterminism in the order some of the LLVM IR is produced |
This comment has been minimized.
This comment has been minimized.
be342d5 to
2022bacComparemichaelwoerister
commented
Jan 21, 2022
Using the new symbol mangling scheme should make codegen item order independent of rustc's version (within codegen items of the same crate). Let's see if this is enough to stabilize the test. |
| // MSVC: !DIGlobalVariable(name: "impl$<debug_vtable::bar::closure_env$0, core::ops::function::FnOnce<tuple$<enum$<core::option::Option<ref$<dyn$<core::ops::function::Fn<tuple$<>,assoc$<Output,tuple$<> > > > > >, {{.*}}, {{.*}}, Some> > > >::vtable$" | ||
| // NONMSVC: !DIGlobalVariable(name: "<debug_vtable::generic_closure::{closure_env#0}<bool> as core::ops::function::FnOnce<()>>::{vtable}" | ||
| // MSVC: !DIGlobalVariable(name: "impl$<debug_vtable::generic_closure::closure_env$0<bool>, core::ops::function::FnOnce<tuple$<> > >::vtable$ |
There was a problem hiding this comment.
I was going to comment on the tuple$<> thing which seems like it could break natvis but I see we already filed #89704 for that.
wesleywiser
commented
Jan 31, 2022
r? @wesleywiser |
bors
commented
Jan 31, 2022
📌 Commit 2022bac has been approved by |
…e-and-generator-debuginfo, r=wesleywiser
debuginfo: Make sure that type names for closure and generator environments are unique in debuginfo.
Before this change, closure/generator environments coming from different instantiations of the same generic function were all assigned the same name even though they were distinct types with potentially different data layout. Now we append the generic arguments of the originating function to the type name.
This commit also emits `{closure_env#0}` as the name of these types in order to disambiguate them from the accompanying closure function (which keeps being called `{closure#0}`). Previously both were assigned the same name.
NOTE: Changing debuginfo names like this can break pretty printers and other debugger plugins. I think it's OK in this particular case because the names we are changing were ambiguous anyway. In general though it would be great to have a process for doing changes like these.matthiaskrgr
commented
Jan 31, 2022
Might have failed in a rollup on windows: #93510 (comment) |
…nments are unique in debuginfo.
Before this change, closure/generator environments coming from different
instantiations of the same generic function were all assigned the same
name even though they were distinct types with potentially different data
layout. Now we append the generic arguments of the originating function
to the type name.
This commit also emits '{closure_env#0}' as the name of these types in
order to disambiguate them from the accompanying closure function
'{closure#0}'. Previously both were assigned the same name.2022bac to
fd7557bComparemichaelwoerister
commented
Feb 1, 2022
Fixed the test case. @bors r=wesleywiser |
bors
commented
Feb 1, 2022
📌 Commit fd7557b has been approved by |
bors
commented
Feb 2, 2022
bors
commented
Feb 2, 2022
☀️ Test successful - checks-actions |
rust-timer
commented
Feb 2, 2022
Finished benchmarking commit (dca1e7a): comparison url. Summary: This benchmark run shows 18 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
Mark-Simulacrum
commented
Feb 8, 2022
@michaelwoerister it looks like this had a negative effect on a number of -debug benchmarks, which I suppose is likely expected -- do you think there are any particular spots where this impl might be inefficient (and so should specifically be investigated), or is this just roughly an expected result? If expected, then please mark as perf-regression-triaged. |
…debuginfo-regression, r=wesleywiser debuginfo: Fix DW_AT_containing_type vtable debuginfo regression This PR brings back the `DW_AT_containing_type` attribute for vtables after it has accidentally been removed in rust-lang#89597. It also implements a more accurate description of vtables. Instead of describing them as an array of void pointers, the compiler will now emit a struct type description with a field for each entry of the vtable. r? `@wesleywiser` This PR should fix issue rust-lang#93164. ~~The PR is blocked on rust-lang#93154 because both of them modify the `codegen/debug-vtable.rs` test case.~~
…debuginfo-regression, r=wesleywiser debuginfo: Fix DW_AT_containing_type vtable debuginfo regression This PR brings back the `DW_AT_containing_type` attribute for vtables after it has accidentally been removed in rust-lang#89597. It also implements a more accurate description of vtables. Instead of describing them as an array of void pointers, the compiler will now emit a struct type description with a field for each entry of the vtable. r? ``@wesleywiser`` This PR should fix issue rust-lang#93164. ~~The PR is blocked on rust-lang#93154 because both of them modify the `codegen/debug-vtable.rs` test case.~~
Before this change, closure/generator environments coming from different instantiations of the same generic function were all assigned the same name even though they were distinct types with potentially different data layout. Now we append the generic arguments of the originating function to the type name.
This commit also emits
{closure_env#0}as the name of these types in order to disambiguate them from the accompanying closure function (which keeps being called{closure#0}). Previously both were assigned the same name.NOTE: Changing debuginfo names like this can break pretty printers and other debugger plugins. I think it's OK in this particular case because the names we are changing were ambiguous anyway. In general though it would be great to have a process for doing changes like these.