dblaikie added a comment.
In D72427#1812954 <https://reviews.llvm.org/D72427#1812954>, @akhuang wrote:
> > What's the plan for this? Is it still in an experimental stage, with the
> > intent to investigate the types that are no longer emitted unedr the flag &
> > explain why they're missing (& either have a justification for why that's
> > acceptable, or work on additional heuristics to address the gaps?)?
> >
> > If so, I'd probably rather this not be a full driver flag - if it's a
> > reliable way to reduce debug info size (like the existing heuristics under
> > -fstandalone-debug*) it should be rolled into -fno-standalone-debug
> > behavior, and if it's not fully fleshed out yet, I think an -Xclang flag
> > would be more suitable for the experimental phase.
>
> Pretty much, I think the plan is to investigate further and maybe have more
> people try it. The -Xclang flag seems reasonable. Do you have thoughts on
> whether the added DebugInfoKind level makes sense?
Yeah, that seems reasonable - though once this mode has been
vetted/stabilized/ironed out, hopefully that can be removed and the
functionality will be rolled into LimitedDebugInfo.
================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:360-364
+
+ /// Check if full debug info should be emitted.
+ bool isFullDebug() const {
+ return getDebugInfo() >= codegenoptions::DebugInfoConstructor;
+ }
----------------
Could you commit (the pre-patch equivalent) of this change now/before your
patch - it'll simplify the diff/make it easier to review.
Though I think the name needs some massaging, since it doesn't return
"getDebugInfo() == codegenoptions::FullDebugInfo", so the name's a bit
misleading.
/maybe/ (but honestly I don't have any great names)
"hasVariableAndTypeDebugInfo" though that's a bit of a mouthful.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2225
+ !CXXDecl->isLambda() && !isClassOrMethodDLLImport(CXXDecl)) {
+ for (auto ctor : CXXDecl->ctors()) {
+ if (ctor->isUserProvided())
----------------
Is the type here a pointer - if so, then "auto *ctor" would be preferred. (if
it's not, then probably want to avoid copying it and use "auto &ctor")
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3692
+ Args.getLastArg(options::OPT_flimit_debug_info_constructor), Args,
D, TC) &&
+ DebugInfoKind == codegenoptions::LimitedDebugInfo)
+ DebugInfoKind = codegenoptions::DebugInfoConstructor;
----------------
I'm surprised that this flag would only apply if you were already using
-flimit-debug-info but that looks like what this code does? (I'd probably
expect these to override each other -flimit-debug-info
-flimit-debug-info-constructor gets you constructor limited, the other way
around gets the classic/pre-patch behavior? but once this becomes not a driver
option anymore, that gets a bit murkier I guess)
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:23
+
+extern int bar(B *b);
+int TestB(B *b) {
----------------
"extern" here is redundant - probably best to remove it.
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:35-36
+C *TestC() {
+ C *c = new C();
+ return c;
+}
----------------
Skip the local variable if it isn't needed & "return new C();" perhaps?
Though perhaps all these uses of "new" could use the direct type instead (as
that should produce simpler IR, maybe make the test cases more obvious)?
"void TestC() { C c; }"
(or "C Test() { return C(); }" but that seems more complicated.
Or maybe even use a straight global variable "C c;" ? (but I guess then you
might need "extern C c; C c;" in some cases to make sure they're emitted at all
- I'm not sure)
Or does that not capture what they're intended to test?
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:54-58
+class E {
+public:
+*d = new D();
+ return d;
+}
----------------
Did something get mangled in the patch upload, perhaps? - this doesn't look
like valid code.
================
Comment at: clang/test/CodeGenCXX/debug-info-limited-ctor.cpp:63-64
+// CHECK-SAME: ){{$}}
+class E {
+public:
+ E();
----------------
I'd probably write these all as structs, if the class/struct distinction isn't
relevant - saves an extra line of "public:".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72427/new/
https://reviews.llvm.org/D72427
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
