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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Thanks for taking a look at this.

In D115325#3186306 <https://reviews.llvm.org/D115325#3186306>, @dblaikie wrote:

> under what conditions do we get this redundant pointer type in the CU you're referring to here?

Here's an example with target `x86_64-unknown-linux-gnu` using clang built at 51dc466642c5 <https://reviews.llvm.org/rG51dc466642c5566c289468b269a8c69b0e447720>.

  $ cat test.cpp
  struct O { struct S { int x; } y; };
  int f(void *p) {
    return static_cast<O::S*>(p)->x;
  }
  
  $ clang++ test.cpp -c -gdwarf-5 -fdebug-types-section -o - | llvm-dwarfdump - -o-
  <snip>
  0x0000003f:   DW_TAG_pointer_type
                  DW_AT_type	(0x0000004d "O::S")
  
  0x00000044:   DW_TAG_structure_type
                  DW_AT_declaration	(true)
                  DW_AT_signature	(0x9ff533511b48ced2)
  
  0x0000004d:     DW_TAG_structure_type
                    DW_AT_declaration	(true)
                    DW_AT_signature	(0x7b36f109e857d200)
  <snip>

DIE `0x0000003f` is not referenced in the CU. The CU DIE is generated because it's a "retained type" (in `CUNode->getRetainedTypes()`). It's still emitted if even when we apply the fix in this patch to retained types because `DW_TAG_pointer_type` types don't get type units (in `DwarfUnit::createTypeDIE(const DIEScope*, DIE&, const DIType*)`. So, my thinking was that consumers probably don't need the `DW_TAG_pointer_type` DIE to interpret pointers of the derived type; as long as we have the derived type DIE (in a type unit) then that should be enough? I don't have 100% confidence in that assumption though - what do you think?

I'm not sure how often this actually comes up in practice - I only noticed it when writing a test for applying this change to "retained types" too (the part that you have also highlighted with an inline comment). I am not sure of all the ways that a type becomes a "retained type"; the source above is inspired by `llvm/test/DebugInfo/COFF/retained-types.ll`. I decided to leave the retained types alone in this patch because I felt I didn't fully understand what causes them, what they look like "on average", and how many useless DIEs we really emit because of them. i.e. if retained types are always or mostly pointers then we'd need to apply the change described above (if it's possible) first to see any wins. Do you have any insight on this? Otherwise I can look into this further.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1454-1457
     for (auto *Ty : CUNode->getRetainedTypes())
       if (DIType *RT = dyn_cast<DIType>(Ty))
         // There is no point in force-emitting a forward declaration.
         CU->getOrCreateTypeDIE(RT);
----------------
dblaikie wrote:
> Should we do the same thing for non-enum types too?
I've responded to this in the main comment thread - SGTM but I have some questions about retained types (which I'm happy to investigate).


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

https://reviews.llvm.org/D115325