[PATCH] D115325: [DWARF] Fix PR51087 Extraneous enum record in DWARF with type units

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D115325#3194733 <https://reviews.llvm.org/D115325#3194733>, @Orlando wrote:

> Thanks for that info on retained types.
>
> In D115325#3190794 <https://reviews.llvm.org/D115325#3190794>, @dblaikie wrote:
>
>> We probably should only put user defined types in the retained types list - but that would require decomposing non-user-defined types in the frontend (eg: if you have a "void (*)(T1, T2)" type retained - we only need to retain T1 and T2, not the function pointer type wrapped around it (it's reasonable to expect a DWARF consumer to be able to build any non-user-defined type - even ones that that never appeared in the source program (so maybe a user ends up needing "T1 (*)(T2)" in their debugging context, the DWARF consumer would be able to create that - there's no need for us to encode that type or any other, so long as we provide T1 and T2)
>
> Ok great, I'll file a bug for this shortly.
>
>> But I guess for now we could apply the same logic to enums and to retained types, with one minor change to use dyn_cast instead of cast, in the retained types version - so if it is a top level user defined type in the retained types list, it'd skip the skeleton type DIE in that case. In other cases it'd still end up with the unnecessary poinetr type, function type, whatever.
>
> I made this change (does this still look okay to land?) and I wanted to add a test for the retained types too so I wrote this:
>
>   $ cat test.cpp
>   struct Int {
>     int i;
>     Int() : i(0) {}
>     operator int() const { return i; }
>   };
>   int f() { return Int(); }
>
> Struct `Int` is indeed a retained type but even with this latest patch `Int` gets a CU DIE:
>
>   0x0000003b:   DW_TAG_structure_type
>                   DW_AT_declaration	(true)
>                   DW_AT_signature	(0x9366bdce481c7390)
>
> Which is only referenced from within its children, e.g. for the overloaded int conversion:
>
>   0x0000003b:   DW_TAG_structure_type
>                   DW_AT_declaration	(true)
>                   DW_AT_signature	(0x9366bdce481c7390)
>   <snip>
>   0x0000008c:   DW_TAG_subprogram
>                   DW_AT_low_pc	(0x0000000000000000)
>                   DW_AT_high_pc	(0x0000000000000010)
>                   DW_AT_frame_base	(DW_OP_reg6 RBP)
>                   DW_AT_object_pointer	(0x0000009c)
>                   DW_AT_specification	(0x0000004e "_ZNK3IntcviEv")
>   
>   0x0000009c:     DW_TAG_formal_parameter
>                     DW_AT_location	(DW_OP_fbreg -8)
>                     DW_AT_name	("this")
>                     DW_AT_type	(0x000000ab "const Int *") # <-- References Int through DW_TAG_pointer_type(DW_TAG_const_type(Int))
>                     DW_AT_artificial	(true)
>   <snip>
>   0x00000087:   DW_TAG_const_type
>                   DW_AT_type	(0x0000003b "Int")
>   <snip>
>   0x000000ab:   DW_TAG_pointer_type
>                   DW_AT_type	(0x00000087 "const Int")
>
> These children that are included in the CU (above) are also duplicated in the TU (i.e. we get a full `DW_TAG_subprogram` DIE in both places). This sounds like a bug, or at least that my implementation needs further work to cover all cases for retained types.
>
> I couldn't generate a test for retained types from source by hand or reduction (from CTMark and clang-3.4) that showed this patch working in action.
>
> I'm happy to land either version of this patch - if we go with it in its current state I can file a(nother) bug for improving the retained types situation.
>
> EDIT: Fixed dwarfdump example.

So in this example the result is unchanged despite the code change to the retained types handling? (or it does change, but not enough?)

Would a simpler struct (`struct x { }; x y;`) show a positive change (remove the skeleton type DIE from the CU)? That'd be enough of an incentive now. Further value might need follow-up patches.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1455-1461
+      if (DICompositeType *CTy = dyn_cast<DICompositeType>(Ty)) {
+        if (generateTypeUnits() && !CTy->isForwardDecl()) {
+          MDString *TypeId = CTy->getRawIdentifier();
+          if (TypeId && getOrCreateDwarfTypeUnit(*CU, TypeId->getString(), CTy))
+            continue; // Type unit added successfully.
+        }
+      }
----------------
This could be pulled into a function or lambda and reused across the enum and retained types handling? (either just using the dyn_cast version for both, or keeping the dyn_cast/cast distinction between the two and everything inside/after that could be shared - I'd probably just keep the dyn_cast version and call that from both places. It could return a bool to indicate whether the thing has been handled or not.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115325/new/

https://reviews.llvm.org/D115325