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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
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:
> 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.
Oh! Indeed!

    gcc -g1 -fno-eliminate-unused-debug-types foo.c -c -o foo.o

produces no additional debug info. `-g2` does.  Let me fix that and add tests.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242