[llvm] [DebugInfo] Swap 'Unit' and 'Type' positions in DISubprogram. (PR #96474)
Abid Qadeer via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 26 07:50:21 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/5] [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/5] 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/5] [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/5] 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/5] 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
More information about the llvm-commits
mailing list