Page MenuHomePhabricator

Adding debug info to support Fortran (part 1)
ClosedPublic

Authored by schweitz on Nov 2 2018, 11:55 AM.

Details

Summary
  1. Elemental, Pure, and Recursive Procedures

DWARF 4 defines attributes for these Fortran procedure specifiers: DW_AT_elemental, DW_AT_pure, DW_AT_recursive, resp. LLVM has a way of informing the DWARF generator of simple boolean attributes in the metadata via the flags parameter. We have added these new values to the existing collection of flags.

!60 = !DISubprogram(…, flags: DIFlagElemental)
!61 = !DISubprogram(…, flags: DIFlagPure)
!62 = !DISubprogram(…, flags: DIFlagRecursive)

Diff Detail

Repository
rL LLVM

Event Timeline

schweitz created this revision.Nov 2 2018, 11:55 AM

Procedural point: When you upload a diff to Phabricator, please include more context (-U999999 is typical).

I really don't like overloading DIFlags this way. I still think a separate subprogram-flags word is a better approach; it will be more effort to get it in place and make bitcode serialization work with it, but the end result would be a lot cleaner.

probinson added inline comments.Nov 2 2018, 3:14 PM
include/llvm/IR/DebugInfoFlags.def
62 ↗(On Diff #172410)

There are equivalents of the flag symbols in the C API, so you'd want to add the new ones there also; see include/llvm-c/IR/DebugInfo.h.

fhahn added a subscriber: fhahn.Nov 4 2018, 8:07 AM
schweitz updated this revision to Diff 172605.Nov 5 2018, 9:23 AM

Add the new flags to the C API.

While I normally would recommend NOT encoding the language name in the constants (I'll be making that suggestion in "part 2" in just a bit), these symbols are clearly Fortran-only (you OR in bit DIFlagFortran 30). The text in the DWARF4 standard also makes the same case but doesn't come out and say Fortran-only but just "These attributes are not relevant for languages that do not support similar keywords or syntax." So should they be named DIFlagFortranPure, DIFlagFortranElemental, DIFlagFortranRecursive or is that too heavy handed?

I'm surprised to see no patch to the IR asm writer. Why is DIFlagRecursive not accidentally printed as DIFlagBitfield and vice versa?

include/llvm/IR/DebugInfoFlags.def
58 ↗(On Diff #172605)

Bit 15 really is unused. Feel free to use it instead for DIFlagFortran.

Please note that D also has the pure attribute on functions, so I would like the pure flag to not be Fortran-specific such that we can use it for D too. Thanks!

Even when DWARF adds something in response to the needs of a specific language, it doesn't use language names in the tags or attributes. So, I think putting "Fortran" in the symbol names here would be inconsistent with that. It's fine to describe it as a Fortran attribute in commentary.
As a hypothetical example, C++ has the concept of a "pure" virtual method declaration, and somebody might come up with a reason to mark those with the new Pure flag.
Which is another reason not to start overloading the existing flags based on assumptions about which languages use which flags. I've started sketching out what it would take to add a separate DISPFlags type/field in DISubprogram.

schweitz updated this revision to Diff 172777.Nov 6 2018, 9:18 AM

Use bit 15 as the "extends" bit and free up bit 30.

Also remove the "F word" (Fortran).

Please note that D also has the pure attribute on functions, so I would like the pure flag to not be Fortran-specific such that we can use it for D too. Thanks!

I don't see any technical reason that this changeset's DIFlagPure cannot be used with another language. (The changes were merely done for Fortran compilers.)

Please note that D also has the pure attribute on functions, so I would like the pure flag to not be Fortran-specific such that we can use it for D too. Thanks!

I don't see any technical reason that this changeset's DIFlagPure cannot be used with another language. (The changes were merely done for Fortran compilers.)

Yep, I meant the use of "Fortran" in the naming. Thanks for changing that.

Looks like this could use an LLVM CodeGen test too (similar to, or extending/renaming test/DebugInfo/Generic/mainsubprogram.ll)

schweitz updated this revision to Diff 174425.Nov 16 2018, 12:07 PM

Added an assembler test.

Looks like this could use an LLVM CodeGen test too (similar to, or extending/renaming test/DebugInfo/Generic/mainsubprogram.ll)

Thanks for the pointer. I've now added a test for the 3 new DW_AT_* values.

schweitz updated this revision to Diff 174430.Nov 16 2018, 12:36 PM

minor fix to a comment in the new test

I believe all the comments have been addressed. Is there anything else required on our end?

I believe all the comments have been addressed. Is there anything else required on our end?

I have a review in progress that will let you use subprogram-specific flags, instead of the general DINode flags. As the new Fortran flags are specific to subprograms, I think that's a much better place to put them, as we are running low on the general flags.

Please wait for D54755 to land, and then update to use those instead.

Thanks, Paul.

Or this could go in first and supply additional tests for your new bit format. :)

For what it's worth, the overloaded bit-encoding follows the precedent already established by LLVMDIFlagIndirectVirtualBase.

Thanks, Paul.

Or this could go in first and supply additional tests for your new bit format. :)

For what it's worth, the overloaded bit-encoding follows the precedent already established by LLVMDIFlagIndirectVirtualBase.

Sorry that it's holding you up, but if it's all the same to you, I'd rather put off the ecstasy of moving an existing flag from one word to the other.
I don't view DIFlagIndirectVirtualBase as a *good* precedent, either.

I agree that it would be cleaner to rebase this on D54755. Bitcode format changes are unfortunately sort of expensive because of the required forwards compatibility...

My patches have landed, and look like they'll stick; please rebase and use the DISubprogram specific flags for the new Fortran flags.

schweitz added a comment.EditedMar 7 2019, 9:52 AM

Noting that porting required both D54597 and D54755

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 9:52 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
schweitz updated this revision to Diff 189865.Mar 8 2019, 8:52 AM

Rework of D54043 to use the DISPFlag* enum added afterward.

FWIW:
In porting, I discovered that mixing the old metadata syntax with the new DISPFlag values is problematic (or, at least, doesn't give a very clear error message). Specifically, if one has a 'isDefinition: true' combined with 'spFlags: DISPFlagRecursive', it will cause the IR Verifier to fail. Using 'spFlags: DISPFlagDefinition | DISPFlagRecursive' and omitting 'isDefinition:' got around this issue.

schweitz updated this revision to Diff 189866.Mar 8 2019, 8:58 AM

Remove note from code.

schweitz updated this revision to Diff 189867.Mar 8 2019, 9:05 AM

A little more cleanup of our code to minimize the exact change.

ormris removed a subscriber: ormris.Mar 8 2019, 9:22 AM

I'd be inclined to add the serialize/deserialize test to an existing test file, rather than a new test.
Similarly for the flag->attribute test.

test/DebugInfo/Generic/fortran-subprogram-at.ll
2 ↗(On Diff #189867)

s/DIFlag/DISPFlag/ in the comments. Also might be worth noting that the test is for serializing/de-serializing the flags (as opposed to the semantic effect).

test/DebugInfo/Generic/more-subprogram-attr.ll
13 ↗(On Diff #189867)

Maybe make these three use CHECK-DAG?
Then add CHECK: {{DW_TAG|NULL}} after, which ensures they're all attached to the same DIE.

Arguably the test should verify each flag individually, i.e. on separate functions, but I'll leave that up to you.

schweitz updated this revision to Diff 189937.Mar 8 2019, 2:21 PM

Merged test files into one. Also deploy CHECK-DAG.

aprantl added inline comments.Mar 11 2019, 10:27 AM
test/DebugInfo/Generic/fortran-subprogram-attr.ll
9 ↗(On Diff #189937)

We usually have the round-trip test in test/Assembler, so it also gets run without object-emission.

schweitz marked an inline comment as done.Mar 11 2019, 11:05 AM
schweitz added inline comments.
test/DebugInfo/Generic/fortran-subprogram-attr.ll
9 ↗(On Diff #189937)

Can someone tell us exactly which file(s) to put what test(s) in? Thanks.

schweitz marked an inline comment as not done.Mar 11 2019, 11:31 AM
aprantl added inline comments.Mar 11 2019, 12:24 PM
test/DebugInfo/Generic/fortran-subprogram-attr.ll
9 ↗(On Diff #189937)

You can either add a new file or perhaps append it to llvm/test/Assembler/debug-info.ll.

9 ↗(On Diff #189937)

The second half of the test can stay here.

schweitz added inline comments.Mar 12 2019, 9:27 AM
test/DebugInfo/Generic/fortran-subprogram-attr.ll
9 ↗(On Diff #189937)

Thank you, Adrian.

schweitz added inline comments.Mar 12 2019, 9:28 AM
test/DebugInfo/Generic/fortran-subprogram-attr.ll
9 ↗(On Diff #189937)

It looks like test/Assembler/disubprogram.ll is a really good place as well. It appears to be testing the DISPFlags.

schweitz updated this revision to Diff 190280.Mar 12 2019, 9:34 AM

Split tests back into 2 files

aprantl accepted this revision.Mar 12 2019, 1:32 PM

LGTM, however it would be important to also document the new flags in docs/SourceLevelDebugging.rst or LangRef.rst.

This revision is now accepted and ready to land.Mar 12 2019, 1:32 PM

Thank you.

Noting that I do not have commit access.

If you update the patch with the documentation, I'd be happy to commit it for you!

If you update the patch with the documentation, I'd be happy to commit it for you!

OK, thanks for that. I wasn't sure if you were expecting a separate patch or what.

schweitz updated this revision to Diff 190544.Mar 13 2019, 4:57 PM

added documentation blurb to SourceLevelDebugging.rst

This revision was automatically updated to reflect the committed changes.