Page MenuHomePhabricator

[DebugInfo] DISubrange support for fortran assumed size array
ClosedPublic

Authored by alok on Fri, Sep 11, 2:33 AM.

Details

Summary
Summary:

This is needed to support assumed size array of fortran which can have missing upperBound/count,
contrary to current DISubrange support.

  Example:
  subroutine sub (array1, array2)
    integer :: array1 (*)
    integer :: array2 (4:9, 10:*)

    array1(7:8) = 9
    array2(5, 10) = 10
  end subroutine
  Now the validation check is relaxed for the Fortran.

Testing:
  • check-llvm
  • check-debuginfo

Diff Detail

Event Timeline

alok created this revision.Fri, Sep 11, 2:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
alok requested review of this revision.Fri, Sep 11, 2:33 AM
aprantl added inline comments.Fri, Sep 11, 8:30 AM
llvm/lib/IR/Verifier.cpp
906

Can you add an isFortran helper function to Dwarf.h?

alok marked an inline comment as done.Fri, Sep 11, 10:54 AM
alok added inline comments.
llvm/lib/IR/Verifier.cpp
906

Thanks. I shall do that in next patch.

alok updated this revision to Diff 291273.Fri, Sep 11, 10:59 AM
alok marked an inline comment as done.

Updated to incorporate @aprantl 's comment.

aprantl added inline comments.Fri, Sep 11, 4:14 PM
llvm/include/llvm/BinaryFormat/Dwarf.h
349

This would trigger an unreachable for the interval (DW_LANG_lo_user, DW_LANG_hi_user). Why not just say default: false instead of trying to enumerate all non-Fortran languages?

alok marked an inline comment as done.Fri, Sep 11, 11:35 PM
alok added inline comments.
llvm/include/llvm/BinaryFormat/Dwarf.h
349

It was taken from function "isCPlusPlus", but I agree with you that it is better to return false for default and dont depend on llvm_unreachable to get assistance in adding new language enums. I shall update the patch.

alok updated this revision to Diff 291375.Fri, Sep 11, 11:36 PM
alok marked an inline comment as done.

Updated to incorporate comment from @aprantl .

I see

llvm/include/llvm/BinaryFormat/Dwarf.h
349

Oh, I see what you mean now! Unfortunately I consider isCplusPlus() to be well-intentioned but broken, too. We can't have an llvm_unreachable() on perfectly valid input. A compiler frontend using LLVM as a backend, or a LLDB loading a third-party binary must not crash when encountering a user constant.

To clarify: I think the idea that isCplusPlus uses to enumerate all constants to get the switch coverage warning is good and I'm fine with keeping the long explicit switch. We just cannot allow an llvm_unreachable for valid (or invalid!) inputs. This function is used by LLDB and you don't want the debugger to crash when loading a third-party object file.

alok marked an inline comment as done.Sat, Sep 12, 11:07 AM
alok added inline comments.
llvm/include/llvm/BinaryFormat/Dwarf.h
349

Thanks for your suggestion, I shall keep the long explicit switch without llvm_unreachable.

alok marked an inline comment as done.Sat, Sep 12, 11:26 AM
alok updated this revision to Diff 291412.Sat, Sep 12, 11:46 AM

Incorporated comment from @aprantl .

aprantl accepted this revision.Tue, Sep 15, 6:17 PM

Thank you!

llvm/include/llvm/BinaryFormat/Dwarf.h
295–296

Would you mind doing the same change here, too? There's no need start to another review for that.

This revision is now accepted and ready to land.Tue, Sep 15, 6:17 PM
alok added a comment.EditedWed, Sep 16, 1:57 AM

Thanks @aprantl for reviewing this. Any idea how to backport it and other fortran related commits to older version of LLVM (11, 10, 9) ?