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

Tue Jun 2 13:12:25 PDT 2020

nickdesaulniers added inline comments.

================
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:
> 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`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80242