[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:103-118
+  case Decl::CXXRecord: // struct/union/class X; [C++]
+    if (CGDebugInfo *DI = getDebugInfo())
+      if (CGM.getCodeGenOpts().hasMaybeUnusedDebugInfo() &&
+          cast<CXXRecordDecl>(D).hasDefinition())
+        DI->completeUnusedClass(cast<CXXRecordDecl>(D));
+    return;
   case Decl::Record:    // struct/union/class X;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > dblaikie wrote:
> > > All of these might be able to be collapsed together - using "cast<TagDecl>(D).hasDefinition()" ... to have a common implementation.
> > `TagDecl` doesn't have a method `hasDefinition`.  Only `CXXRecordDecl` does.
> Also, how to get the `QualType` differs between `Decl::Enum` an `Decl::Record`; `getEnumType` vs `getRecordType` respectively.
Looks like ASTContext::getTypeDeclType could be used, if it makes things especially tidier. But perhaps the TagDecl/hasDefinition difference is enough for it not to be especially significant.


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:116
+    if (CGDebugInfo *DI = getDebugInfo())
+      if (cast<EnumDecl>(&D)->getDefinition())
+        DI->EmitAndRetainType(getContext().getEnumType(cast<EnumDecl>(&D)));
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > I think the right thing to call here (& the other places in this patch) is "hasDefinition" rather than "getDefinition" (this will avoid the definition being deserialized when it's not needed yet (then if we don't end up emitting the type because the flag hasn't been given, it won't be deserialized unnecessarily))
> `TagDecl` doesn't have a method `hasDefinition`.  Only `CXXRecordDecl` does.
Ah, fair enough!


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5385
+      if (getCodeGenOpts().hasMaybeUnusedDebugInfo() && CRD->hasDefinition())
+        DI->completeUnusedClass(*CRD);
+      else if (auto *ES = D->getASTContext().getExternalSource())
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > I think this might not work as intended if the type trips over the other class deduplication optimizations (try manually testing with a type that has an external VTable?) - and perhaps this codepath should use the EmitAndRetain functionality.
> completeUnusedClass eventually calls into CGDebugInfo::completeClass which makes use of the TypeCache.  What does it mean for a class to have "an external vtable?"  Can you give me an example?
Here's some sample code that uses a type in the DWARF but due to the vtable being external (an external vtable is one that's guaranteed to be emitted in some other object file - due to the key function optimization in the Itanium ABI - since all virtual functions must be defined if a type is ODR used, the compiler can make a shared assumption that the vtable will only be emitted with a single consistently chosen virtual function (ideally: the first non-inline one) and emit the vtable only in that object and nowhere else) the type is emitted as a declaration (when using "-fno-standalone-debug"):

```
struct t1 {
  virtual void f1();
};
t1 v1;
```


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3825-3828
+  if (EmitDwarf &&
+      Args.hasFlag(options::OPT_fno_eliminate_unused_debug_types,
+                   options::OPT_feliminate_unused_debug_types, false))
+    DebugInfoKind = codegenoptions::UnusedTypeInfo;
----------------
How does GCC compose this with -gmlt, for instance? I'd suspect that the desired behavior might be for -gmlt to override this -f flag? So maybe this should be predicated on "if (EmitDwarf && DebugInfoKind >= codegenoptions::LimitedDebugInfo && ... "?

(so if someone wants to use only -gmlt in certain parts of their build that doesn't get upgraded by the presence of this -f flag)


================
Comment at: clang/test/CodeGen/debug-info-unused-types.c:46-60
+struct aaaaaaaaaaaa;
+enum bbbbbbbbbbbb;
+union cccccccccccc;
+void b0(void) {
+  struct dddddddddddd;
+  enum eeeeeeeeeeee;
+  union ffffffffffff;
----------------
Maybe give these more specific names (similarly in the other test) - and/or you could use a pattern in the naming that might make the -NOT lines easier?

eg:

```
struct decl_struct;
enum decl_enum;
...
// NODBG-NOT: name: "decl_
```

(also best to include a bit more context in the -NOT checks, otherwise there's risk that a users build directory (even with these longer names) or other paths might accidentally fail the checks)


================
Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:15-21
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "baz"
+// CHECK: !DIEnumerator(name: "BAZ"
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "z"
+// CHECK: !DIEnumerator(name: "Z"
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "foo"
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "y"
----------------
Probably good to check these end up in the retained types list


================
Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:23-29
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_enumeration_type, name: "baz"
+// NODBG-NOT: !DIEnumerator(name: "BAZ"
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_enumeration_type, name: "z"
+// NODBG-NOT: !DIEnumerator(name: "Z"
+// NODBG-NOT: !DIDerivedType(tag: DW_TAG_typedef, name: "foo"
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_class_type, name: "bar"
+// NODBG-NOT: !DICompositeType(tag: DW_TAG_class_type, name: "y"
----------------
With a uniform naming scheme, perhaps this could be `// NODBG-NOT: name: "unused_` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242