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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5559
+  case Decl::Typedef:
+    if (getCodeGenOpts().DebugUnusedTypes)
+      if (CGDebugInfo *DI = getModuleDebugInfo())
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242