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

Thu Jul 16 11:36:27 PDT 2020

nickdesaulniers 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())
----------------
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?

================
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;
----------------
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?

================
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"
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242