fdeazeve created this revision.
Herald added subscribers: jdoerfert, hiraditya.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1.
Herald added projects: clang, LLVM.
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
<https://reviews.llvm.org/owners/package/2/>
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D143921
Files:
clang/lib/CodeGen/CGDebugInfo.cpp
llvm/docs/LangRef.rst
llvm/lib/IR/Verifier.cpp
Index: llvm/lib/IR/Verifier.cpp
===================================================================
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -1400,6 +1400,8 @@
} 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()) {
Index: llvm/docs/LangRef.rst
===================================================================
--- llvm/docs/LangRef.rst
+++ llvm/docs/LangRef.rst
@@ -5772,11 +5772,12 @@
.. _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
@@ -5785,9 +5786,9 @@
}
!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,
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===================================================================
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4217,7 +4217,7 @@
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
Index: llvm/lib/IR/Verifier.cpp
===================================================================
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -1400,6 +1400,8 @@
} 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()) {
Index: llvm/docs/LangRef.rst
===================================================================
--- llvm/docs/LangRef.rst
+++ llvm/docs/LangRef.rst
@@ -5772,11 +5772,12 @@
.. _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
@@ -5785,9 +5786,9 @@
}
!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,
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===================================================================
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4217,7 +4217,7 @@
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
