[CloneFunction][DebugInfo] Clone DISubprogram's local types

[dzhidzhoev]

Summary

When DISubprogram is being cloned, types belonging to its scope
must be cloned too. Otherwise, function-local types of the original
function are used in the cloned one, which may cause conflicts.

It helps to tackle the crash encountered after committing
https://reviews.llvm.org/D144006.

[dblaikie]

Additionally, when tracking function-local types in the DICompileUnit (via retainedTypes or enums fields), it seems reasonable to track the clones as well.

Hmm, when do we have function-local types in the DICompileUnit list? I thought we always put those on the subprogram now?

llvm/lib/Transforms/Utils/CloneFunction.cpp

299–307

maybe turn this into a lambda or helper function - since it's 10 lines that's repeated twice, by the looks of it? (can pass in the list to push back to so one pushes back to the enums, and the other pushes back to the types)

311–318

Maybe this could be streamlined a bit too - maybe doesn't saves lines, but makes it easier to read?

[dzhidzhoev]

In D155818#4529566, @dblaikie wrote:

Additionally, when tracking function-local types in the DICompileUnit (via retainedTypes or enums fields), it seems reasonable to track the clones as well.

Hmm, when do we have function-local types in the DICompileUnit list? I thought we always put those on the subprogram now?

The commit with this change https://reviews.llvm.org/D144006 was reverted because of the crash I'm trying to fix here. Since I will commit this before https://reviews.llvm.org/D144006, if it gets accepted xD, we technically still have function-local types in the DICompileUnit list.

[dblaikie]

In D155818#4531577, @dzhidzhoev wrote:

In D155818#4529566, @dblaikie wrote:

Additionally, when tracking function-local types in the DICompileUnit (via retainedTypes or enums fields), it seems reasonable to track the clones as well.

Hmm, when do we have function-local types in the DICompileUnit list? I thought we always put those on the subprogram now?

The commit with this change https://reviews.llvm.org/D144006 was reverted because of the crash I'm trying to fix here. Since I will commit this before https://reviews.llvm.org/D144006, if it gets accepted xD, we technically still have function-local types in the DICompileUnit list.

So this code would be removed once D144006 goes in? Perhaps we could review them separately but assuming they go in together (they could be squashed together before committing) if it's causing this much extra code that'd only exist for 1-2 commits anyway?

[dzhidzhoev]

Addressed @dblaikie comments. This PR is supposed to be commited
along with https://reviews.llvm.org/D144006.

[dzhidzhoev]

Throw away excessive dump() calls from test.

[dblaikie]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

I guess this comes up, though I'd love it if it doesn't...

When does this situation arise? I would've hoped that function-local types wouldn't appear in the CU's retained types list. It'd simplify the model if they were only retained by either the CU or the subprogram, and not both.

[dzhidzhoev]

This comment was removed by dzhidzhoev.

[dzhidzhoev]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

enum my_enum { AA, AB, AC } e;
int main() {
  enum my_local_enum { A, B, C };
  return 0;
}

This example can be compiled with the flag -fno-eliminate-unused-debug-types :

bin/clang local-retained.c -c -fno-eliminate-unused-debug-types -O0 -g -emit-llvm

That's what metadata looks like:

