This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][Try 2] Only create concrete DIEs of concrete functions
AbandonedPublic

Authored by ellis on Nov 11 2021, 11:13 PM.

Details

Reviewers
dblaikie
Summary

At the begining of the module we can iterate through the functions to
see which SPs should have concrete DIEs. Then when we need to reference
a DIE for a SP we can decide if it's ok to create a concrete DIE or not.

Fixes

This was previously landed in https://reviews.llvm.org/D112337 but was
reverted because of a memory bug. This patch fixes that bug by making
sure we don't reference abstract origins from another CU.

Diff Detail

Event Timeline

ellis created this revision.Nov 11 2021, 11:13 PM
ellis published this revision for review.Nov 11 2021, 11:27 PM
ellis added a reviewer: dblaikie.
ellis added a project: debug-info.
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2021, 11:28 PM
ellis added a comment.Nov 12 2021, 1:02 PM

I just saw https://reviews.llvm.org/D113741 but I haven't done a careful review yet. If the tests from this patch pass there then I would be happy to go with that solution.

dblaikie added a subscriber: krisb.Nov 12 2021, 2:31 PM

@krisb @ellis - can we have some design discussion of these alternatives in one place (here, or an llvm-dev thread) to try to avoid redundant/divergent design discussions across multiple mutually exclusive patches/directions to address these issues?

krisb added a comment.Nov 15 2021, 2:55 AM

@ellis, @dblaikie

can we have some design discussion of these alternatives in one place (here, or an llvm-dev thread) to try to avoid redundant/divergent design discussions across multiple mutually exclusive patches/directions to address these issues?

That probably would have been done in the first place. But I was going to base on ellis's work for inlined SPs while working on lexical blocks scopes for static locals and other until realized that ellis's patch doesn't fix the issue for SPs with concrete out-of-line instances (which I'm looking to get fixed as well).
The problem is if we have a concrete out-of-line instance, static locals and local type declarations will be children of this concrete scope, which makes them not accessible from inlined SPs.
To solve this problem we need to know if the given SP has (or will have) abstract origin or not before we will try to emit any local declarations. Thus we need either (or both) to collect this information before processing local entities or (and) to postpone creating local entities until we have all SPs get emitted. The latter one seems the most safe and reliable option, except for the fact that it completely changes the order of emitted entities (but it's still possible to maintain some specific order, if the ordering is an issue).

@ellis, @dblaikie

can we have some design discussion of these alternatives in one place (here, or an llvm-dev thread) to try to avoid redundant/divergent design discussions across multiple mutually exclusive patches/directions to address these issues?

That probably would have been done in the first place. But I was going to base on ellis's work for inlined SPs while working on lexical blocks scopes for static locals and other until realized that ellis's patch doesn't fix the issue for SPs with concrete out-of-line instances (which I'm looking to get fixed as well).
The problem is if we have a concrete out-of-line instance, static locals and local type declarations will be children of this concrete scope, which makes them not accessible from inlined SPs.
To solve this problem we need to know if the given SP has (or will have) abstract origin or not before we will try to emit any local declarations. Thus we need either (or both) to collect this information before processing local entities or (and) to postpone creating local entities until we have all SPs get emitted. The latter one seems the most safe and reliable option, except for the fact that it completely changes the order of emitted entities (but it's still possible to maintain some specific order, if the ordering is an issue).

Hmm, I'm still a bit lost with regards to which patches fix what, whether they overlap, are dependent, or are independent.

For instance, does D113741 subsume this patch (D113736)? (ie: if we take D113741 do we still/not need D113736?)

ellis added a comment.Nov 29 2021, 4:30 PM

@ellis, @dblaikie

can we have some design discussion of these alternatives in one place (here, or an llvm-dev thread) to try to avoid redundant/divergent design discussions across multiple mutually exclusive patches/directions to address these issues?

That probably would have been done in the first place. But I was going to base on ellis's work for inlined SPs while working on lexical blocks scopes for static locals and other until realized that ellis's patch doesn't fix the issue for SPs with concrete out-of-line instances (which I'm looking to get fixed as well).
The problem is if we have a concrete out-of-line instance, static locals and local type declarations will be children of this concrete scope, which makes them not accessible from inlined SPs.
To solve this problem we need to know if the given SP has (or will have) abstract origin or not before we will try to emit any local declarations. Thus we need either (or both) to collect this information before processing local entities or (and) to postpone creating local entities until we have all SPs get emitted. The latter one seems the most safe and reliable option, except for the fact that it completely changes the order of emitted entities (but it's still possible to maintain some specific order, if the ordering is an issue).

Hmm, I'm still a bit lost with regards to which patches fix what, whether they overlap, are dependent, or are independent.

For instance, does D113741 subsume this patch (D113736)? (ie: if we take D113741 do we still/not need D113736?)

D113741 includes the test cases for this patch so if we take D113741 (and I think we should) then I will drop this patch.

@ellis, @dblaikie

can we have some design discussion of these alternatives in one place (here, or an llvm-dev thread) to try to avoid redundant/divergent design discussions across multiple mutually exclusive patches/directions to address these issues?

That probably would have been done in the first place. But I was going to base on ellis's work for inlined SPs while working on lexical blocks scopes for static locals and other until realized that ellis's patch doesn't fix the issue for SPs with concrete out-of-line instances (which I'm looking to get fixed as well).
The problem is if we have a concrete out-of-line instance, static locals and local type declarations will be children of this concrete scope, which makes them not accessible from inlined SPs.
To solve this problem we need to know if the given SP has (or will have) abstract origin or not before we will try to emit any local declarations. Thus we need either (or both) to collect this information before processing local entities or (and) to postpone creating local entities until we have all SPs get emitted. The latter one seems the most safe and reliable option, except for the fact that it completely changes the order of emitted entities (but it's still possible to maintain some specific order, if the ordering is an issue).

Hmm, I'm still a bit lost with regards to which patches fix what, whether they overlap, are dependent, or are independent.

For instance, does D113741 subsume this patch (D113736)? (ie: if we take D113741 do we still/not need D113736?)

D113741 includes the test cases for this patch so if we take D113741 (and I think we should) then I will drop this patch.

OK, I'll set this aside for now - perhaps you can mark it as "abandoned" (or wait until D113741 is committed before doing that)

ellis abandoned this revision.Dec 1 2021, 11:36 AM

These test cases were added to D113741