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

Felipe de Azevedo Piovezan via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 20 11:23:13 PST 2023


Author: Felipe de Azevedo Piovezan
Date: 2023-02-20T14:22:49-05:00
New Revision: 997dc7e00f49663b60a78e18df1dfecdf62a4172

URL: https://github.com/llvm/llvm-project/commit/997dc7e00f49663b60a78e18df1dfecdf62a4172
DIFF: https://github.com/llvm/llvm-project/commit/997dc7e00f49663b60a78e18df1dfecdf62a4172.diff

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

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

Differential Revision: https://reviews.llvm.org/D143921

Added: 
    llvm/test/Verifier/disubprogram_declaration.ll

Modified: 
    clang/lib/CodeGen/CGDebugInfo.cpp
    llvm/docs/LangRef.rst
    llvm/lib/IR/Verifier.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index bb91b5116d71b..4dab595ada76b 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4225,10 +4225,9 @@ void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
 
   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);
+  llvm::DISubprogram *SP = DBuilder.createFunction(
+      FDContext, Name, LinkageName, Unit, LineNo, STy, ScopeLine, Flags,
+      SPFlags, TParamsArray.get(), 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

diff  --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 4052e58faa7fb..b4c9cff4821cc 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -5776,11 +5776,12 @@ retained, even if their IR counterparts are optimized out of the IR. The
 
 .. _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
 
@@ -5789,9 +5790,9 @@ and ``scope:``.
     }
 
     !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,

diff  --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 40968a262811c..97ab9d2dfd663 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -1400,6 +1400,8 @@ void Verifier::visitDISubprogram(const DISubprogram &N) {
   } 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()) {

diff  --git a/llvm/test/Verifier/disubprogram_declaration.ll b/llvm/test/Verifier/disubprogram_declaration.ll
new file mode 100644
index 0000000000000..e3402c9970866
--- /dev/null
+++ b/llvm/test/Verifier/disubprogram_declaration.ll
@@ -0,0 +1,17 @@
+; RUN: llvm-as -disable-output <%s 2>&1| FileCheck %s
+
+declare !dbg !12 i32 @declared_only()
+
+!llvm.module.flags = !{!2}
+!llvm.dbg.cu = !{!5}
+
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !6, producer: "clang", emissionKind: FullDebug)
+!6 = !DIFile(filename: "a.cpp", directory: "/")
+!7 = !{}
+!11 = !DISubroutineType(types: !7)
+
+!12 = !DISubprogram(name: "declared_only", scope: !6, file: !6, line: 2, type: !11, spFlags: DISPFlagOptimized, retainedNodes: !7, declaration: !13)
+!13 = !DISubprogram(name: "declared_only", scope: !6, file: !6, line: 2, type: !11, spFlags: DISPFlagOptimized, retainedNodes: !7)
+; CHECK: subprogram declaration must not have a declaration field
+; CHECK: warning: ignoring invalid debug info


        


More information about the llvm-commits mailing list