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

Fri Jul 17 13:07:36 PDT 2020

dblaikie added inline comments.

================
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:
> > 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;
> > ```
> Cool, thanks for the example.  For your example:
> `clang -g -emit-llvm -S foo.cpp -o -`:
> ```
> ...
> DIGlobalVariable(name: "v1"
> ...
> DICompositeType(tag: DW_TAG_structure_type, name: "t1"
> ...
> ```
> `clang -g -fno-eliminate-unused-debug-types -emit-llvm -S foo.cpp -o -`:
> ```
> ...
> DIGlobalVariable(name: "v1"
> ...
> DICompositeType(tag: DW_TAG_structure_type, name: "t1"
> ...
> DIDerivedType(tag: DW_TAG_member, name: "_vptr$t1"
> DIDerivedType(tag: DW_TAG_pointer_type
> DIDerivedType(tag: DW_TAG_pointer_type, name: "__vtbl_ptr_type"
> DIBasicType(name: "int"
> DISubprogram(name: "f1"
> DISubroutineType
> DIDerivedType(tag: DW_TAG_pointer_type
> ...
> ```
> So even though `f1` was not defined, we still emit debug info for it.
> 
> Comparing the output of `llvm-dwarfdump` on `$CC -g -fno-eliminate-unused-debug-types -c <example.cpp>` for `g++` vs `clang++`; we're definitely producing more debug info (the vtable info).
> 
> 
> 
> That said, the creation of `v1` technically means that `t1` is no longer an "unused type."  If we comment out its creation, then compare `$CC -g -fno-eliminate-unused-debug-types -c <example.cpp>` for `g++` vs `clang++`, `g++` produces no debug info (That seems like a bug in `g++` to me), while `clang++` with this patch produces the debug info for the `CXXRecord`, and additional info for the vtable ptr and signature of `f1`.
> 
> I'm not sure what's expected in this case (we should at least emit the `DW_TAG_structure_type` for `t1`, but I'm not sure about the `DW_TAG_subprogram` for `f1`.  I assume that's because we've enabled more-than-full-debuginfo behavior? WDYT?
I think it'd be nice to handle this the same as GCC (though technically we don't offer the same customization that GCC offers - I imagine the GCC behavior of -femit-class-debug-always, which Clang doesn't have a configuration option for (well, not specifically - again, comes back to that "independent configuration" or "overriding behavior" implementation choice) - I'd sort of think of it like that flag is always enabled)

But I guess if we think of it as a series of escalations, then, yeah, it makes sense in the Clang way of thinking of it, as -fno-eliminate-unused-debug-types as overriding the default -fno-emit-class-debug-always... *shrug* don't feel /super/ strongly either way I guess. If the current implementation is such that -fno-eliminate-unused-debug-types overrides all other type homing techniques (vtable based, explicit template specialization decl/def, ctor based, and not-required-to-be-complete (define a type but only use pouinters to that type and never dereference them)) that's fine by me.

================
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;
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > 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)
> GCC does not yet support `-gmlt`.
> https://godbolt.org/z/Wrv6sK
> 
> IIUC, `-gmlt` ("minimum line tables") is meant to minimize the amount of debug info produced.  I'm not sure what the user expects if they simultaneous ask for more debug info (via `-fno-eliminate-unused-types`) and less debug info (via `-gmlt`).
> 
> The current behavior is that `-fno-eliminate-unused-types` "wins" out over `-gmlt`, resulting in the "fullest" amount of debug info being produced.  (Would you like me to add tests for this behavior to clang/test/Driver/debug-options.c for this behavior?)
> 
> If we don't want that behavior, we should reconsider whether we want `UnusedTypeInfo` to be part of the `DebugInfoKind` "progression."  I also don't understand what the suggested added condition to the predicate is supposed to do; if `EmitDebugInfo` is set, doesn't the default value of `DebugInfoConstructor` (after 227db86a1b7dd6f96f7df14890fcd071bc4fe1f5, rebased this patch on top of) mean that we wouldn't use `UnusedTypeInfo` with `-g -fno-eliminate-unused-type-info?  Or am I misunderstanding the suggestion?
GCC's nearest equivalent is -g1 by the looks of it - I'd hazard a guess that -fno-eliminate-unused-types has no effect when compiling with -g1 (as -fstandalone-debug has no effect when -gmlt is specified)

I thin it's probably appropriate to implement similar behavior in Clang - that -fstandalone-debug/no-standalone-debug/no-eliminate-unused-types only applies to users that already asked for some kind of class debug info. It looks like that functionality's already implemented correctly (at least in one or two small cases I tested) for -fstandalone-debug and -fno-eliminate-unused-types should follow that.

What should happen between -fstandalone-debug and -fno-eliminate-unused-types is an open question I don't feel too strongly about which order they work in.

================
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"
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > Probably good to check these end up in the retained types list
> What's the best way to check this?  In particular, I worry about the `!<number>` number changing in the future, resulting in this test spuriously failing.  For this test case, we have:
> 
> ```
> !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "Nick Desaulniers clang version 12.0.0 (git at github.com:llvm/llvm-project.git e541558d11ca82b3664b907b350da5187f23c0c9)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !15, splitDebugInlining: false, nameTableKind: None)
> ```
> which tells us the `retainedTypes` is `!15`. `!15` looks like:
> ```
> !15 = !{!16, !17, !3, !5, !18, !8}
> ```
> In this case, should I just add the numbers?  If we used regex patterns `[0-9]`, I worry that we couldn't ensure the list would match.  Is it ok to just hardcode these?
You can use captures in FileCheck to make references a little more flexible. Surprised there aren't /any/ tests in LLVM testing the retainedTypes list... test/CodeGen/debug-info-extern-call.c used to use the retained types list & still has some testing remnants of it you could draw from, something like::
```
// CHECK: !DICompileUnit({{.*}}retainedTypes: [[RETTYPES:![0-9]+]]
// CHECK: [[RETTYPES]] = !{[[T1:![0-9]+]], [[T2:![0-9]+]], ...}
...
// CHECK: [[T1]] = !DICompositeType(...
```

It doesn't protect the test from changes in the order of the retained types list but otherwise resilient to extraneous metadata nodes being added or removed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242