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

[djtodoro]

Summary

In order to have call_origin to an external function we need DISubprogram for the function.

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

[djtodoro]

-Rebase
-Improve the verifier
-Add a test
-Document the spFlag
-Refactor the code

[aprantl]

Can you explain or (link to a review that explains) why this flag is necessary? What are we avoiding/gaining by having it?

[djtodoro]

@aprantl Thanks again for your comments!

The patch is needed to describe a call site to an external function. Currently, we do not have all necessary info for the debug call site (particular debug subprogram is missing for the external function). Since we can describe call site parameters for calls like that, we are producing additional subprogram for the declaration of the extern function by marking it with the DISPFlagDeclForCallSite flag.
Basically, this is improvement of existing call site debug info production.

Please find the frontend part: D60714

[aprantl]

The patch is needed to describe a call site to an external function. Currently, we do not have all necessary info for the debug call site (particular debug subprogram is missing for the external function). Since we can describe call site parameters for calls like that, we are producing additional subprogram for the declaration of the extern function by marking it with the DISPFlagDeclForCallSite flag.

I'm sorry if the answer to that question is already in your reply and I just failed to parse it: Why do we need the DISPFlagDeclForCallSite *flag* to differentiate the a DISubprogram. Isn't the fact that it is attached to a forward-declaration enough?

Secondly: since this is attached to a forward *declaration*, would it make sense to attach the unique (= not distinct) DISubprogram from the type hierarchy instead of creating another distinct node? What is the intended behavior when two LLVM modules containing the same forward declaration are llvm-linked during LTO?

[djtodoro]

-Refactor the code
-Change the test case

[djtodoro]

I'm sorry if the answer to that question is already in your reply and I just failed to parse it: Why do we need the DISPFlagDeclForCallSite *flag* to differentiate the a DISubprogram. Isn't the fact that it is attached to a forward-declaration enough?

We could avoid the flag, but since existing LLVM infrastructure does not allow debug info for function declarations itself, we wanted to emphasize that we are creating it only for the purpose of call site’s debug info. The verifier to distinguish that the declaration will be used in that purpose uses the flag.

Secondly: since this is attached to a forward *declaration*, would it make sense to attach the unique (= not distinct) DISubprogram from the type hierarchy instead of creating another distinct node?

Oh, the test case from this patch was artificial one. We are actually creating it that way. There is no distinct subprograms for the declarations. Please see the test case updated.

What is the intended behavior when two LLVM modules containing the same forward declaration are llvm-linked during LTO?

LLVM linked file will have just one debug info subprogram instance for the declaration.

[aprantl]

Prior to this patch Function declarations may not have a !dbg attachment, because there was no use-case for it. With call site debug info, I think it would make sense to revisit this rule. When I made my previous comment, I thought that the updated rule could be:

(1) function definitions may have a distinct DISubprogram attached (as before)
(2) function declarations may have a unique DISubprogram attached (new)

If I read the code in Verifier.cpp correctly, then it is legal for a unique DISubprogram to not have a unit, which would cause the declaration-DISubprograms to be uniqued during LTO.

so the (2) should probably be:

(2) function declarations may have a unique DISubprogram attached and it may not have a unit: field

I've CC'ed Duncan in case he can catch a logic error with this reasoning.

[dexonsmith]

In D60713#1523315, @aprantl wrote:

Prior to this patch Function declarations may not have a !dbg attachment, because there was no use-case for it. With call site debug info, I think it would make sense to revisit this rule. When I made my previous comment, I thought that the updated rule could be:

(1) function definitions may have a distinct DISubprogram attached (as before)
(2) function declarations may have a unique DISubprogram attached (new)

If I read the code in Verifier.cpp correctly, then it is legal for a unique DISubprogram to not have a unit, which would cause the declaration-DISubprograms to be uniqued during LTO.

so the (2) should probably be:

(2) function declarations may have a unique DISubprogram attached and it may not have a unit: field

I've CC'ed Duncan in case he can catch a logic error with this reasoning.

