[llvm] [DebugInfo] Swap 'Unit' and 'Type' positions in DISubprogram. (PR #96474)
Abid Qadeer via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 28 03:36:44 PDT 2024
https://github.com/abidh updated https://github.com/llvm/llvm-project/pull/96474
>From 40d316d0533b8412c2e797863f5353dccab54595 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Mon, 24 Jun 2024 10:23:23 +0100
Subject: [PATCH 1/6] [DebugInfo] Swap 'Unit' and 'Type' positions in
DISubprogram.
In current order, `Type` is processed before `Unit` by the Verifier.
This can cause a race condition. Take the following example code:
```
int test(int a[][5])
{
return a[0][2];
}
```
when compiled with clang, you will notice that control reaches
`Verifier::visitDISubrange` first with `CurrentSourceLang` still equal
to dwarf::DW_LANG_lo_user (32768). The control reaches
`Verifier::visitDICompileUnit` later and sets the value of
`CurrentSourceLang` correctly.
This behavior does not effect C like language much but is a problem for
Fortran. There is special processing in `Verifier::visitDISubrange` when
`CurrentSourceLang` is Fortran. With this problem, that special handling
is missed and verifier fails for any code that has Fortran's assumed
size array in a global subroutine.
To fix this, I have swapped the position of `Type` and `Unit`. They
were already adjacent so it does not require changing position of
anything else.
---
llvm/include/llvm/IR/DebugInfoMetadata.h | 15 ++++++++-------
llvm/lib/IR/DebugInfoMetadata.cpp | 7 +++----
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 524945862e8d4..a1d2f4c1791cf 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -1865,6 +1865,11 @@ class DISubprogram : public DILocalScope {
/// Only used by clients of CloneFunction, and only right after the cloning.
void replaceLinkageName(MDString *LN) { replaceOperandWith(3, LN); }
+ DICompileUnit *getUnit() const {
+ return cast_or_null<DICompileUnit>(getRawUnit());
+ }
+ void replaceUnit(DICompileUnit *CU) { replaceOperandWith(4, CU); }
+
DISubroutineType *getType() const {
return cast_or_null<DISubroutineType>(getRawType());
}
@@ -1873,13 +1878,9 @@ class DISubprogram : public DILocalScope {
}
void replaceType(DISubroutineType *Ty) {
assert(isDistinct() && "Only distinct nodes can mutate");
- replaceOperandWith(4, Ty);
+ replaceOperandWith(5, Ty);
}
- DICompileUnit *getUnit() const {
- return cast_or_null<DICompileUnit>(getRawUnit());
- }
- void replaceUnit(DICompileUnit *CU) { replaceOperandWith(5, CU); }
DITemplateParameterArray getTemplateParams() const {
return cast_or_null<MDTuple>(getRawTemplateParams());
}
@@ -1903,8 +1904,8 @@ class DISubprogram : public DILocalScope {
Metadata *getRawScope() const { return getOperand(1); }
MDString *getRawName() const { return getOperandAs<MDString>(2); }
MDString *getRawLinkageName() const { return getOperandAs<MDString>(3); }
- Metadata *getRawType() const { return getOperand(4); }
- Metadata *getRawUnit() const { return getOperand(5); }
+ Metadata *getRawUnit() const { return getOperand(4); }
+ Metadata *getRawType() const { return getOperand(5); }
Metadata *getRawDeclaration() const { return getOperand(6); }
Metadata *getRawRetainedNodes() const { return getOperand(7); }
Metadata *getRawContainingType() const {
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index 161a30dfb3828..438ac7b96f345 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1138,10 +1138,9 @@ DISubprogram *DISubprogram::getImpl(
RetainedNodes, ThrownTypes, Annotations,
TargetFuncName));
SmallVector<Metadata *, 13> Ops = {
- File, Scope, Name, LinkageName,
- Type, Unit, Declaration, RetainedNodes,
- ContainingType, TemplateParams, ThrownTypes, Annotations,
- TargetFuncName};
+ File, Scope, Name, LinkageName, Unit,
+ Type, Declaration, RetainedNodes, ContainingType, TemplateParams,
+ ThrownTypes, Annotations, TargetFuncName};
if (!TargetFuncName) {
Ops.pop_back();
if (!Annotations) {
>From 694679c7bd1987abaac9e85595b49f20d2a919b4 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Wed, 26 Jun 2024 11:32:28 +0100
Subject: [PATCH 2/6] Revert "[DebugInfo] Swap 'Unit' and 'Type' positions in
DISubprogram."
This reverts commit 40d316d0533b8412c2e797863f5353dccab54595.
---
llvm/include/llvm/IR/DebugInfoMetadata.h | 15 +++++++--------
llvm/lib/IR/DebugInfoMetadata.cpp | 7 ++++---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index a1d2f4c1791cf..524945862e8d4 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -1865,11 +1865,6 @@ class DISubprogram : public DILocalScope {
/// Only used by clients of CloneFunction, and only right after the cloning.
void replaceLinkageName(MDString *LN) { replaceOperandWith(3, LN); }
- DICompileUnit *getUnit() const {
- return cast_or_null<DICompileUnit>(getRawUnit());
- }
- void replaceUnit(DICompileUnit *CU) { replaceOperandWith(4, CU); }
-
DISubroutineType *getType() const {
return cast_or_null<DISubroutineType>(getRawType());
}
@@ -1878,9 +1873,13 @@ class DISubprogram : public DILocalScope {
}
void replaceType(DISubroutineType *Ty) {
assert(isDistinct() && "Only distinct nodes can mutate");
- replaceOperandWith(5, Ty);
+ replaceOperandWith(4, Ty);
}
+ DICompileUnit *getUnit() const {
+ return cast_or_null<DICompileUnit>(getRawUnit());
+ }
+ void replaceUnit(DICompileUnit *CU) { replaceOperandWith(5, CU); }
DITemplateParameterArray getTemplateParams() const {
return cast_or_null<MDTuple>(getRawTemplateParams());
}
@@ -1904,8 +1903,8 @@ class DISubprogram : public DILocalScope {
Metadata *getRawScope() const { return getOperand(1); }
MDString *getRawName() const { return getOperandAs<MDString>(2); }
MDString *getRawLinkageName() const { return getOperandAs<MDString>(3); }
- Metadata *getRawUnit() const { return getOperand(4); }
- Metadata *getRawType() const { return getOperand(5); }
+ Metadata *getRawType() const { return getOperand(4); }
+ Metadata *getRawUnit() const { return getOperand(5); }
Metadata *getRawDeclaration() const { return getOperand(6); }
Metadata *getRawRetainedNodes() const { return getOperand(7); }
Metadata *getRawContainingType() const {
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index 438ac7b96f345..161a30dfb3828 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1138,9 +1138,10 @@ DISubprogram *DISubprogram::getImpl(
RetainedNodes, ThrownTypes, Annotations,
TargetFuncName));
SmallVector<Metadata *, 13> Ops = {
- File, Scope, Name, LinkageName, Unit,
- Type, Declaration, RetainedNodes, ContainingType, TemplateParams,
- ThrownTypes, Annotations, TargetFuncName};
+ File, Scope, Name, LinkageName,
+ Type, Unit, Declaration, RetainedNodes,
+ ContainingType, TemplateParams, ThrownTypes, Annotations,
+ TargetFuncName};
if (!TargetFuncName) {
Ops.pop_back();
if (!Annotations) {
>From bc3bb20761c5ba568f27f956b43c537105c366e3 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Wed, 26 Jun 2024 11:54:06 +0100
Subject: [PATCH 3/6] [Verifier] Set CurrentSourceLang in visitDISubprogram
too.
When visiting metadata attached to DISubprogram, it was possible to
read `CurrentSourceLang` before `visitDICompileUnit` has a chance to set
it to proper value. This happened because `Type` is processed before
`Unit` because of the order of the metadata in DISubprogram.
My initial fix was to change the order. But @jmorse suggested setting
`CurrentSourceLang` in `visitDISubprogram` which eliminates the need
for changing the order of metadata fields.
---
llvm/lib/IR/Verifier.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index c98f61d555140..537884b967a07 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -1494,6 +1494,8 @@ void Verifier::visitDISubprogram(const DISubprogram &N) {
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 (auto *CU = dyn_cast_or_null<DICompileUnit>(Unit))
+ CurrentSourceLang = (dwarf::SourceLanguage)CU->getSourceLanguage();
// There's no good way to cross the CU boundary to insert a nested
// DISubprogram definition in one CU into a type defined in another CU.
auto *CT = dyn_cast_or_null<DICompositeType>(N.getRawScope());
>From 81d812bfe1da0b412b9a0497e83549770c02c986 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Wed, 26 Jun 2024 15:37:38 +0100
Subject: [PATCH 4/6] Add a test case.
Test checks that DISubrange without a count or upper bound entry is
accepted when source language is Fortran.
---
.../DebugInfo/X86/fortran-empty-subrange.ll | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 llvm/test/DebugInfo/X86/fortran-empty-subrange.ll
diff --git a/llvm/test/DebugInfo/X86/fortran-empty-subrange.ll b/llvm/test/DebugInfo/X86/fortran-empty-subrange.ll
new file mode 100644
index 0000000000000..edfa0b66f7bb3
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/fortran-empty-subrange.ll
@@ -0,0 +1,24 @@
+; RUN: llvm-as -disable-output %s 2>&1 | FileCheck %s --allow-empty
+
+; CHECK-NOT: Subrange must contain count or upperBound
+
+define void @fn_(ptr %0) {
+ #dbg_declare(ptr %0, !12, !DIExpression(), !13)
+ ret void
+}
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_Fortran95, file: !2, emissionKind: FullDebug)
+!2 = !DIFile(filename: "test.f90", directory: "")
+!3 = distinct !DISubprogram(scope: !2, type: !5, unit: !1)
+!5 = !DISubroutineType(cc: DW_CC_normal, types: !6)
+!6 = !{!7, !8}
+!7 = !DICompositeType(tag: DW_TAG_array_type, baseType: !8, elements: !9)
+!8 = !DIBasicType(name: "integer", size: 32, encoding: DW_ATE_signed)
+!9 = !{!11}
+!11 = !DISubrange()
+!12 = !DILocalVariable(name: "a1", arg: 1, scope: !3, file: !2, type: !7)
+!13 = !DILocation(line: 4, column: 3, scope: !3)
>From dce166aeebb05604ae0304be3eb6dfcb4b24f1f1 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Wed, 26 Jun 2024 15:49:45 +0100
Subject: [PATCH 5/6] Move test out of X86 directory.
---
llvm/test/DebugInfo/{X86 => }/fortran-empty-subrange.ll | 0
1 file changed, 0 insertions(+), 0 deletions(-)
rename llvm/test/DebugInfo/{X86 => }/fortran-empty-subrange.ll (100%)
diff --git a/llvm/test/DebugInfo/X86/fortran-empty-subrange.ll b/llvm/test/DebugInfo/fortran-empty-subrange.ll
similarity index 100%
rename from llvm/test/DebugInfo/X86/fortran-empty-subrange.ll
rename to llvm/test/DebugInfo/fortran-empty-subrange.ll
>From add361fc3422a9487759e909944e6de09d91031d Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Fri, 28 Jun 2024 11:23:23 +0100
Subject: [PATCH 6/6] Remove check that Subrange must have count or upper
bound.
Also removed related tests.
---
llvm/lib/IR/Verifier.cpp | 13 ----------
.../invalid-disubrange-count-missing.ll | 5 ----
llvm/test/DebugInfo/fortran-empty-subrange.ll | 24 -------------------
.../digenericsubrange-missing-upperBound.ll | 5 ----
.../Verifier/disubrange-missing-upperBound.ll | 5 ----
5 files changed, 52 deletions(-)
delete mode 100644 llvm/test/Assembler/invalid-disubrange-count-missing.ll
delete mode 100644 llvm/test/DebugInfo/fortran-empty-subrange.ll
delete mode 100644 llvm/test/Verifier/digenericsubrange-missing-upperBound.ll
delete mode 100644 llvm/test/Verifier/disubrange-missing-upperBound.ll
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 537884b967a07..cb55601aec703 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -359,9 +359,6 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
/// Whether the current function has a DISubprogram attached to it.
bool HasDebugInfo = false;
- /// The current source language.
- dwarf::SourceLanguage CurrentSourceLang = dwarf::DW_LANG_lo_user;
-
/// Stores the count of how many objects were passed to llvm.localescape for a
/// given function and the largest index passed to llvm.localrecover.
DenseMap<Function *, std::pair<unsigned, unsigned>> FrameEscapeInfo;
@@ -1156,10 +1153,6 @@ void Verifier::visitDIScope(const DIScope &N) {
void Verifier::visitDISubrange(const DISubrange &N) {
CheckDI(N.getTag() == dwarf::DW_TAG_subrange_type, "invalid tag", &N);
- bool HasAssumedSizedArraySupport = dwarf::isFortran(CurrentSourceLang);
- CheckDI(HasAssumedSizedArraySupport || N.getRawCountNode() ||
- N.getRawUpperBound(),
- "Subrange must contain count or upperBound", &N);
CheckDI(!N.getRawCountNode() || !N.getRawUpperBound(),
"Subrange can have any one of count or upperBound", &N);
auto *CBound = N.getRawCountNode();
@@ -1188,8 +1181,6 @@ void Verifier::visitDISubrange(const DISubrange &N) {
void Verifier::visitDIGenericSubrange(const DIGenericSubrange &N) {
CheckDI(N.getTag() == dwarf::DW_TAG_generic_subrange, "invalid tag", &N);
- CheckDI(N.getRawCountNode() || N.getRawUpperBound(),
- "GenericSubrange must contain count or upperBound", &N);
CheckDI(!N.getRawCountNode() || !N.getRawUpperBound(),
"GenericSubrange can have any one of count or upperBound", &N);
auto *CBound = N.getRawCountNode();
@@ -1413,8 +1404,6 @@ void Verifier::visitDICompileUnit(const DICompileUnit &N) {
CheckDI(!N.getFile()->getFilename().empty(), "invalid filename", &N,
N.getFile());
- CurrentSourceLang = (dwarf::SourceLanguage)N.getSourceLanguage();
-
CheckDI((N.getEmissionKind() <= DICompileUnit::LastEmissionKind),
"invalid emission kind", &N);
@@ -1494,8 +1483,6 @@ void Verifier::visitDISubprogram(const DISubprogram &N) {
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 (auto *CU = dyn_cast_or_null<DICompileUnit>(Unit))
- CurrentSourceLang = (dwarf::SourceLanguage)CU->getSourceLanguage();
// There's no good way to cross the CU boundary to insert a nested
// DISubprogram definition in one CU into a type defined in another CU.
auto *CT = dyn_cast_or_null<DICompositeType>(N.getRawScope());
diff --git a/llvm/test/Assembler/invalid-disubrange-count-missing.ll b/llvm/test/Assembler/invalid-disubrange-count-missing.ll
deleted file mode 100644
index 8b7bf713a8e91..0000000000000
--- a/llvm/test/Assembler/invalid-disubrange-count-missing.ll
+++ /dev/null
@@ -1,5 +0,0 @@
-; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
-
-!named = !{!0}
-; CHECK: Subrange must contain count or upperBound
-!0 = !DISubrange(lowerBound: -3)
diff --git a/llvm/test/DebugInfo/fortran-empty-subrange.ll b/llvm/test/DebugInfo/fortran-empty-subrange.ll
deleted file mode 100644
index edfa0b66f7bb3..0000000000000
--- a/llvm/test/DebugInfo/fortran-empty-subrange.ll
+++ /dev/null
@@ -1,24 +0,0 @@
-; RUN: llvm-as -disable-output %s 2>&1 | FileCheck %s --allow-empty
-
-; CHECK-NOT: Subrange must contain count or upperBound
-
-define void @fn_(ptr %0) {
- #dbg_declare(ptr %0, !12, !DIExpression(), !13)
- ret void
-}
-
-!llvm.module.flags = !{!0}
-!llvm.dbg.cu = !{!1}
-
-!0 = !{i32 2, !"Debug Info Version", i32 3}
-!1 = distinct !DICompileUnit(language: DW_LANG_Fortran95, file: !2, emissionKind: FullDebug)
-!2 = !DIFile(filename: "test.f90", directory: "")
-!3 = distinct !DISubprogram(scope: !2, type: !5, unit: !1)
-!5 = !DISubroutineType(cc: DW_CC_normal, types: !6)
-!6 = !{!7, !8}
-!7 = !DICompositeType(tag: DW_TAG_array_type, baseType: !8, elements: !9)
-!8 = !DIBasicType(name: "integer", size: 32, encoding: DW_ATE_signed)
-!9 = !{!11}
-!11 = !DISubrange()
-!12 = !DILocalVariable(name: "a1", arg: 1, scope: !3, file: !2, type: !7)
-!13 = !DILocation(line: 4, column: 3, scope: !3)
diff --git a/llvm/test/Verifier/digenericsubrange-missing-upperBound.ll b/llvm/test/Verifier/digenericsubrange-missing-upperBound.ll
deleted file mode 100644
index 72f0aed264937..0000000000000
--- a/llvm/test/Verifier/digenericsubrange-missing-upperBound.ll
+++ /dev/null
@@ -1,5 +0,0 @@
-; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
-
-!named = !{!0}
-; CHECK: GenericSubrange must contain count or upperBound
-!0 = !DIGenericSubrange(lowerBound: !DIExpression(DW_OP_push_object_address, DW_OP_over, DW_OP_constu, 48, DW_OP_mul, DW_OP_plus_uconst, 80, DW_OP_plus, DW_OP_deref), stride: !DIExpression(DW_OP_push_object_address, DW_OP_over, DW_OP_constu, 48, DW_OP_mul, DW_OP_plus_uconst, 112, DW_OP_plus, DW_OP_deref, DW_OP_constu, 4, DW_OP_mul))
diff --git a/llvm/test/Verifier/disubrange-missing-upperBound.ll b/llvm/test/Verifier/disubrange-missing-upperBound.ll
deleted file mode 100644
index 26b707caa6093..0000000000000
--- a/llvm/test/Verifier/disubrange-missing-upperBound.ll
+++ /dev/null
@@ -1,5 +0,0 @@
-; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
-
-!named = !{!0}
-; CHECK: Subrange must contain count or upperBound
-!0 = !DISubrange(lowerBound: 1, stride: 4)
More information about the llvm-commits
mailing list