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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Hi Vedant, apologies for the delay in reviewing this. I have a few nits inline but otherwise this LGTM!



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4386
+  if (!CGM.getLangOpts().Optimize ||
+      (DebugKind == codegenoptions::NoDebugInfo ||
+       DebugKind == codegenoptions::LocTrackingOnly))
----------------
Any reason you have extra parenthesis here?


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4396
+      CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB;
+  if (CGM.getCodeGenOpts().DwarfVersion < 5 && !SupportsDWARFv4Ext)
+    return llvm::DINode::FlagZero;
----------------
nit: Let's switch those around so we gain short-circuiting for the already computed boolean.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:552
+      const MCSymbol *ReturnPC = nullptr;
+      if (!IsTail)
+        ReturnPC = getLabelAfterInsn(&MI);
----------------
nit: seems like you could simplify this with a ternary operator.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1393
+  // in call site entries. TODO: Add support for targets with delay slots.
+  if (SP->areAllCallsDescribed())
+    if (MI->isCall() && !MI->hasDelaySlot())
----------------
nit: you could merge these two. 


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:216
 
+bool DWARFVerifier::verifyDebugInfoCallSite(const DWARFDie &Die) {
+  if (Die.getTag() != DW_TAG_call_site)
----------------
We tend to return an `unsigned` for the verifiers. It doesn't really matter much but it's nice for consistency and makes it easy to change if there's ever more than one failures. 


https://reviews.llvm.org/D49887