That rule seems reasonable. The critical piece is that declarations will show up in ~every TU, and to be uniqued properly they can't point at anything identifying which TU they're coming from.

[djtodoro]

-Remove the SP flag
-Attach a unique sp to a declaration for the purpose of call site dbg info

[djtodoro]

@aprantl , @dexonsmith Thanks for your comments!

Now, we avoid the sp flag and a unique sp is attached on to a declaration needed for call site debug info.
Please see the following example.

---

For the test1.c:

` ...

extern int fn1 (int a);

int fn2(int arg) {
  ...
  arg = fn1(s);
  printf("\n");

...`

LLVM IR will look like:

...
!0 = distinct !DICompileUnit(...,retainedTypes: !3)
!1 = !DIFile(filename: "test1.c", directory: "/dir")
...
!3 = !{!4, !8}
!4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...
!8 = !DISubprogram(name: "printf", scope: !9, file: !9, line: 361, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
!9 = !DIFile(filename: "path_to/include/stdio.h", directory: "")
...

---

For the test2.c:

` ...

extern int fn1 (int a);

int fn3(int arg3) {
  ...
  arg3 = fn1(k);
  printf("\n");

...`

LLVM IR will look like:

...
!0 = distinct !DICompileUnit(...,retainedTypes: !3)
!1 = !DIFile(filename: "test2.c", directory: "/dir")
...
!3 = !{!4, !8}
!4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...
!8 = !DISubprogram(name: "printf", scope: !9, file: !9, line: 361, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
!9 = !DIFile(filename: "path_to/include/stdio.h", directory: "")
...

---

By linking the llvm files together we will have:

...
!0 = distinct !DICompileUnit(...,retainedTypes: !3)
!1 = !DIFile(filename: "test1.c", directory: "/dir")
...
!3 = !{!4, !8}
!4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...
!8 = !DISubprogram(name: "printf", scope: !9, file: !9, line: 361, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
!9 = !DIFile(filename: "path_to/include/stdio.h", directory: "")
...
!15 = distinct !DICompileUnit(..., retainedTypes: !17)
!16 = !DIFile(filename: "test2.c", directory: "/dir")
!17 = !{!18, !8}
!18 = !DISubprogram(name: "fn1", scope: !16, file: !16, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...

[djtodoro]

(2) function declarations may have a unique DISubprogram attached (new)

In addition, this will address only this.

[aprantl]

Thanks!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

672 ↗

(On Diff #202417)

Create DIEs for function declarations used for call site debug info.

lib/IR/Verifier.cpp

2228 ↗

(On Diff #202417)

LGTM!

[aprantl]

lib/IR/Verifier.cpp

2228 ↗

(On Diff #202417)

Perhaps add a comment here:
// This is used for call site debug information.

[aprantl]

Oh.. and could you please document this either in LangRef.rst or SourceLevelDebugging.rst?

[djtodoro]

-Address suggestions
-Document this

[djtodoro]

Oh.. and could you please document this either in LangRef.rst or SourceLevelDebugging.rst?

Sure. Please find it in the latest patch.
Thanks for your comments!

[aprantl]

Thanks!

llvm/docs/LangRef.rst

 a :ref:`compile unit <DICompileUnit>`.
 
 .. _DISubprogram:
 
 DISubprogram
 """"""""""""
 
 ``DISubprogram`` nodes represent functions from the source language. A
-``DISubprogram`` may be attached to a function definition using ``!dbg``
-metadata. The ``variables:`` field points at :ref:`variables <DILocalVariable>`
-that must be retained, even if their IR counterparts are optimized out of
-the IR. The ``type:`` field must point at an :ref:`DISubroutineType`.
+distinct ``DISubprogram`` may be attached to a function definition using
+``!dbg`` metadata. A unique ``DISubprogram`` may be attached to a function
+declaration used for call site debug info. The ``variables:`` field points at
+:ref:`variables <DILocalVariable>` that must be retained, even if their IR
+counterparts are optimized out of the IR. The ``type:`` field must point at an
+:ref:`DISubroutineType`.
 
 .. _DISubprogramDeclaration:
 
 When ``isDefinition: false``, subprograms describe a declaration in the type
 tree as opposed to a definition of a function.  If the scope is a composite
 type with an ODR ``identifier:`` and that does not set ``flags: DIFwdDecl``,
 then the subprogram declaration is uniqued based only on its ``linkageName:``
 and ``scope:``.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

DwarfDebug::getOrCreateDwarfCompileUnit(const DICompileUnit *DIUnit) {
   if (useSplitDwarf()) {
     NewCU.setSkeleton(constructSkeletonCU(NewCU));
     NewCU.setSection(Asm->getObjFileLowering().getDwarfInfoDWOSection());
   } else {
     finishUnitAttributes(DIUnit, NewCU);
     NewCU.setSection(Asm->getObjFileLowering().getDwarfInfoSection());
   }
 
+  // Create DIEs for function declarations used for call site debug info.
+  for (auto Scope : DIUnit->getRetainedTypes())
+    if (auto *SP = dyn_cast_or_null<DISubprogram>(Scope))
+      NewCU.getOrCreateSubprogramDIE(SP);
+
   CUMap.insert({DIUnit, &NewCU});
   CUDieMap.insert({&NewCU.getUnitDie(), &NewCU});
   return NewCU;
 }
 
 void DwarfDebug::constructAndAddImportedEntityDIE(DwarfCompileUnit &TheCU,
                                                   const DIImportedEntity *N) {
   if (isa<DILocalScope>(N->getScope()))

llvm/lib/IR/Verifier.cpp

void Verifier::visitFunction(const Function &F) {
   }
 
   if (F.isMaterializable()) {
     // Function has a body somewhere we can't see.
     Assert(MDs.empty(), "unmaterialized function cannot have metadata", &F,
            MDs.empty() ? nullptr : MDs.front().second);
   } else if (F.isDeclaration()) {
     for (const auto &I : MDs) {
-      AssertDI(I.first != LLVMContext::MD_dbg,
-               "function declaration may not have a !dbg attachment", &F);
+      // This is used for call site debug information.
+      AssertDI(I.first != LLVMContext::MD_dbg ||
+                   !cast<DISubprogram>(I.second)->isDistinct(),
+               "function declaration may only have a unique !dbg attachment",
+               &F);
       Assert(I.first != LLVMContext::MD_prof,
              "function declaration may not have a !prof attachment", &F);
 
       // Verify the metadata itself.
       visitMDNode(*I.second);
     }
     Assert(!F.hasPersonalityFn(),
            "Function declaration shouldn't have a personality routine", &F);

llvm/test/Verifier/metadata-function-dbg.ll

 ; RUN: llvm-as %s -disable-output 2>&1 | FileCheck %s
 
-; CHECK:      function declaration may not have a !dbg attachment
+; CHECK:      function declaration may only have a unique !dbg attachment
 declare !dbg !4 void @f1()
 
+; CHECK-NOT:      function declaration may only have a unique !dbg attachment
+declare !dbg !6 void @f5()
+
 ; CHECK:      function must have a single !dbg attachment
 define void @f2() !dbg !4 !dbg !4 {
   unreachable
 }
 
 ; CHECK:      DISubprogram attached to more than one function
 define void @f3() !dbg !4 {
   unreachable
define void @bar() !dbg !3 {
   unreachable
 }
 
 ; CHECK: warning: ignoring invalid debug info
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"Debug Info Version", i32 3}
 
 !llvm.dbg.cu = !{!1}
-!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2)
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, retainedTypes: !5)
 !2 = !DIFile(filename: "t.c", directory: "/path/to/dir")
 !3 = !{}
 !4 = distinct !DISubprogram(name: "foo", scope: !1, file: !2, unit: !1)
+!5 = !{!6}
+!6 = !DISubprogram(name: "f5", scope: !1, file: !2, unit: !1, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)