[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 04:25:15 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/3] [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 524945862e8d42..a1d2f4c1791cfd 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 161a30dfb38288..438ac7b96f3453 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/3] 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 a1d2f4c1791cfd..524945862e8d42 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 438ac7b96f3453..161a30dfb38288 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/3] [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 c98f61d5551408..537884b967a074 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());
More information about the llvm-commits
mailing list