Page MenuHomePhabricator

Adding debug info to support Fortran (part 1)
Needs ReviewPublic

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

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
57

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.