[DebugInfo] Allow DISubrange/DIGenericSubrange without count/upperBound. #96474
Conversation
abidh
requested review from
echristo,
cchen15,
adrian-prantl,
dpalermo,
dwblaikie and
jinisusan
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: Abid Qadeer (abidh) ChangesDue to the current order of metadata in DISubprgram, when compiled with clang, the control reaches This behavior does not effect C like language much but is a problem for Fortran. There is special processing in To fix this, I have swapped the position of Full diff: https://github.com/llvm/llvm-project/pull/96474.diff 2 Files Affected:
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) {
|
|
As far as I understand this patch, it's swapping which field of a DISubprogram the data is stored in, and all the accessors are updated, so this patch should be transparent to any code using the accessors. Thus, surely the problem is caused by code that directly accesses the fields of DISubprogram and is exposed to their ordering -- shouldn't such code instead examine fields in a logical manner rather than the order of storage? |
The caller here is |
|
Most curious; my understanding of [0] llvm-project/llvm/lib/IR/Verifier.cpp Line 1058 in 6481dc5 |
Problem comes from the loop a few lines below [0]. It calls the visitMDNode on operands in order. You can reproduce it using following code. You will see that control reaches [0] llvm-project/llvm/lib/IR/Verifier.cpp Line 1063 in 6481dc5 |
|
Aaaaahhh, I get it now, thanks for bearing with me -- I see that the I still feel flipping the order of fields is fixing the symptom rather than the cause -- stepping back a bit, by having state in the Verifier object we're implicitly requiring metadata to be explored in a particular order. This patch will fix one incorrect order, but there might be other incorrect orders or it might expose more. Instead: how about setting CurrentSourceLang in I get the feeling that the state in |
Thanks for taking a look. Setting |
|
In principle this LGTM, perhaps other reviewers could chime in though. In an ideal world we'd have a test that ensures this behaviour continues to be present, is there an LLVM-IR input from flang that triggers the previous error, and that we can have in the LLVM DebugInfo test suite? |
I have added a test. |
Yeah, bit janky - I don't totally object. But alternatively: Could we remove the use/need for CurrentSourceLang? Guess it was added for fortran support, but perhaps we should've gone the toher way and weakened the checks? Just don't check for count/upperbound at all? (in visitDISubrange) Presumably LLVM can produce whatever DWARF we do for Fortran given such info, and do that for C++ too - even if it's incorrect for C++, maybe we move that to just be up to the IR generator to get that right? |
I did a bit of digging and it seems that those checks for count/upperBound were added for Fortran's allocatable array in https://reviews.llvm.org/D80197. Later they had to be modified for Fortran's assumed size arrays in https://reviews.llvm.org/D87500. So the checks were added for a Fortran feature and later had to be disabled for Fortran. Removing/weakening them seems reasonable to me. Please let me know if you prefer this solution. |
|
Yeah, lets weaken the verification here - if LLVM's DWARF generation can succeed (not crash), I think that's all we should care about. If someone makes a DISubrange with an upper bound lower than the lower bound (implicit or explicit) - shrug garbage in, garbage out. I don't think it's worth trying to save them from themselves with the IR verifier. |
|
(might be worth checking where the earlier version of the check was (before fortran - looks like it just checked the upper bound was >= 0) - and if the author is still around, cc them on this in case they want to protest that checking that is really important or something) |
I have removed the check that Subrange/GenericSubrange must contain count or upper bound. I have also removed From your comments, I guess that you also wanted to remove the |
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.
This reverts commit 40d316d.
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.
Test checks that DISubrange without a count or upper bound entry is accepted when source language is Fortran.
Also removed related tests.
|
Yeah... @alokkrsharma in case they've got thoughts on this issue/changes... |
|
This change will need test coverage - it might be worth keeping the existing tests around and turning them into code generation tests, to check we can generate DWARF even when we violate these verifier checks? |
I have added a test that checks that debug info is generated correctly when these conditions are violated. The test covers the original motivating case that compilation does not fail for things like |
I'll have a look and get back on this. Thanks. |
|
This LGTM. Initially (Before enhancements for the Fortran debugging) Verifier had a restriction for DISubrange to have count as REQUIRED field. As languages do have default values for lowerBound, count explicitly gives number of elements and upper bound can be calculated (upper - lower +1). Removing restriction completely in place of relaxing would make any language (like Fortran) with prosperous arrays happy. I agree with the point that back-end should have common/minimum restrictions which applies to all the languages, any extra restriction should be moved to respective front-ends if needed. Thanks. |
abidh
changed the title
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/1378 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/1377 Here is the relevant piece of the build log for the reference: |
|
I am adding |
The test added in #96474 seems to cause buildbot failure on some system. I have added `REQUIRES: object-emission` in the test and also used `%t` instead of `-` for output.
…nd. (llvm#96474) Due to the current order of metadata in DISubprgram, `Type` is processed before `Unit` by the Verifier. This can cause a race and use of garbage data. Consider the following code: ``` int test(int a[][5]) { return a[0][2]; } ``` when compiled with clang, the control reaches `Verifier::visitDISubrange` first with `CurrentSourceLang` still equal to dwarf::DW_LANG_lo_user (32768). The `Verifier::visitDICompileUnit` which sets the value of `CurrentSourceLang` is reached later. So `Verifier::visitDISubrange` ends up using a wrong value of `CurrentSourceLang`. 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. Various solutions were tried to solve this problem before it was decided that best course of action is to remove these checks from Verifier.
The test added in llvm#96474 seems to cause buildbot failure on some system. I have added `REQUIRES: object-emission` in the test and also used `%t` instead of `-` for output.
Due to the current order of metadata in DISubprgram,
Typeis processed beforeUnitby the Verifier. This can cause a race anduse of garbage data. Consider the following code:
when compiled with clang, the control reaches
Verifier::visitDISubrangefirst withCurrentSourceLangstill equal to dwarf::DW_LANG_lo_user (32768). TheVerifier::visitDICompileUnitwhich sets the value ofCurrentSourceLangis reached later. SoVerifier::visitDISubrangeends up using a wrong value ofCurrentSourceLang.This behavior does not effect C like language much but is a problem for Fortran. There is special processing in
Verifier::visitDISubrangewhenCurrentSourceLangis 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
TypeandUnit. They were already adjacent so it does not require changing position of anything else.