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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
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));
+
----------------
dblaikie wrote:
> 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.
I think it'd make sense to add a -cc1/driver option if we wanted users to be able to toggle this functionality on/off. But, I don't think we do.

FWIW, GCC just turns call site info on by default (see https://godbolt.org/g/BrLCTd, note that they use .uleb128 0x2117 for DW_AT_GNU_all_call_sites but it's the same thing). From what I can make of GCC's '-###' output, they have no frontend toggle. I think the view here (which I share) is that call site info is just part of generic debugging support.

It seems prudent/sufficient to gate the llvm support on a temporary cl::opt. We'd remove the cl::opt in short order when https://reviews.llvm.org/D50478 lands.

Wdyt? Is there some use case for having a driver-level I'm just missing?


================
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.
----------------
dblaikie wrote:
> Rather than taking and returning the flags - maybe just return the flags here and users can OR them with their other flags?
Sounds good.


================
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;
----------------
dblaikie wrote:
> 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. 
Thanks for catching this! Hm, I'm still not sure that we'd need a flag to toggle call site info. ISTM that it'd be appropriate to turn it on under DebugLineTablesOnly, LimitedDebugInfo, and FullDebugInfo. FWIW, GCC enables it at -g{1,2,3}.


================
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
----------------
dblaikie wrote:
> 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?
Yes, that's the intent behind the test. I assumed that this would be covered by the `-implicit-check-not=DIFlagAllCallsDescribed`? Yep:
```
$ echo "foo foo" | ./bin/FileCheck <(echo "CHECK: foo") -implicit-check-not=foo
command line:1:22: error: CHECK-NOT: excluded string found in input
-implicit-check-not='foo'
                     ^
<stdin>:1:5: note: found here
foo foo
```


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


================
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);
----------------
dblaikie wrote:
> 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)
That's a good point, but I'm not sure it'd be worth doing the extra bookkeeping to prevent that from happening.


https://reviews.llvm.org/D49887