This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fixup DIEs for inlined functions
AbandonedPublic

Authored by ellis on Oct 15 2021, 5:11 PM.

Details

Reviewers
dblaikie
Summary

Some DIEs, like for static variables, get added to concrete DIEs for
inlined functions instead of their abstract origin DIEs. At the end of
the module, we fixup these cases by moving appropriate child DIEs from
the concrete DIE to the abstract origin DIE and adding the
DW_AT_abstract_origin attribute.

Add the API releaseNodes() to DIEs so that we can remove all the
children from a DIE.

This diff was discussed in llvm-dev.
https://groups.google.com/g/llvm-dev/c/mlNUyj-Q5To/m/GLq2zWw1AAAJ

Fixes

Diff Detail

Event Timeline

ellis created this revision.Oct 15 2021, 5:11 PM
ellis published this revision for review.Oct 15 2021, 6:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2021, 6:56 PM

So, currently we put the DIEs in the concrete definition and then when we didn't need a concrete one we end up with this weird uninitialized DIE with a few children, that we'd like to have those up in the abstract origin instead, right?

And this change moves those DIEs up at the end, trying to filter the DIEs even if we did end up legitimately creating the concrete DIE (so we only move the DIEs that should reside in the abstract origin? (ie: we don't move up a local non-static variable from concrete to abstract))?

Hmm - what would happen if we moved them up earlier - like as soon as we create the concrete one properly/legitimately, we move anything that came before that up to the abstract origin?

I guess that would break down if we hadn't created the abstract origin at that point (ie: would leave the static local down in the concrete definition instead of the abstract definition)?

Maybe we could always create the abstract origin, then at the end collapse it down into the concrete definition if it's unused/there are no inline instances? Less need to try to separate things out?

ellis added a comment.Oct 15 2021, 8:37 PM

So, currently we put the DIEs in the concrete definition and then when we didn't need a concrete one we end up with this weird uninitialized DIE with a few children, that we'd like to have those up in the abstract origin instead, right?

Yep

And this change moves those DIEs up at the end, trying to filter the DIEs even if we did end up legitimately creating the concrete DIE (so we only move the DIEs that should reside in the abstract origin? (ie: we don't move up a local non-static variable from concrete to abstract))?

Yeah, even if the concrete DIE is legitimate we don't want it to hold the static variables since they wouldn't be visible to inlined subroutines. For non-static variables, those DIEs actually belong to the abstract origin, but the concrete DIE also has a DIE that references them via DW_AT_abstract_origin.

// Concrete
0x0000002a:   DW_TAG_subprogram
                DW_AT_low_pc	(0x0000000000000000)
                DW_AT_high_pc	(0x0000000000000011)
                DW_AT_frame_base	(DW_OP_reg6 RBP)
                DW_AT_abstract_origin	(0x00000046 "_Z7removedv")

0x0000003d:     DW_TAG_variable
                  DW_AT_location	(DW_OP_fbreg -4)
                  DW_AT_abstract_origin	(0x00000056 "A")

0x00000045:     NULL

// Abstract origin
0x00000046:   DW_TAG_subprogram
                DW_AT_linkage_name	("_Z7removedv")
                DW_AT_name	("removed")
                DW_AT_decl_file	("foo.cpp")
                DW_AT_decl_line	(2)
                DW_AT_type	(0x00000062 "int")
                DW_AT_external	(true)
                DW_AT_inline	(DW_INL_inlined)

0x00000056:     DW_TAG_variable
                  DW_AT_name	("A")
                  DW_AT_decl_file	("foo.cpp")
                  DW_AT_decl_line	(2)
                  DW_AT_type	(0x00000062 "int")

I mentioned this in one of the comments, but even with this patch we don't do this for static variables and I was wondering if we should.

Hmm - what would happen if we moved them up earlier - like as soon as we create the concrete one properly/legitimately, we move anything that came before that up to the abstract origin?

I guess that would break down if we hadn't created the abstract origin at that point (ie: would leave the static local down in the concrete definition instead of the abstract definition)?

Yep, at that point there is no abstract origin and we don't know if there will be one.

Maybe we could always create the abstract origin, then at the end collapse it down into the concrete definition if it's unused/there are no inline instances? Less need to try to separate things out?

I'd have to think about this a bit more.

krisb added a subscriber: krisb.Oct 16 2021, 6:37 AM
ellis added a comment.Oct 19 2021, 9:53 AM

Maybe we could always create the abstract origin, then at the end collapse it down into the concrete definition if it's unused/there are no inline instances? Less need to try to separate things out?

Since some inlined functions also have concrete instances, we won't know until the end of the module if a function with a concrete instance should have an abstract origin or not. That means we have to maintain both an abstract origin and possibly a concrete DIE for every function, then at the end replace the abstract origin if it isn't used. I think this will cost a lot more memory and also require deleting a DIE, which I haven't seen in the code.

Maybe we could always create the abstract origin, then at the end collapse it down into the concrete definition if it's unused/there are no inline instances? Less need to try to separate things out?

Since some inlined functions also have concrete instances, we won't know until the end of the module if a function with a concrete instance should have an abstract origin or not.

Yep, that's why I was suggesting doing it always - because we don't know.

That means we have to maintain both an abstract origin and possibly a concrete DIE for every function, then at the end replace the abstract origin if it isn't used.

Yep.

I think this will cost a lot more memory and also require deleting a DIE, which I haven't seen in the code.

be nice to have some data on how much memory that costs - I'm not sure deleting a DIE would be complicated (I haven't checked on the ownership model in a while) - I think they're block allocated, and the block allocator handles cleaning them up? So having a DIE that gets removed from the DIE tree is /probably/ OK?

be nice to have some data on how much memory that costs - I'm not sure deleting a DIE would be complicated (I haven't checked on the ownership model in a while) - I think they're block allocated, and the block allocator handles cleaning them up? So having a DIE that gets removed from the DIE tree is /probably/ OK?

I did some testing and it isn't very hard to remove a DIE from the tree, but I ran into some problems when I tried to maintain a concrete and abstract DIE for every function, so I didn't measure the memory overhead. The problem with deferring the decision of making a DIE concrete or abstract is that updateSubprogramScopeDIE(), which makes a DIE concrete by adding location info, references the MF and must be called only when processing that function.

However, I realized it isn't hard to iterate through the module's functions to see which SP are concrete. Then we can know when it is not ok to create a concrete DIE and instead create/reference the abstract DIE. I've just created https://reviews.llvm.org/D112337 which does this.