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

Sat May 30 12:43:09 PDT 2020

dblaikie added inline comments.

================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5565
+    if (CGDebugInfo *DI = getModuleDebugInfo())
+      if (getCodeGenOpts().DebugUnusedTypes) {
+        QualType QT = getContext().getTypedefType(cast<TypedefNameDecl>(D));
----------------
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. 

================
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);
----------------
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".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242