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

Mon Jun 15 13:18:24 PDT 2020

dblaikie added a comment.

> - fix docs (use declare consistently, avoid define)

Defined would be the right word here (declaring a type is code like "struct foo;" - and this change probably doesn't/shouldn't emit debug info for type declarations, yes?)

================
Comment at: clang/include/clang/Driver/Options.td:1490-1492
+def fno_eliminate_unused_debug_types : Flag<["-"],
+  "fno-eliminate-unused-debug-types">, Group<f_Group> , Flags<[CC1Option]>,
+  HelpText<"Emit debug info for types that may be unused.">;
----------------
I /think/ @MaskRay added some kind of helper to make it easier to define the f/fno pair together, perhaps? Ah, right, mentioned in another part of this review, D80883 - though that'll only be applicable if this doesn't become a debug-info-kind.

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5559
+  case Decl::Typedef:
+    if (getCodeGenOpts().DebugUnusedTypes)
+      if (CGDebugInfo *DI = getModuleDebugInfo())
----------------
dblaikie wrote:
> nickdesaulniers wrote:
> > dblaikie wrote:
> > > Probably test this within the implementation of CGDebugInfo? & rename the EmitTypedef function to something that clarifies that it's for an otherwise unused type?
> > > 
> > > But that function might need to be generalized further, rather than only having it for typedefs. (see general comment above)
> > I think it makes sense at this point to rename `EmitExplicitCastType` to `RetainType` or `EmitType`.  WDYT?
> Yep - now that I understand the confusingly named hasReducedDebugInfo
> 
> "RetainType" sounds good to me, maybe "EmitAndRetainType" might be worthwhile for the extra clarity/symmetry with other "Emit" functions.
Ping on this ^

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5365-5369
+      if (getCodeGenOpts().DebugUnusedTypes && CRD->hasDefinition())
+        DebugInfo->completeUnusedClass(*CRD);
+      else if (auto *ES = D->getASTContext().getExternalSource())
         if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never)
