Page MenuHomePhabricator

[DebugInfo] Add handling of stringLengthExp operand of DIStringType
ClosedPublic

Authored by cchen15 on Dec 1 2020, 11:19 AM.

Details

Summary

This patch makes DWARF writer emit DW_AT_string_length using the stringLengthExp operand of DIStringType.

This is part of the effort to add debug info support for Fortran deferred length strings.

Also updated the tests to exercise the change.

Diff Detail

Event Timeline

cchen15 created this revision.Dec 1 2020, 11:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 11:19 AM
cchen15 requested review of this revision.Dec 1 2020, 11:19 AM
cchen15 added inline comments.Dec 1 2020, 11:21 AM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
696

Can we consolidate stringLength and stringLengthExp into one operand? It seems to me that a DIStringType can have one or the other, but not both.

aprantl added inline comments.Dec 2 2020, 5:57 PM
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
702

This would benefit from a comment explaining why forcing MemoryLocationKind is correct here.

llvm/test/DebugInfo/X86/distringtype.ll
12

can you check the contents of the DW_AT_string_length attribute, too?

Thanks @cchen15 for the patch! It seems like you're also adding support for FORTRAN deferred length string would you please make appropriate changes in summary to reflect the intent.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
696

Thanks! makes sense in general. (As a side note, just please make sure we don't accidentally mess/touch up exisiting assumed length string support).

702

+1 Please add a comment.

In FORTRAN deferred length string; string length is not known, that's why memory location I suppose ?
i.e
character(len=:), allocatable :: deferred, this will be allocated later.
allocate(character(10)::deferred)

llvm/test/DebugInfo/X86/distringtype.ll
17

NIT: Since the test case also contains FORTRAN deferred length string too.
Please update the comment like or
....assumed length and deferred length string type.

19

NIT: missing`=`
character(len=:), allocatable :: deferred

llvm/test/DebugInfo/fortran-string-type.ll
30

NIT: Probably editor related ?

cchen15 updated this revision to Diff 309286.Dec 3 2020, 9:34 AM
cchen15 edited the summary of this revision. (Show Details)

Made changes based on the review comments.

cchen15 marked 5 inline comments as done.Dec 3 2020, 9:38 AM
cchen15 added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
696

Thanks for the feedback. I plan to add data_location operand to DIStringType in the next couple of weeks. I will include in that patch the consolidation of stringLength and stringLengthExp.

SouraVX accepted this revision.Dec 3 2020, 9:59 PM

LGTM Thanks! Please wait for @aprantl for any comments/approval.

This revision is now accepted and ready to land.Dec 3 2020, 9:59 PM
aprantl accepted this revision.Dec 4 2020, 11:41 AM

lgtm

cchen15 added a subscriber: cchen15.Dec 7 2020, 3:45 AM
This comment was removed by cchen15.