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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3842-3846
+  if (EmitDwarf &&
+      Args.hasFlag(options::OPT_fno_eliminate_unused_debug_types,
+                   options::OPT_feliminate_unused_debug_types, false) &&
+      DebugInfoKind >= codegenoptions::DebugInfoConstructor)
+    DebugInfoKind = codegenoptions::UnusedTypeInfo;
----------------
Would this be tidier if it were rolled into the if/checking around 380 since it's a very similar option?


================
Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:24-30
+// 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"
----------------
Maybe simpler to test the NODBG by `NODBG-NOT: DI{{[a-zA-Z]*}}Type` ? Not sure if that'd quite work, but might be adequate.


================
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:
> > 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.
> So one issue I'm running into with this approach is that the retained type list is emitted in between the other debug nodes (some before it, some after it).  The capture logic seems to have an issue referring to captures that have yet to be defined yet.
> 
>     note: uses undefined variable(s): "TYPE7"
> 
> but if I move the `CHECK` for the retained types last, then `FileCheck` errors because it seems it only doesn't one pass for the `CHECK` statements (IIUC), so it's "too late" to parse.
> 
> Is there another way to test this, or am I holding it wrong?
Order dependence is a currently unavoidable part of FileCheck tests, so if the content you're trying to test (I'm not entirely clear from your description - so guessing a bit) looks something like:
```
!1 = DICompileUnit ... retainedTypes: !3
!2 = DICompositeType "x"
!3 = !{!2, !4}
!4 = DICompositeType "y"
```
Then you'd CHECK this something like:
```
; CHECK: DICompileUnit {{.*}} retainedTypes: [[RET_TY:![0-9]*]]
; CHECK: [[X:![0-9]*]] = DICompositeType "x"
; CHECK: [[RET_TY]] = !{[[X]], [[Y:![0-9]*]]}
; CHECK: [[Y]] = DICompositeType "y"
```
Yes, this does mean that if the types were emitted in a different order, the test would fail - but that's a limitation we're living with for pretty much all LLVM and Clang testing at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242