+          DebugInfo->completeUnusedClass(*CRD);
----------------
dblaikie wrote:
> Perhaps this could be modified to use the same general purpose utility like the other type emissions (as speculated in another comment "EmitUnusedType" or the like)
Ping on this ^

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5565
+    if (CGDebugInfo *DI = getModuleDebugInfo())
+      if (getCodeGenOpts().DebugUnusedTypes) {
+        QualType QT = getContext().getTypedefType(cast<TypedefNameDecl>(D));
----------------
dblaikie wrote:
> nickdesaulniers wrote:
> > dblaikie wrote:
> > > Rather than testing DebugUnusedType for every call - might be best to add a "EmitUnusedType" function that tests the flag and does the emission? (same way EmitExplicitCastType already checks for a certain level of debug info before emitting)
> > It can be; what I don't like about that approach is that we have to determine the `QualType` here to pass in, at which point such function might just return without doing anything. (ie. we have to do slightly more work in the case where the debugging of unused types was *not* requested).  But that's a minor nit and we can live with it?
> I don't believe getting the QualType from a TypeDecl is an expensive operation/enough to try to avoid it here. If it is I'd rather have a "EmitUnusedType(TypeDecl*)" function, and then the conditional QualType retrieval could be in there, written once rather than several times. 
Ping on this^

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:768
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
+  Opts.DebugUnusedTypes = Args.hasArg(OPT_eliminate_unused_debug_types_fno);
   Opts.EmbedSource = Args.hasArg(OPT_gembed_source);
----------------
nickdesaulniers wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > dblaikie wrote:
> > > > > > Could this be rolled into the debug-info-kind? (a kind beyond "standalone")
> > > > > That sounds like a good idea.  Looking at the definition of `DebugInfoKind` (llvm/llvm-project/clang/include/clang/Basic/DebugInfoOptions.h), it seems that `DebugInfoKind` is an `enum` that defines a "level" of debug info to emit? Looking at the guard in `CGDebugInfo::EmitExplicitCastType` (llvm/llvm-project/clang/lib/CodeGen/CGDebugInfo.cpp), it calls `CodeGenOptions::hasReducedDebugInfo()` which does a comparison against a certain level.  That seems to indicate the order of the enumerations is important.  Do you have a suggestion what order I should add the new enum?
> > > > > 
> > > > > I'm guessing after `LimitedDebugInfo` or `DebugInfoConstructor`, but before `FullDebugInfo`? (I find the name of the method `hasReducedDebugInfo` confusing in that regard).
> > > > Ok, so I have a diff that implements this approach.  I feel like I should maybe publish it as a child commit to this one, to be able to more easily discuss it?
> > > > 
> > > > Two problems I run into:
> > > > 1. as alluded to in my previous response in this thread, `DebugInfoKind` is an enum that specifies a level.  It tends to "drag" other debug flags in based on the ordering.  Looking at extending the `switch` in `CGDebugInfo::CreateCompileUnit` (clang/lib/CodeGen/CGDebugInfo.cpp), it's not at all clear to me which we existing case should we choose?
> > > > 2. we want the flag `-fno-eliminate-unused-debug-types` to match GCC for compatibility.  We can additionally add a new debug info kind like `"standalone"` (clang/lib/Frontend/CompilerInvocation.cpp), but it's not clear how the two flags together should interact.
> > > > 
> > > > The suggestion for a new debug info kind feels like a recommendation to add a new "level" of debug info, but `-fno-eliminate-unused-debug-types` feels like it should be mutually exclusive of debug info kind? (I guess GCC does *require* `-g` with `-fno-eliminate-unused-debug-types`)
> > > > 
> > > > @dblaikie maybe you have more recommendations on this?
> > > This value would probably go "after" "full" (full isn't full enough, as you've found - you need a mode that's even "fullerer")
> > > 
> > > Perhaps renaming "Full" to "Referenced" and then introducing this new kind as the new "Full" or maybe under a somewhat different name to avoid ambiguity. 
> > > 
> > > Any suggestions on a less confusing name for "hasReducedDebugInfo"? (I think it was basically "Has less than full debug info"... but, yep, it doesn't do that at all - it's "HasTypeAndVariableDebugInfo" by the looks of it/by the comment)
> > > Ok, so I have a diff that implements this approach. I feel like I should maybe publish it as a child commit to this one, to be able to more easily discuss it?
> > > 
> > > Two problems I run into:
> > > 
> > > 1. as alluded to in my previous response in this thread, DebugInfoKind is an enum that specifies a level. It tends to "drag" other debug flags in based on the ordering. Looking at extending the switch in CGDebugInfo::CreateCompileUnit (clang/lib/CodeGen/CGDebugInfo.cpp), it's not at all clear to me which we existing case should we choose?
> > > 2. we want the flag -fno-eliminate-unused-debug-types to match GCC for compatibility. We can additionally add a new debug info kind like "standalone" (clang/lib/Frontend/CompilerInvocation.cpp), but it's not clear how the two flags together should interact.
> > 
> > I'm not suggesting adding a new driver-level flag for this, but implementing the GCC flag name in terms of the debug-info-kind. Probably by having "-fno-eliminate-unused-debug-types" override "-fstandalone-debug" - whichever comes later wins. Because they are part of a progression. Though admittedly that might get a smidge confusing about exactly whit no/yes versions of these two flags override each other - but I think that's inevitable confusion with the nature of these flags.
> > 
> > What does GCC do for its -f[no-]emit-class-debug-always (which is somewhat similar to -fstandalone-debug) V -f[no-]eliminate-unused-debug-types? I'm not sure we'll want to emulate the behavior exxactly, but it's probably a good place to start to see if there's an existing model that looks OK here.
> > 
> > > The suggestion for a new debug info kind feels like a recommendation to add a new "level" of debug info, but -fno-eliminate-unused-debug-types feels like it should be mutually exclusive of debug info kind? (I guess GCC does *require* -g with -fno-eliminate-unused-debug-types)
> > 
> > Sorry, I'm not following here - perhaps you could explain in different words? At the driver/user level, "debug info kind" doesn't exist - there's flags like "-g", "-gmlt", "-fstandalone-debug", etc. That are mapped to debug-info-kind on the -cc1 command line. I'm suggesting "-fno-eliminate-unused-debug-types" is another of those driver flags that is taken into account when mapping down to the cc1 "debug info kind".
> Are debug info kinds a progression or mutually exclusive?  I think they're a progression, but I have concerns with modeling `-fno-eliminate-unused-debug-types` as part of that progression.
> 
> I don't think we should consider `-fno-eliminate-unused-debug-types` to be part of the progression of debug info levels via debug info kind.  Users must specify `-g` in addition to `-fno-eliminate-unused-debug-types` to get any debug info, at which point they'll get both full debug info via `-g` and unused type debug info via `-fno-eliminate-unused-debug-types`.
> 
> To me, `-fno-eliminate-unused-debug-types` and `-fno-eliminate-unused-debug-symbols` seem orthogonal to debug info level.  For example, how would you order `-fno-eliminate-unused-debug-types` vs `-fno-eliminate-unused-debug-symbols` in a progression?  One does not imply the other; they are mutually exclusive.
> 
> I agree in a progression `-fno-eliminate-unused-debug-types` should come after full debug info, but then it seems weird for `-fno-eliminate-unused-debug-symbols` if we ever wanted to add support for that (YAGNI?) So maybe a progression isn't the best representation?
> 
> ---
> I should probably fix the documentation in this commit for -fstandalone-debug to mention this, and add documentation for this flag in the first place.
> 
> ---
> Regarding `-femit-class-debug-always` g++ with `-femit-class-debug-always` for the test case in this CL doesn't produce any debug info.  Looks like clang does not currently support `-femit-class-debug-always`.
> Are debug info kinds a progression or mutually exclusive?

Both? But yes, generally considered a progression.

> I don't think we should consider -fno-eliminate-unused-debug-types to be part of the progression of debug info levels via debug info kind. Users must specify -g in addition to -fno-eliminate-unused-debug-types to get any debug info, at which point they'll get both full debug info via -g and unused type debug info via -fno-eliminate-unused-debug-types.

The same is true of the existing -fstandalone-debug (similar to GCC's -femit-class-debug-always)

> To me, -fno-eliminate-unused-debug-types and -fno-eliminate-unused-debug-symbols seem orthogonal to debug info level. For example, how would you order -fno-eliminate-unused-debug-types vs -fno-eliminate-unused-debug-symbols in a progression? One does not imply the other; they are mutually exclusive.

I'd probably implement -fno-eliminate-unused-debug-symbols as a separate thing, not in that progression & would be happy to design the current feature without considering that future possibility in any case.

> I should probably fix the documentation in this commit for -fstandalone-debug to mention this, and add documentation for this flag in the first place.

Mention what, sorry? 

> Regarding -femit-class-debug-always g++ with -femit-class-debug-always for the test case in this CL doesn't produce any debug info. 

Sorry, more specifically I meant "-g -femit-class-debug-always" and "-g -fno-eliminate-unused-debug-types" and I meant on more than only this test case.

Basically: how do those two flags interact in GCC? Are they both considered separately (in the no-X and X variants) and then the "more verbose" of the two wins (ie: -femit-class-debug-always -fno-eliminate-unused-debug-types -feliminate-unused-debug-types still produces "Class debug always" behavior?)?

> Looks like clang does not currently support -femit-class-debug-always.

================
Comment at: clang/test/CodeGen/debug-info-unused-types.c:20-29
+// CHECK: !3 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "bar", file: !4, line: 7, baseType: !5, size: 32, elements: !6)
+// CHECK: !7 = !DIEnumerator(name: "BAR", value: 0, isUnsigned: true)
+// CHECK: !8 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "z", scope: !9, file: !4, line: 13, baseType: !5, size: 32, elements: !13)
+// CHECK: !14 = !DIEnumerator(name: "Z", value: 0, isUnsigned: true)
+// CHECK: !16 = !DIDerivedType(tag: DW_TAG_typedef, name: "my_int", file: !4, line: 5, baseType: !17)
+// CHECK: !17 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+// CHECK: !18 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo", file: !4, line: 6, elements: !12)
----------------
Might be a smidge easier to read without the enumerators (& this doesn't check the enumerators are in the "elements" list of the enumeration anyway) - presumably the type emission codepath is well enough tested that we only need to to briefly test that the types were emitted at all? (might even be easy enough to stop after the tag and name))

================
Comment at: clang/test/CodeGen/debug-info-unused-types.c:35-44
+// NODBG-NOT: !3 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "bar", file: !4, line: 7, baseType: !5, size: 32, elements: !6)
+// NODBG-NOT: !7 = !DIEnumerator(name: "BAR", value: 0, isUnsigned: true)
+// NODBG-NOT: !8 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "z", scope: !9, file: !4, line: 13, baseType: !5, size: 32, elements: !13)
+// NODBG-NOT: !14 = !DIEnumerator(name: "Z", value: 0, isUnsigned: true)
+// NODBG-NOT: !16 = !DIDerivedType(tag: DW_TAG_typedef, name: "my_int", file: !4, line: 5, baseType: !17)
+// NODBG-NOT: !17 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+// NODBG-NOT: !18 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo", file: !4, line: 6, elements: !12)
----------------
Could this be simplified down to:
```
NODBG-NOT: DICompositeType
```
?

================
Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:15-29
+// CHECK: !3 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "baz", file: !4, line: 7, baseType: !5, size: 32, flags: DIFlagEnumClass, elements: !6, identifier: "_ZTS3baz")
+// CHECK: !7 = !DIEnumerator(name: "BAZ", value: 0)
+// CHECK: !8 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "z", scope: !9, file: !4, line: 12, baseType: !5, size: 32, flags: DIFlagEnumClass, elements: !13)
+// CHECK: !14 = !DIEnumerator(name: "Z", value: 0)
+// CHECK: !16 = !DIDerivedType(tag: DW_TAG_typedef, name: "foo", file: !4, line: 5, baseType: !5)
+// CHECK: !17 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "bar", file: !4, line: 6, size: 8, flags: DIFlagTypePassByValue, elements: !12, identifier: "_ZTS3bar")
+// CHECK: !18 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "y", scope: !9, file: !4, line: 11, size: 8, flags: DIFlagTypePassByValue, elements: !12)
----------------
Similar feedback as for the C example above

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242