[PATCH] D72427: [DebugInfo] Add option to clang to limit debug info that is emitted for classes.

[David Blaikie via Phabricator via cfe-commits]

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