Adding debug info to support Fortran (part 1)

[schweitz]

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)

[probinson]

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]

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.

[schweitz]

Add the new flags to the C API.

[JohnReagan]

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?

[aprantl]

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.

[johanengelen]

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!

[probinson]

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]

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

Also remove the "F word" (Fortran).

[schweitz]

In D54043#1288590, @johanengelen wrote:

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.)

[johanengelen]

In D54043#1288888, @schweitz wrote:

In D54043#1288590, @johanengelen wrote:

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.

[dblaikie]

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

[schweitz]

Added an assembler test.

[schweitz]

In D54043#1296240, @dblaikie wrote:

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]

minor fix to a comment in the new test

[schweitz]

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

[probinson]

In D54043#1310116, @schweitz wrote:

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.

[schweitz]

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.

[probinson]

In D54043#1310486, @schweitz wrote:

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.

[aprantl]

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...

[probinson]

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

[schweitz]

Noting that porting required both D54597 and D54755

[schweitz]

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]

Remove note from code.

[schweitz]

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

[probinson]

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]

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

[aprantl]

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]

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.

[aprantl]

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]

test/DebugInfo/Generic/fortran-subprogram-attr.ll

9 ↗

(On Diff #189937)

Thank you, Adrian.

[schweitz]

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]

Split tests back into 2 files

[aprantl]

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

[schweitz]

Thank you.

Noting that I do not have commit access.

[aprantl]

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

[schweitz]

In D54043#1426875, @aprantl wrote:

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]

added documentation blurb to SourceLevelDebugging.rst

llvm/trunk/docs/SourceLevelDebugging.rst

.. code-block:: text
 
   ;;
   ;; Define the subprogram itself.
   ;;
   define i32 @main(i32 %argc, i8** %argv) !dbg !4 {
   ...
   }
 
+Fortran specific debug information
+==================================
+
+Fortran function information
+----------------------------
+
+There are a few DWARF attributes defined to support client debugging of Fortran programs.  LLVM can generate (or omit) the appropriate DWARF attributes for the prefix-specs of ELEMENTAL, PURE, IMPURE, RECURSIVE, and NON_RECURSIVE.  This is done by using the spFlags values: DISPFlagElemental, DISPFlagPure, and DISPFlagRecursive.
+
+.. code-block:: fortran
+
+  elemental function elem_func(a)
+
+a Fortran front-end would generate the following descriptors:
+
+.. code-block:: text
+
+  !11 = distinct !DISubprogram(name: "subroutine2", scope: !1, file: !1,
+          line: 5, type: !8, scopeLine: 6,
+          spFlags: DISPFlagDefinition | DISPFlagElemental, unit: !0,
+          retainedNodes: !2)
+
+and this will materialize an additional DWARF attribute as:
+
+.. code-block:: text
+
+  DW_TAG_subprogram [3]  
+     DW_AT_low_pc [DW_FORM_addr]     (0x0000000000000010 ".text")
+     DW_AT_high_pc [DW_FORM_data4]   (0x00000001)
+     ...
+     DW_AT_elemental [DW_FORM_flag_present]  (true)
+
 Debugging information format
 ============================
 
 Debugging Information Extension for Objective C Properties
 ----------------------------------------------------------
 
 Introduction
 ^^^^^^^^^^^^

llvm/trunk/include/llvm/IR/DebugInfoFlags.def

 HANDLE_DISP_FLAG(0, Zero)
 // Virtuality is a two-bit enum field in the LSB of the word.
 // Values should match DW_VIRTUALITY_*.
 HANDLE_DISP_FLAG(1u, Virtual)
 HANDLE_DISP_FLAG(2u, PureVirtual)
 HANDLE_DISP_FLAG((1u << 2), LocalToUnit)
 HANDLE_DISP_FLAG((1u << 3), Definition)
 HANDLE_DISP_FLAG((1u << 4), Optimized)
+HANDLE_DISP_FLAG((1u << 5), Pure)
+HANDLE_DISP_FLAG((1u << 6), Elemental)
+HANDLE_DISP_FLAG((1u << 7), Recursive)
 
 #ifdef DISP_FLAG_LARGEST_NEEDED
 // Intended to be used with ADT/BitmaskEnum.h.
 // NOTE: Always must be equal to largest flag, check this when adding new flags.
-HANDLE_DISP_FLAG((1 << 4), Largest)
+HANDLE_DISP_FLAG((1 << 7), Largest)
 #undef DISP_FLAG_LARGEST_NEEDED
 #endif
 
 #undef HANDLE_DI_FLAG
 #undef HANDLE_DISP_FLAG

llvm/trunk/include/llvm/IR/DebugInfoMetadata.h

