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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
