[PATCH] D49887: [DebugInfo] Add support for DWARF5 call site-related attributes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

probably leave this a bit more to @aprantl and @JDevlieghere (figure they can speak more to the LLDB needs/tradeoffs/etc that this is targeting) - but there's a first pass.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:51-55
+static llvm::cl::opt<bool> CallSiteDebugInfo(
+    "callsite-debuginfo-experimental", llvm::cl::Hidden,
+    llvm::cl::desc("Emit call site-related debug information (experimental)"),
+    llvm::cl::init(false));
+
----------------
Notice there aren't many cl::opts in clang - this should instead probably be added as a cc1 option, probably a '-f' option (see if GCC has a flag name for this already you can reuse?). Once it's more than experimental, promoted up to a full driver option that forwards down to a cc1 option, etc.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4371
+llvm::DINode::DIFlags
+CGDebugInfo::setCallSiteRelatedAttrs(llvm::DINode::DIFlags Flags) const {
+  // This is an opt-in experimental feature.
----------------
Rather than taking and returning the flags - maybe just return the flags here and users can OR them with their other flags?


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4376-4380
+  // Call site-related attributes are only useful in optimized programs, and
+  // should only be emitted when full debug info is desired.
+  if (!CGM.getLangOpts().Optimize ||
+      DebugKind < codegenoptions::FullDebugInfo)
+    return Flags;
----------------
I don't think "full" debug info is the right property here - the difference between "full"/standalone and "limited" is that limited assumes the whole program is built with debug info - so it can expect to find debug info in another translation unit/compile unit to cover needed types, etc.

This doesn't fit under that umbrella - I think it probably needs a separate -f flag and it can default to on when tuning for LLDB potentially. 


================
Comment at: clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp:44
+// HAS-ATTR-DAG: DISubprogram(name: "struct2", {{.*}}, isDefinition: true, {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed
+// HAS-ATTR-DAG: DISubprogram(name: "method2", {{.*}}, isDefinition: false, {{.*}}, flags: DIFlagPrototyped
+// HAS-ATTR-DAG: DISubprogram(name: "method2", {{.*}}, isDefinition: true, {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed
----------------
I take it this is intended to test that non-defining declarations do /not/ have the DIFlagAllCallsDescribed added? But it looks like this check line doesn't test that it's /not/ present. Perhaps adding a comma at the end to ensure no other flags are being passed in the flags field?


================
Comment at: clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp:48-67
+void declaration1();
+
+void declaration2();
+
+void declaration2() {}
+
+class class1 {
----------------
Are these all interesting? Looks like they're all handled by the same code/aren't uniquely interesting situations, perhaps?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:222
+  for (; Curr.isValid() && !Curr.isSubprogramDIE(); Curr = Die.getParent()) {
+    if (Curr.isSubroutineDIE()) {
+      error() << "Call site entry nested within inlined subroutine:";
----------------
isSubroutineDIE checks for both DW_TAG_subprogram and DW_TAG_inlined_subroutine. Given this code specifically only wants to check for the latter - perhaps it should do that directly?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:239
+  if (!CallAttr) {
+    error() << "Subprogram with call site entry has no DW_AT_call attribute:";
+    Curr.dump(OS);
----------------
Not sure how this compares to other parts of the verifier - but I imagine this would be rather noisy if there was debug info with all correct DW_TAG_call_sites, but missing DW_AT_call attribute - a function would have many DW_TAG_call_sites, all generating errors, rather than just the first one in each function? 

(but maybe that's not a huge deal? Dunno)


https://reviews.llvm.org/D49887