bool isPublic() const {
     return (getFlags() & FlagAccessibility) == FlagPublic;
   }
   bool isExplicit() const { return getFlags() & FlagExplicit; }
   bool isPrototyped() const { return getFlags() & FlagPrototyped; }
   bool areAllCallsDescribed() const {
     return getFlags() & FlagAllCallsDescribed;
   }
   bool isMainSubprogram() const { return getFlags() & FlagMainSubprogram; }
+  bool isPure() const { return getSPFlags() & SPFlagPure; }
+  bool isElemental() const { return getSPFlags() & SPFlagElemental; }
+  bool isRecursive() const { return getSPFlags() & SPFlagRecursive; }
 
   /// Check if this is reference-qualified.
   ///
   /// Return true if this subprogram is a C++11 reference-qualified non-static
   /// member function (void foo() &).
   bool isLValueReference() const { return getFlags() & FlagLValueReference; }
 
   /// Check if this is rvalue-reference-qualified.

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

else if (SP->isPublic())
     addUInt(SPDie, dwarf::DW_AT_accessibility, dwarf::DW_FORM_data1,
             dwarf::DW_ACCESS_public);
 
   if (SP->isExplicit())
     addFlag(SPDie, dwarf::DW_AT_explicit);
 
   if (SP->isMainSubprogram())
     addFlag(SPDie, dwarf::DW_AT_main_subprogram);
+  if (SP->isPure())
+    addFlag(SPDie, dwarf::DW_AT_pure);
+  if (SP->isElemental())
+    addFlag(SPDie, dwarf::DW_AT_elemental);
+  if (SP->isRecursive())
+    addFlag(SPDie, dwarf::DW_AT_recursive);
 }
 
 void DwarfUnit::constructSubrangeDIE(DIE &Buffer, const DISubrange *SR,
                                      DIE *IndexTy) {
   DIE &DW_Subrange = createAndAddDIE(dwarf::DW_TAG_subrange_type, Buffer);
   addDIEEntry(DW_Subrange, dwarf::DW_AT_type, *IndexTy);
 
   // The LowerBound value defines the lower bounds which is typically zero for

llvm/trunk/test/Assembler/disubprogram.ll

 ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder %s
 
 ; CHECK: define void @_Z3foov() !dbg !9
 define void @_Z3foov() !dbg !9 {
   ret void
 }
 
-; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15}
-!named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15}
+; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19}
+!named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15, !17, !19, !20, !21}
 
 !0 = !{null}
 !1 = distinct !DICompositeType(tag: DW_TAG_structure_type)
 !2 = !DIFile(filename: "path/to/file", directory: "/path/to/dir")
 !3 = !DISubroutineType(types: !0)
 !4 = distinct !DICompositeType(tag: DW_TAG_structure_type)
 !5 = distinct !{}
 !6 = distinct !{}
!15 = distinct !DISubprogram(name: "foo", linkageName: "_Zfoov", scope: !1,
                flags: DIFlagPrototyped,
                spFlags: DISPFlagPureVirtual | DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized,
                unit: !8, templateParams: !5, declaration: !9,
                retainedNodes: !6)
 
 !16 = !{i32 1, !"Debug Info Version", i32 3}
 !llvm.module.flags = !{!16}
 !llvm.dbg.cu = !{!8}
+
+; CHECK: !DISubprogram({{.*}}subroutine1{{.*}}, spFlags: DISPFlagDefinition | DISPFlagPure,
+; CHECK: !DISubprogram({{.*}}subroutine2{{.*}}, spFlags: DISPFlagDefinition | DISPFlagElemental,
+; CHECK: !DISubprogram({{.*}}subroutine3{{.*}}, spFlags: DISPFlagDefinition | DISPFlagRecursive,
+; CHECK: !DISubprogram({{.*}}subroutine4{{.*}}, spFlags: DISPFlagDefinition | DISPFlagPure | DISPFlagElemental | DISPFlagRecursive,
+
+!17 = distinct !DISubprogram(name: "subroutine1", scope: !1, file: !2, line: 1, type: !18, scopeLine: 2, spFlags: DISPFlagDefinition | DISPFlagPure, unit: !8, retainedNodes: !6)
+!18 = !DISubroutineType(types: !0)
+!19 = distinct !DISubprogram(name: "subroutine2", scope: !1, file: !2, line: 5, type: !18, scopeLine: 6, spFlags: DISPFlagDefinition | DISPFlagElemental, unit: !8, retainedNodes: !6)
+!20 = distinct !DISubprogram(name: "subroutine3", scope: !1, file: !2, line: 9, type: !18, scopeLine: 10, spFlags: DISPFlagDefinition | DISPFlagRecursive, unit: !8, retainedNodes: !6)
+!21 = distinct !DISubprogram(name: "subroutine4", scope: !1, file: !2, line: 13, type: !18, scopeLine: 14, spFlags: DISPFlagDefinition | DISPFlagPure | DISPFlagElemental | DISPFlagRecursive, unit: !8, retainedNodes: !6)