Page MenuHomePhabricator

[DebugInfo] Fixing CodeView assert related to lowerBound field of DISubrange.
ClosedPublic

Authored by alok on Wed, Sep 9, 11:45 AM.

Details

Summary
This is to fix CodeView build failure https://bugs.llvm.org/show_bug.cgi?id=47287
after DIsSubrange upgrade D80197

Assert condition is now removed and Count is calculated in case LowerBound
is absent or zero and Count or UpperBound is constant. If Count is unknown
it is later handled as VLA (currently Count is set to zero).

Diff Detail

Event Timeline

alok created this revision.Wed, Sep 9, 11:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 9, 11:45 AM
alok requested review of this revision.Wed, Sep 9, 11:45 AM
rnk added a subscriber: rnk.Thu, Sep 10, 9:19 AM
rnk added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1581–1582

I think we should handle the presence of anything we don't understand (non-zero lower bound, stride, and upper bound) as a VLA. As in, translate it to an array count of -1. That's how we can eliminate the assertion. If the user tries to emit codeview when compiling fortran with these types of arrays, they would probably prefer to have an array bound of zero in their debug info than for the compiler to crash.

I should've provided that guidance during the original review when this code landed. The author was trying to be cautious.

alok added a reviewer: rnk.Thu, Sep 10, 1:13 PM
alok marked an inline comment as done.
alok added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1581–1582

Thanks for your comment. I shall update the patch and request you to have a look at updated patch and share your feedback.

alok updated this revision to Diff 291072.Thu, Sep 10, 1:43 PM
alok marked an inline comment as done.
alok edited the summary of this revision. (Show Details)
dmajor added a subscriber: dmajor.Thu, Sep 10, 1:54 PM
rnk accepted this revision.Thu, Sep 10, 2:07 PM

I would ask you to add a test, but I can't recommend the existing test, llvm/test/DebugInfo/COFF/types-array.ll, as a good basis for a test. I'll look into rewriting it and adding coverage after you land.

This revision is now accepted and ready to land.Thu, Sep 10, 2:07 PM
alok added a comment.Thu, Sep 10, 10:48 PM
In D87406#2266722, @rnk wrote:

I would ask you to add a test, but I can't recommend the existing test, llvm/test/DebugInfo/COFF/types-array.ll, as a good basis for a test. I'll look into rewriting it and adding coverage after you land.

Thanks a lot for all your guidance and review.