[debug-info][codegen] Prevent creation of self-referential SP node

[fdeazeve]

Summary

The function CGDebugInfo::EmitFunctionDecl is supposed to create a
declaration -- never a _definition_ -- of a subprogram. This is made
evident by the fact that the SPFlags never have the "Declaration" bit
set by that function.

However, when EmitFunctionDecl calls DIBuilder::createFunction, it
still tries to fill the "Declaration" argument by passing it the result
of getFunctionDeclaration(D). This will query an internal cache of
previously created declarations and, for most code paths, we return
nullptr; all is good.

However, as reported in [0], there are pathological cases in which we
attempt to recreate a declaration, so the cache query succeeds,
resulting in a subprogram declaration whose declaration field points to
another declaration. Through a series of RAUWs, the declaration field
ends up pointing to the SP itself. Self-referential MDNodes can't be
unique, which causes the verifier to fail (declarations must be
unique).

We can argue that the caller should check the cache first, but this is
not a correctness issue (declarations are unique anyway). The bug is
that CGDebugInfo::EmitFunctionDecl should always pass nullptr to the
declaration argument of DIBuilder::createFunction, expressing the fact
that declarations don't point to other declarations. AFAICT this is not
something for which any reasonable meaning exists.

This seems a lot like a copy-paste mistake that has survived for ~10
years, since other places in this file have the exact same call almost
token-by-token.

I've tested this by compiling LLVMSupport with and without the patch, O2
and O0, and comparing the dwarfdump of the lib. The dumps are identical
modulo the attributes decl_file/producer/comp_dir.

[0]: https://github.com/llvm/llvm-project/issues/59241

[fdeazeve]

Any testing suggestions here? I can use what we have on GH (cpp -> codegen test), but I'm not sure if there's a finer grained test we could use.

[aprantl]

llvm/lib/IR/Verifier.cpp

1404

Can you a new test for this?

[aprantl]

In D143921#4123218, @fdeazeve wrote:

Any testing suggestions here? I can use what we have on GH (cpp -> codegen test), but I'm not sure if there's a finer grained test we could use.

I was thinking of a very small IR test similar to llvm/test/Verifier/disubprogram-name-match-only.ll.

[fdeazeve]

Added verifier test

[fdeazeve]

In D143921#4124533, @aprantl wrote:

In D143921#4123218, @fdeazeve wrote:

Any testing suggestions here? I can use what we have on GH (cpp -> codegen test), but I'm not sure if there's a finer grained test we could use.

I was thinking of a very small IR test similar to llvm/test/Verifier/disubprogram-name-match-only.ll.

Yup, good idea, this should be enough! Added.

[aprantl]

Test looks good!

[fdeazeve]

Fixed clang format issue

clang/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
   if (CGM.getLangOpts().Optimize)
     SPFlags |= llvm::DISubprogram::SPFlagOptimized;
 
   llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(D);
   llvm::DISubroutineType *STy = getOrCreateFunctionType(D, FnType, Unit);
   llvm::DISubprogram *SP =
       DBuilder.createFunction(FDContext, Name, LinkageName, Unit, LineNo, STy,
                               ScopeLine, Flags, SPFlags, TParamsArray.get(),
-                              getFunctionDeclaration(D), nullptr, Annotations);
+                              nullptr, nullptr, Annotations);
 
   // Preserve btf_decl_tag attributes for parameters of extern functions
   // for BPF target. The parameters created in this loop are attached as
   // DISubprogram's retainedNodes in the subsequent finalizeSubprogram call.
   if (IsDeclForCallSite && CGM.getTarget().getTriple().isBPF()) {
     if (auto *FD = dyn_cast<FunctionDecl>(D)) {
       llvm::DITypeRefArray ParamTypes = STy->getTypeArray();
       unsigned ArgNo = 1;

llvm/docs/LangRef.rst

 metadata. A unique ``DISubprogram`` may be attached to a function declaration
 used for call site debug info. The ``retainedNodes:`` field is a list of
 :ref:`variables <DILocalVariable>` and :ref:`labels <DILabel>` 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:``.
+When ``spFlags: DISPFlagDefinition`` is not present, subprograms describe a
+declaration in the type tree as opposed to a definition of a function. In this
+case, the ``declaration`` field must be empty. 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:``.
 
 .. code-block:: text
 
     define void @_Z3foov() !dbg !0 {
       ...
     }
 
     !0 = distinct !DISubprogram(name: "foo", linkageName: "_Zfoov", scope: !1,
-                                file: !2, line: 7, type: !3, isLocal: true,
-                                isDefinition: true, scopeLine: 8,
-                                containingType: !4,
+                                file: !2, line: 7, type: !3,
+                                spFlags: DISPFlagDefinition | DISPFlagLocalToUnit,
+                                scopeLine: 8, containingType: !4,
                                 virtuality: DW_VIRTUALITY_pure_virtual,
                                 virtualIndex: 10, flags: DIFlagPrototyped,
                                 isOptimized: true, unit: !5, templateParams: !6,
                                 declaration: !7, retainedNodes: !8,
                                 thrownTypes: !9)
 
 .. _DILexicalBlock:
 

llvm/lib/IR/Verifier.cpp

if (N.isDefinition()) {
     CheckDI(N.isDistinct(), "subprogram definitions must be distinct", &N);
     CheckDI(Unit, "subprogram definitions must have a compile unit", &N);
     CheckDI(isa<DICompileUnit>(Unit), "invalid unit type", &N, Unit);
     if (N.getFile())
       verifySourceDebugInfo(*N.getUnit(), *N.getFile());
   } else {
     // Subprogram declarations (part of the type hierarchy).
     CheckDI(!Unit, "subprogram declarations must not have a compile unit", &N);
+    CheckDI(!N.getRawDeclaration(),
+            "subprogram declaration must not have a declaration field");
   }
 
   if (auto *RawThrownTypes = N.getRawThrownTypes()) {
     auto *ThrownTypes = dyn_cast<MDTuple>(RawThrownTypes);
     CheckDI(ThrownTypes, "invalid thrown types list", &N, RawThrownTypes);
     for (Metadata *Op : ThrownTypes->operands())
       CheckDI(Op && isa<DIType>(Op), "invalid thrown type", &N, ThrownTypes,
               Op);