!2 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, retainedTypes: !11 ...
!5 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "my_enum", ...
!11 = !{!5, !12}
!12 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "my_local_enum", scope: !13, ...
!13 = distinct !DISubprogram(name: "main", retainedNodes: !17, ...
!17 = !{!12}

Here my_local_enum, which is local to the subprogram, is referenced by both CU and Subprogram, via retainedTypes and retainedNodes correspondingly. I haven't encountered such a situation in real-world metadata since I haven't got much experience dealing with DebugInfo.

[dzhidzhoev]

Rebased.

[dexonsmith]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

I agree with @dblaikie that local types seem like a property of subprograms.

To replicate the current IRGen retention policy, we could make -fno-eliminate-unused-debug-types retain any subprogram that has a local type (dropping the explicit reference to the local types from compile-unit) and get the same effect.

But maybe -fno-eliminate-unused-debug-types is overreaching here? It's not obvious to me that it implies retaining function-local types. Feels like -fno-eliminate-unused-debug-subprograms (or similar) would be a better fit for that behaviour.

Either way, then we'd end up with a clean model here and this could wouldn't be necessary.

[dblaikie]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

Yeah - @nickdesaulniers do you care about local types for optimized-out functions in -fno-eliminate-unused-debug-types? Looks like GCC is maybe slightly inconsistent. For a totally uncalled inline function with a local type, it doesn't generate any DWARF, but if it's called and then optimized away, it still manages to emit a DW_TAG_inlined_subroutine (though it seems to do this even without the local type - so the local type isn't what's causing the inlined subroutine description to be emitted)

I think it's fine to, yeah, change -fno-eliminate-unused-debug-types to attach local types to the subprogram - and if at some point we improve inlining (I'm not sure it's an improvement to include a DW_TAG_inlined_subroutine that describes a zero length range of instructions, though... ) then we'll keep these types too.

[nickdesaulniers]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

do you care about local types for optimized-out functions in -fno-eliminate-unused-debug-types?

I assume the answer is "yes" but let me check with the consumers of this data (in Android in the UK).

[nickdesaulniers]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

Direct feedback:

I'm not sure what GCC's intent is. When I experimented with this (and the vaguely related -fstandalone-debug) it looked like that instead of doing some reachability analysis it was just emitting everything.
In general we are completely uninterested in types that are defined wholly within functions (at any level of nesting if allowing nested functions) as the language semantics imply they cannot leak out.
There is the edge case of types defined as part of function prototypes. Whatever their actual usefulness, it makes sense for them to be considered "used" if the function itself is.
The only bug we've seen with Clang is when there is a DIE outside a function referring to a type defined in a function.
From the perspective of knowing "all types', I'd say it makes sense to emit them, if there's a DWARF context that is meaningful for them (like an unused out of line function definition or something).
From ABI perspective, I don't think we care.

[dexonsmith]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

@aprantl, do your users use -fno-eliminate-unused-debug-types? If so, is the proposed change fine for them too?

[aprantl]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

On Darwin, the flag is exposed via the -gfull option, however, I am not aware that this feature is being (widely) used at all.

[dzhidzhoev]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

I think it's fine to, yeah, change -fno-eliminate-unused-debug-types to attach local types to the subprogram - and if at some point we improve inlining (I'm not sure it's an improvement to include a DW_TAG_inlined_subroutine that describes a zero length range of instructions, though... ) then we'll keep these types too.

Are there any obstacles to committing that change together with https://reviews.llvm.org/D144006? Since I'm not sure about the consistency of changing this behavior while having other function-local types tracked by DICompileUnit's enum field.

[dzhidzhoev]

llvm/lib/Transforms/Utils/CloneFunction.cpp

281–301

I think it's fine to, yeah, change -fno-eliminate-unused-debug-types to attach local types to the subprogram - and if at some point we improve inlining (I'm not sure it's an improvement to include a DW_TAG_inlined_subroutine that describes a zero length range of instructions, though... ) then we'll keep these types too.

Is it meant to look like this https://reviews.llvm.org/D158730?

[dzhidzhoev]

Friendly ping

[dzhidzhoev]

Rebased & dropped redundant code for cloning local types referenced by DICompileUnit.

[dzhidzhoev]

Gentle ping

[dblaikie]

Is this comment in the patch description (and any associated actual functionality in the patch) still relevant, now we're planning to/have moved local types out of the retained types list?

Additionally, when tracking function-local types in the DICompileUnit (via retainedTypes fields), it seems reasonable to track the clones as well.

[dzhidzhoev]

In D155818#4647515, @dblaikie wrote:

Is this comment in the patch description (and any associated actual functionality in the patch) still relevant, now we're planning to/have moved local types out of the retained types list?

Additionally, when tracking function-local types in the DICompileUnit (via retainedTypes fields), it seems reasonable to track the clones as well.

Thanks for the catch! No, it's not relevant anymore. The code processing retainedTypes of DICompileUnit has been dropped.

[dzhidzhoev]

In D155818#4647515, @dblaikie wrote:

Is this comment in the patch description (and any associated actual functionality in the patch) still relevant, now we're planning to/have moved local types out of the retained types list?

Additionally, when tracking function-local types in the DICompileUnit (via retainedTypes fields), it seems reasonable to track the clones as well.

Should this patch be reviewed for other potential issues? Because https://reviews.llvm.org/D158730 can't be merged without this one.

[dexonsmith]

LGTM.