[clang] Add DISuprogram and DIE for func decl of an external

[djtodoro]

Summary

Front-end part for production of DISubprogram of an external function necessary for DWARF info about call sites.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

3596 ↗

(On Diff #195194)

Does that mean that there is another call to setSubprogram elsewhere that becomes redundant now?

lib/CodeGen/CGExpr.cpp

4798 ↗

(On Diff #195194)

From a layering perspective, all logic that knows about DWARF should really live in CGDebugInfo and there should be a higher-level and much simpler callback here.

Also I wonder: Is this the only place where calls are emitted?
What about ObjC message sends, for example?

[djtodoro]

lib/CodeGen/CGDebugInfo.cpp

3596 ↗

(On Diff #195194)

There is another call to setSubprogram whenever start of a function gets emitted (in CGDebugInfo::EmitFunctionStart).

In general, extern functions don't have function definition in a current compile unit, so in those situations setSubprogram should be called just at this place. But, we found that in some situations, even if a function is extern, function definition of the function gets created, so in that case we would firstly set SP at this place and than replace it in CGDebugInfo::EmitFunctionStart.

lib/CodeGen/CGExpr.cpp

4798 ↗

(On Diff #195194)

We only tested this functionality (debug user experience) within GDB (by using GNU extensions).
Since we are adding DWARF 5 (and LLDB will use call_site_parameters for printing @entry) this definitely should be extended for it.

Also I wonder: Is this the only place where calls are emitted?

I think so.

What about ObjC message sends, for example?

We have not yet tested this for ObjC. We should at some point, but I think this all functionality should work for ObjC as well?

[djtodoro]

-Format
-Support DWARF 5 as well

[aprantl]

Thanks for all your responses. I think the only thing left to do here is to factor out all the CGDebugInfo-related code into a CGDebugInfo::handleFunctionCall() (or similarly named) method, so all the debug info code is contained in CGDebugInfo.cpp.

[djtodoro]

@aprantl Sure. Thanks for your comments!

[djtodoro]

-Move debug info related code into CGDebugInfo::HandleFunctionCall()
-Add test case

[aprantl]

(I apologize if I keep asking the same questions over and over again.)
If I understand correctly, this causes clang to emit a DISubprogram with SPFlagDeclForCallSite set for each call site.

• This is needed because DW_AT_call_parameter needs to link to the DW_TAG_formal_parameter DIE in the called DW_TAG_subprogram. Correct?
• Why is that flag not being tested for in the test case?
• What happens when we emit a function body *after* we emitted a DISubprogram+ SPFlagDeclForCallSite for the same function? Do we get two DISubprograms, or one, and does it have the SPFlagDeclForCallSite flag set?

lib/CodeGen/CGDebugInfo.h

412 ↗

(On Diff #196208)

All the other methods are called "Emit"-something.

[djtodoro]

@aprantl Since this is a very big feature, we want to clarify everything as well, so everything is OK. :)

Actually, I think that we have a small misunderstanding about this part. SPFlagDeclForCallSite is used just to mark a DISubprogram as declaration for call site. It will be emitted only in the case if subprogram of a function is not there at the moment (if (!Func->getSubprogram()). So, it is the case only for extern functions. We are doing this in order to avoid ‘DW_TAG_call_site’ with no ‘call_reference’ attribute set. GCC does the same.

This part improves existing call site debug info in LLVM.

lib/CodeGen/CGDebugInfo.h

412 ↗

(On Diff #196208)

What about EmitFuncDeclForCallSite()?

[aprantl]

In D60714#1478674, @djtodoro wrote:

Actually, I think that we have a small misunderstanding about this part. SPFlagDeclForCallSite is used just to mark a DISubprogram as declaration for call site. It will be emitted only in the case if subprogram of a function is not there at the moment (if (!Func->getSubprogram()). So, it is the case only for extern functions.

That's what wasn't quite clear to me: When the call site of a (internal or linkonce-odr) function is visited before the function bode is code-generated, what happens? Do we end up with two DISubprograms, one with a forward decl and one with the real definition? Will we end up with a DISubprogram with a definition *and* an SPFlagDeclForCallSite?

We are doing this in order to avoid ‘DW_TAG_call_site’ with no ‘call_reference’ attribute set. GCC does the same.

Are you saying that without the extra forward declarations emitted by this patch, we would generate DW_TAG_call_site entries that don't have a DW_AT_call_origin attribute? Or something else?

This part improves existing call site debug info in LLVM.

IIUC, that makes sense to me.

[djtodoro]

That's what wasn't quite clear to me: When the call site of a (internal or linkonce-odr) function is visited before the function bode is code-generated, what happens? Do we end up with two DISubprograms, one with a forward decl and one with the real definition? Will we end up with a DISubprogram with a definition *and* an SPFlagDeclForCallSite?

At this place functions are already visited, so every function with its body has DISubprogram set up. But, if somewhere out there is the example like that, we will have two DISubprograms generated.

Are you saying that without the extra forward declarations emitted by this patch, we would generate DW_TAG_call_site entries that don't have a DW_AT_call_origin attribute? Or something else?

Yes, you understand it right. We see that it is missing in some cases by using the latest version of LLVM.

[aprantl]

In D60714#1478793, @djtodoro wrote:

That's what wasn't quite clear to me: When the call site of a (internal or linkonce-odr) function is visited before the function bode is code-generated, what happens? Do we end up with two DISubprograms, one with a forward decl and one with the real definition? Will we end up with a DISubprogram with a definition *and* an SPFlagDeclForCallSite?

At this place functions are already visited, so every function with its body has DISubprogram set up. But, if somewhere out there is the example like that, we will have two DISubprograms generated.

Are you saying that without the extra forward declarations emitted by this patch, we would generate DW_TAG_call_site entries that don't have a DW_AT_call_origin attribute? Or something else?

Yes, you understand it right. We see that it is missing in some cases by using the latest version of LLVM.

Good :-)
IIUC, the Verifier should reject any DIExpressions containing DW_OP_entry_values, because it doesn't know about DW_OP_reg operations. That means you are not calling the verifier on any of the expressions generated by LiveDebugValues. Could we add an assert(DIExpr->isValid()) somewhere there? Or alternatively shouldn't the machine verifier also call DIExpression::isValid on all DBG_VALUEs? The latter would be even better.

[aprantl]

Sorry! I commented on the wrong review...

[djtodoro]

-Rename the method for emitting a declaration SP for a call site
-Rebase

[aprantl]

lib/CodeGen/CGDebugInfo.cpp

3611 ↗

(On Diff #199215)

Perhaps add a comment explaining what (and why) is being done in this function?

3611 ↗

(On Diff #199215)

move this into the early-exit condition above?

3612 ↗

(On Diff #199215)

ditto

3613 ↗

(On Diff #199215)

what can this be instead of a FuncDecl? Just asking whether the dyn_cast is necessary.

[djtodoro]

-Address suggestions

[aprantl]

LGTM with three nitpicks inside.

lib/CodeGen/CGDebugInfo.cpp

3589 ↗

(On Diff #199789)

llvm::DISubroutineType *SubroutineType = nullptr;
if (!IsDeclForCallSite)
  SubroutineType = getOrCreateFunctionType(D, FnType, Unit);

Can you explain (in a comment) why? Is this not necessary, not safe, not possible?

lib/CodeGen/CGDebugInfo.h

407 ↗

(On Diff #199789)

Please add a comment explaining when the last parameter is supposed to be set.

[djtodoro]

lib/CodeGen/CGDebugInfo.cpp

3589 ↗

(On Diff #199789)

Actually, it is possible to create the subroutine type even in the case of declaration for call site, but we need a correct callee FnType. I will update this patch with the small fix.
Thanks again for your comments!

lib/CodeGen/CGDebugInfo.h

407 ↗

(On Diff #199789)

Sure. We will add that.

[djtodoro]

-Address suggestions
-Use the right FnType needed for production of the subroutine type

[djtodoro]

-Rebase

[djtodoro]

-Rebase

[djtodoro]

-Rebase

[djtodoro]

-Handle the TargetDecl in more secure way (the same as the other code in the function)

cfe/trunk/include/clang/AST/Decl.h

public:
   bool isInlined() const { return FunctionDeclBits.IsInline; }
 
   bool isInlineDefinitionExternallyVisible() const;
 
   bool isMSExternInline() const;
 
   bool doesDeclarationForceExternallyVisibleDefinition() const;
 
+  bool isStatic() const { return getStorageClass() == SC_Static; }
+
   /// Whether this function declaration represents an C++ overloaded
   /// operator, e.g., "operator+".
   bool isOverloadedOperator() const {
     return getOverloadedOperator() != OO_None;
   }
 
   OverloadedOperatorKind getOverloadedOperator() const;
 

cfe/trunk/lib/CodeGen/CGDebugInfo.h

void EmitFunctionStart(GlobalDecl GD, SourceLocation Loc,
                          CGBuilderTy &Builder);
 
   /// Start a new scope for an inlined function.
   void EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD);
   /// End an inlined function scope.
   void EmitInlineFunctionEnd(CGBuilderTy &Builder);
 
   /// Emit debug info for a function declaration.
-  void EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc, QualType FnType);
+  /// \p Fn is set only when a declaration for a debug call site gets created.
+  void EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
+                        QualType FnType, llvm::Function *Fn = nullptr);
+
+  /// Emit debug info for an extern function being called.
+  /// This is needed for call site debug info.
+  void EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
+                               QualType CalleeType,
+                               const FunctionDecl *CalleeDecl);
 
   /// Constructs the debug code for exiting a function.
   void EmitFunctionEnd(CGBuilderTy &Builder, llvm::Function *Fn);
 
   /// Emit metadata to indicate the beginning of a new lexical block
   /// and push the block onto the stack.
   void EmitLexicalBlockStart(CGBuilderTy &Builder, SourceLocation Loc);
 

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::EmitFunctionStart(GlobalDecl GD, SourceLocation Loc,
   // Push the function onto the lexical block stack.
   LexicalBlockStack.emplace_back(SP);
 
   if (HasDecl)
     RegionMap[D].reset(SP);
 }
 
 void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
-                                   QualType FnType) {
+                                   QualType FnType, llvm::Function *Fn) {
   StringRef Name;
   StringRef LinkageName;
 
   const Decl *D = GD.getDecl();
   if (!D)
     return;
 
   llvm::DINode::DIFlags Flags = llvm::DINode::FlagZero;
   llvm::DIFile *Unit = getOrCreateFile(Loc);
-  llvm::DIScope *FDContext = getDeclContextDescriptor(D);
+  bool IsDeclForCallSite = Fn ? true : false;
+  llvm::DIScope *FDContext =
+      IsDeclForCallSite ? Unit : getDeclContextDescriptor(D);
   llvm::DINodeArray TParamsArray;
   if (isa<FunctionDecl>(D)) {
     // If there is a DISubprogram for this function available then use it.
     collectFunctionDeclProps(GD, Unit, Name, LinkageName, FDContext,
                              TParamsArray, Flags);
   } else if (const auto *OMD = dyn_cast<ObjCMethodDecl>(D)) {
     Name = getObjCMethodName(OMD);
     Flags |= llvm::DINode::FlagPrototyped;
if (Loc.isInvalid())
       CurLoc = SourceLocation();
   }
   unsigned LineNo = getLineNumber(Loc);
   unsigned ScopeLine = 0;
   llvm::DISubprogram::DISPFlags SPFlags = llvm::DISubprogram::SPFlagZero;
   if (CGM.getLangOpts().Optimize)
     SPFlags |= llvm::DISubprogram::SPFlagOptimized;
 
-  DBuilder.retainType(DBuilder.createFunction(
+  llvm::DISubprogram *SP = DBuilder.createFunction(
       FDContext, Name, LinkageName, Unit, LineNo,
       getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
-      TParamsArray.get(), getFunctionDeclaration(D)));
+      TParamsArray.get(), getFunctionDeclaration(D));
+
+  if (IsDeclForCallSite)
+    Fn->setSubprogram(SP);
+
+  DBuilder.retainType(SP);
+}
+
+void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
+                                          QualType CalleeType,
+                                          const FunctionDecl *CalleeDecl) {
+  auto &CGOpts = CGM.getCodeGenOpts();
+  if (!CGOpts.EnableDebugEntryValues || !CGM.getLangOpts().Optimize ||
+      !CallOrInvoke ||
+      CGM.getCodeGenOpts().getDebugInfo() < codegenoptions::LimitedDebugInfo)
+    return;
+
+  auto *Func = CallOrInvoke->getCalledFunction();
+  if (!Func)
+    return;
+
+  // If there is no DISubprogram attached to the function being called,
+  // create the one describing the function in order to have complete
+  // call site debug info.
+  if (Func->getSubprogram())
+    return;
+
+  if (!CalleeDecl->isStatic() && !CalleeDecl->isInlined())
+    EmitFunctionDecl(CalleeDecl, CalleeDecl->getLocation(), CalleeType, Func);
 }
 
 void CGDebugInfo::EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD) {
   const auto *FD = cast<FunctionDecl>(GD.getDecl());
   // If there is a subprogram for this function available then use it.
   auto FI = SPCache.find(FD->getCanonicalDecl());
   llvm::DISubprogram *SP = nullptr;
   if (FI != SPCache.end())

cfe/trunk/lib/CodeGen/CGExpr.cpp

if (isa<FunctionNoProtoType>(FnType) || Chain) {
     llvm::Type *CalleeTy = getTypes().GetFunctionType(FnInfo);
     CalleeTy = CalleeTy->getPointerTo();
 
     llvm::Value *CalleePtr = Callee.getFunctionPointer();
     CalleePtr = Builder.CreateBitCast(CalleePtr, CalleeTy, "callee.knr.cast");
     Callee.setFunctionPointer(CalleePtr);
   }
 
-  return EmitCall(FnInfo, Callee, ReturnValue, Args, nullptr, E->getExprLoc());
+  llvm::CallBase *CallOrInvoke = nullptr;
+  RValue Call = EmitCall(FnInfo, Callee, ReturnValue, Args, &CallOrInvoke,
+                         E->getExprLoc());
+
+  // Generate function declaration DISuprogram in order to be used
+  // in debug info about call sites.
+  if (CGDebugInfo *DI = getDebugInfo()) {
+    if (auto *CalleeDecl = dyn_cast_or_null<FunctionDecl>(TargetDecl))
+      DI->EmitFuncDeclForCallSite(CallOrInvoke, QualType(FnType, 0),
+                                  CalleeDecl);
+  }
+
+  return Call;
 }
 
 LValue CodeGenFunction::
 EmitPointerToDataMemberBinaryExpr(const BinaryOperator *E) {
   Address BaseAddr = Address::invalid();
   if (E->getOpcode() == BO_PtrMemI) {
     BaseAddr = EmitPointerWithAlignment(E->getLHS());
   } else {

cfe/trunk/test/CodeGen/debug-info-extern-call.c

+// RUN: %clang -Xclang -femit-debug-entry-values -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-EXT
+// CHECK-EXT: !DISubprogram(name: "fn1"
+
+// RUN: %clang -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
+// CHECK-NOT: !DISubprogram(name: "fn1"
+
+extern int fn1(int a, int b);
+
+int fn2 () {
+  int x = 4, y = 5;
+  int res = fn1(x, y);
+
+  return res;
+}
+