@llvm/pr-subscribers-clang
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang-codegen

Author: J. Ryan Stinnett (jryans)

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 465f3f4e670c2..2e4ffd42b915c 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -6277,6 +6277,21 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
     pushDestroy(QualType::DK_nontrivial_c_struct, Ret.getAggregateAddress(),
                 RetTy);
 
+  if (CalleeDecl) {
+    // Generate function declaration DISuprogram in order to be used
+    // in debug info about call sites.
+    if (CGDebugInfo *DI = getDebugInfo()) {
+      CodeGenFunction CalleeCGF(CGM);
+      const GlobalDecl &CalleeGlobalDecl =
+          Callee.getAbstractInfo().getCalleeDecl();
+      CalleeCGF.CurGD = CalleeGlobalDecl;
+      FunctionArgList Args;
+      QualType ResTy = CalleeCGF.BuildFunctionArgList(CalleeGlobalDecl, Args);
+      DI->EmitFuncDeclForCallSite(
+          CI, DI->getFunctionType(CalleeDecl, ResTy, Args), CalleeGlobalDecl);
+    }
+  }
+
   return Ret;
 }
 
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 07a2cfb21bef2..67675dae71c34 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4952,7 +4952,7 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
 
 void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
                                           QualType CalleeType,
-                                          const FunctionDecl *CalleeDecl) {
+                                          GlobalDecl CalleeGlobalDecl) {
   if (!CallOrInvoke)
     return;
   auto *Func = dyn_cast<llvm::Function>(CallOrInvoke->getCalledOperand());
@@ -4961,6 +4961,9 @@ void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
   if (Func->getSubprogram())
     return;
 
+  const FunctionDecl *CalleeDecl =
+      cast<FunctionDecl>(CalleeGlobalDecl.getDecl());
+
   // Do not emit a declaration subprogram for a function with nodebug
   // attribute, or if call site info isn't required.
   if (CalleeDecl->hasAttr<NoDebugAttr>() ||
@@ -4971,7 +4974,8 @@ void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
   // create the one describing the function in order to have complete
   // call site debug info.
   if (!CalleeDecl->isStatic() && !CalleeDecl->isInlined())
-    EmitFunctionDecl(CalleeDecl, CalleeDecl->getLocation(), CalleeType, Func);
+    EmitFunctionDecl(CalleeGlobalDecl, CalleeDecl->getLocation(), CalleeType,
+                     Func);
 }
 
 void CGDebugInfo::EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD) {
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 78c3eb9c5792e..1f695941f113a 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -511,7 +511,7 @@ class CGDebugInfo {
   /// This is needed for call site debug info.
   void EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
                                QualType CalleeType,
-                               const FunctionDecl *CalleeDecl);
+                               GlobalDecl CalleeGlobalDecl);
 
   /// Constructs the debug code for exiting a function.
   void EmitFunctionEnd(CGBuilderTy &Builder, llvm::Function *Fn);
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 01f2161f27555..a837f00732748 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -6632,15 +6632,6 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType,
                          E == MustTailCall, E->getExprLoc());
 
   if (auto *CalleeDecl = dyn_cast_or_null<FunctionDecl>(TargetDecl)) {
-    // Generate function declaration DISuprogram in order to be used
-    // in debug info about call sites.
-    if (CGDebugInfo *DI = getDebugInfo()) {
-      FunctionArgList Args;
-      QualType ResTy = BuildFunctionArgList(CalleeDecl, Args);
-      DI->EmitFuncDeclForCallSite(LocalCallOrInvoke,
-                                  DI->getFunctionType(CalleeDecl, ResTy, Args),
-                                  CalleeDecl);
-    }
     if (CalleeDecl->hasAttr<RestrictAttr>() ||
         CalleeDecl->hasAttr<AllocSizeAttr>()) {
       // Function has 'malloc' (aka. 'restrict') or 'alloc_size' attribute.
diff --git a/clang/test/DebugInfo/CXX/decl-member-call.cpp b/clang/test/DebugInfo/CXX/decl-member-call.cpp
new file mode 100644
index 0000000000000..95758a2985c0c
--- /dev/null
+++ b/clang/test/DebugInfo/CXX/decl-member-call.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -O1 -triple x86_64-unknown_unknown -emit-llvm \
+// RUN:   -debug-info-kind=standalone -dwarf-version=5 %s -o - | FileCheck %s
+
+// Ensure both nonmember and member calls to declared function
+// have attached `DISubprogram`s.
+
+int nonmember(int n);
+
+struct S {
+  int x;
+  int member(int n);
+};
+
+int main(int argc, char** argv) {
+  struct S s = {};
+  int a = s.member(argc);
+  int b = nonmember(argc);
+  return a + b;
+}
+
+// CHECK: declare !dbg ![[SP1:[0-9]+]] noundef i32 @_ZN1S6memberEi(
+// CHECK: declare !dbg ![[SP2:[0-9]+]] noundef i32 @_Z9nonmemberi(
+
+// CHECK: ![[SP1]] = !DISubprogram(name: "member", linkageName: "_ZN1S6memberEi"
+// CHECK: ![[SP2]] = !DISubprogram(name: "nonmember", linkageName: "_Z9nonmemberi"

This change does increase debug info size a bit. Here's a diff of object file section sizes when compiling a recent commit of Clang using -O2 -g on Linux:

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +1.4% +9.97Mi  [ = ]       0    .debug_info
  +2.3% +1.01Mi  [ = ]       0    .debug_addr
  +0.7%  +836Ki  [ = ]       0    .debug_str_offsets
  +0.0%  +129Ki  [ = ]       0    .debug_str
  +0.0% +22.1Ki  [ = ]       0    .debug_loclists
  +0.0% +7.96Ki  [ = ]       0    .debug_rnglists
   +21% +1.44Ki  [ = ]       0    [Unmapped]
  -0.0%     -59  [ = ]       0    .debug_line_str
  -0.0%    -540  [ = ]       0    .debug_line
  -0.0% -1.44Ki  -0.0% -1.44Ki    .rodata
  -0.5% -60.6Ki  [ = ]       0    .debug_abbrev
  +0.7% +11.9Mi  -0.0% -1.44Ki    TOTAL

Since this additional call site info was intended to be there all along (AIUI) and is needed to properly retrieve variable values that use DW_OP_entry_value, I believe we should accept the larger binary size.

This change does increase debug info size a bit. Here's a diff of object file section sizes when compiling a recent commit of Clang using -O2 -g on Linux:

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +1.4% +9.97Mi  [ = ]       0    .debug_info
  +2.3% +1.01Mi  [ = ]       0    .debug_addr
  +0.7%  +836Ki  [ = ]       0    .debug_str_offsets
  +0.0%  +129Ki  [ = ]       0    .debug_str
  +0.0% +22.1Ki  [ = ]       0    .debug_loclists
  +0.0% +7.96Ki  [ = ]       0    .debug_rnglists
   +21% +1.44Ki  [ = ]       0    [Unmapped]
  -0.0%     -59  [ = ]       0    .debug_line_str
  -0.0%    -540  [ = ]       0    .debug_line
  -0.0% -1.44Ki  -0.0% -1.44Ki    .rodata
  -0.5% -60.6Ki  [ = ]       0    .debug_abbrev
  +0.7% +11.9Mi  -0.0% -1.44Ki    TOTAL

Since this additional call site info was intended to be there all along (AIUI) and is needed to properly retrieve variable values that use DW_OP_entry_value, I believe we should accept the larger binary size.

This is okay. Thanks.

However, I would mention that AFAIK, a call_site entry can be useful iff:

1. is marked as a tail call (debuggers use it to show the artificial frames)
2. has call-site-paramters (debuggers use it along with entry_values)

Otherwise, we can avoid the call_sites in the final DWARF, and it should reduce the final binary size.

I would mention that AFAIK, a call_site entry can be useful iff:

1. is marked as a tail call (debuggers use it to show the artificial frames)
2. has call-site-paramters (debuggers use it along with entry_values)

Otherwise, we can avoid the call_sites in the final DWARF, and it should reduce the final binary size.

Thanks for summarising those major use cases for the call site info.

From a quick skim, I believe many of the added call site entries have call site parameters (and thus add clear value assuming any callee variables use DW_OP_entry_value). There are also some added non-tail call site entries without call site parameters, and those indeed may not be used by debuggers today (though perhaps some misc. tools use it to build a call graph).

If we wish to remove call site entries that don't seem to add value, I think it would be best to discuss that separately from this PR.

✅ With the latest revision this PR passed the C/C++ code formatter.

Heads-up: This is causing ~1.5% increase in bin size of clang when built internally in Google and ~1.8% increase in dwp file. It does the same for a couple other internal google targets. I'm posting a heads-up for now and we will discuss internally how much of an issue this is before we come back to the thread. Thanks

I guess a patch that handles this #166202 (comment) can improve things here a lot.

This is causing ~1.5% increase in bin size of clang when built internally in Google and ~1.8% increase in dwp file.

Thanks for the update. A bit more of an increase than my own comparison above, but perhaps further optimisations, LTO modes, etc. are revealing more opportunities to insert this kind of debug info.

Since this is effectively fixing a debug correctness issue, we would want to retain the core fix here. If the size change is deemed to be too large, please open a new issue so we can use that to coordinate ways to reduce space, such as @djtodoro's suggestions above.

However, I would mention that AFAIK, a call_site entry can be useful iff:

1. is marked as a tail call (debuggers use it to show the artificial frames)
2. has call-site-paramters (debuggers use it along with entry_values)
   Otherwise, we can avoid the call_sites in the final DWARF, and it should reduce the final binary size.

I don't think this is true for our (Sony) debugger, which wants to see call site info even in cases without call site parameters (it's not only using it for entry_value computation).

@asmok-g said:

Heads-up: This is causing ~1.5% increase in bin size of clang when built internally in Google and ~1.8% increase in dwp file. It does the same for a couple other internal google targets. I'm posting a heads-up for now and we will discuss internally how much of an issue this is before we come back to the thread. Thanks

@jryans said:

[...] this is effectively fixing a debug correctness issue [...]

As @jryans points out this is more of a bug fix as iIIUC t's adding coverage that should have been there from the start.

Does google use call site information (does it care about entry_values, or do any extra stuff with the info)? If not, I wonder if it would be useful to add a flag to disable call site info for your builds (if such a flag doesn't already exist)?

Does google use call site information (does it care about entry_values, or do any extra stuff with the info)?

@dwblaikie or @labath should know the answer.

I wonder if it would be useful to add a flag to disable call site info for your builds (if such a flag doesn't already exist)?

I think regardless of the debug information completeness improvement, the 1.5%+ increase may be problematic in some cases. We probably need a flag to mitigate this increase without disabling debug information altogether. If there's already a flag that controls the information being added by this change (or a reasonable superset of it), we may be able to suggest it as a workaround for teams who have to squeeze their binaries into hard size limits.

I have attempted to carry over the points made so far into #168851. Let's try to focus further discussion there.