Page MenuHomePhabricator

[DebugInfo] Enhance DIImportedEntity to accept children entities for renamed variables
ClosedPublic

Authored by alok on Mon, Sep 6, 11:14 PM.

Details

Summary
This is needed to dump optimized debugging information where all names in a module are imported, but a few names are imported with overriding aliases

Please consider blow test case.
```````````````````
module mymod
   integer :: var1 = 11
   integer :: var2 = 12
   integer :: var3 = 13
 end module mymod

 Program main
   call use_renamed()
   contains
     subroutine use_renamed()
       use mymod, var4 => var1
       print *, var4
     end subroutine use_renamed
 end program main
```````````````````
Which currently produces IR as
```
 !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
 !1 = distinct !DIGlobalVariable(name: "var1", scope: !2, file: !3, line: 2, type: !9, isLocal: false, isDefinition: true)
 !2 = !DIModule(scope: !4, name: "mymod", file: !3, line: 1)
 !3 = !DIFile(filename: "usemodulealias.f90", directory: "/tmp")
 !4 = distinct !DICompileUnit(language: DW_LANG_Fortran90, file: !3, producer: " F90 Flang - 1.5 2017-05-01", isOptimized: false, flags: "'+flang -g -S -emit-llvm usemodulealias.f90'", runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, globals: !6, **imports: !12**, nameTableKind: None)
 !5 = !{}
 !6 = !{!0, !7, !10}
 !7 = !DIGlobalVariableExpression(var: !8, expr: !DIExpression(DW_OP_plus_uconst, 4))
 !8 = distinct !DIGlobalVariable(name: "var2", scope: !2, file: !3, line: 3, type: !9, isLocal: false, isDefinition: true)
 !9 = !DIBasicType(name: "integer", size: 32, align: 32, encoding: DW_ATE_signed)
 !10 = !DIGlobalVariableExpression(var: !11, expr: !DIExpression(DW_OP_plus_uconst, 8))
 !11 = distinct !DIGlobalVariable(name: "var3", scope: !2, file: !3, line: 4, type: !9, isLocal: false, isDefinition: true)
** !12 = !{!13, !19, !20}**
 !13 = !**DIImportedEntity**(tag: DW_TAG_imported_declaration, scope: !14, entity: !11, file: !3, line: 10)
 !14 = distinct !DISubprogram(name: "use_renamed", scope: !15, file: !3, line: 10, type: !18, scopeLine: 10, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !4)
 !15 = distinct !DISubprogram(name: "main", scope: !4, file: !3, line: 7, type: !16, scopeLine: 7, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagMainSubprogram, unit: !4)
 !16 = !DISubroutineType(cc: DW_CC_program, types: !17)
 !17 = !{null}
 !18 = !DISubroutineType(types: !17)
 !19 = !**DIImportedEntity**(tag: DW_TAG_imported_declaration, scope: !14, entity: !8, file: !3, line: 10)
 !20 = !**DIImportedEntity**(tag: DW_TAG_imported_declaration, name: "var4", scope: !14, entity: !1, file: !3, line: 10)
``````
Please consider an imported Module having one renamed member out of
 total thousand members. With above representation we need to generate
thousand DIImportedEntity's for each member.
which can be optimize to
`````
 !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
 !1 = distinct !DIGlobalVariable(name: "var1", scope: !2, file: !3, line: 2, type: !9, isLocal: false, isDefinition: true)
 !2 = !DIModule(scope: !4, name: "mymod", file: !3, line: 1)
 !3 = !DIFile(filename: "usemodulealias.f90", directory: "/tmp")
 !4 = distinct !DICompileUnit(language: DW_LANG_Fortran90, file: !3, producer: " F90 Flang - 1.5 2017-05-01", isOptimized: false, flags: "'+flang usemodulealias.f90 -g -S -emit-llvm'", runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, globals: !6, **imports: !12**, nameTableKind: None)
 !5 = !{}
 !6 = !{!0, !7, !10}
 !7 = !DIGlobalVariableExpression(var: !8, expr: !DIExpression(DW_OP_plus_uconst, 4))
 !8 = distinct !DIGlobalVariable(name: "var2", scope: !2, file: !3, line: 3, type: !9, isLocal: false, isDefinition: true)
 !9 = !DIBasicType(name: "integer", size: 32, align: 32, encoding: DW_ATE_signed)
 !10 = !DIGlobalVariableExpression(var: !11, expr: !DIExpression(DW_OP_plus_uconst, 8))
 !11 = distinct !DIGlobalVariable(name: "var3", scope: !2, file: !3, line: 4, type: !9, isLocal: false, isDefinition: true)
 **!12 = !{!13}**
 !13 = !**DIImportedEntity**(tag: DW_TAG_imported_module, scope: !14, entity: !2, file: !3, line: 10, **elements**: **!19**)
 !14 = distinct !DISubprogram(name: "use_renamed", scope: !15, file: !3, line: 10, type: !18, scopeLine: 10, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !4)
 **!19 = !{!20}**
** !20 = !DIImportedEntity**(tag: DW_TAG_imported_declaration, **name: "var4"**, scope: !14, entity: !1, file: !3, line: 10)
`````````````````
DIImportedEntity should include elements field which includes renamed variables only.

Diff Detail

Event Timeline

alok created this revision.Mon, Sep 6, 11:14 PM
alok requested review of this revision.Mon, Sep 6, 11:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Sep 6, 11:14 PM
alok updated this revision to Diff 370996.Tue, Sep 7, 12:07 AM

Re-based and added backward comp test.

alok updated this revision to Diff 371854.Fri, Sep 10, 3:24 AM

Updated to satisfy clang-tidy for new code.

Warnings related to existing code is handled in separate patch as D109590

alok edited the summary of this revision. (Show Details)Sun, Sep 12, 9:39 PM

The subject might be slightly misleading - DIImportedEntity already supports importing variables (see C++ like this:

namespace ns {
extern int i;
int i;
}
namespace ns2 {
using ns::i;
}
int main() {
  return ns2::i;
}

which produces a DW_TAG_imported_declaration in DW_TAG_namespace ns2 that imports the DW_TAG_variable i from DW_TAG_namespace ns.

. This patch is specifically about supporting a single imported entity declaration importing multiple entities at the same time, but not a whole namespace of them?

Hmm, nope - it looks like it's only testing importing a single variable. What sort of DWARF are you expecting to generate when importing multiple variables (when "children" has a size greater than 1). Oh, in the implementation in DwarfDebug it's still generating separate DW_TAG_imported_declaration for each thing - so why not encode these as separate DIImportedDeclarations, which would already be supported in the current IR handling, so far as I can tell?

alok added a comment.Tue, Sep 14, 3:57 AM

The subject might be slightly misleading - DIImportedEntity already supports importing variables (see C++ like this:

namespace ns {
extern int i;
int i;
}
namespace ns2 {
using ns::i;
}
int main() {
  return ns2::i;
}

which produces a DW_TAG_imported_declaration in DW_TAG_namespace ns2 that imports the DW_TAG_variable i from DW_TAG_namespace ns.

. This patch is specifically about supporting a single imported entity declaration importing multiple entities at the same time, but not a whole namespace of them?

Hmm, nope - it looks like it's only testing importing a single variable. What sort of DWARF are you expecting to generate when importing multiple variables (when "children" has a size greater than 1). Oh, in the implementation in DwarfDebug it's still generating separate DW_TAG_imported_declaration for each thing - so why not encode these as separate DIImportedDeclarations, which would already be supported in the current IR handling, so far as I can tell?

I am sorry for not being clear earlier.

Please consider below testcase.

module mymod
  integer :: var1
  integer :: var2
  integer :: var3
  integer :: var4
  integer :: var5
end module mymod

Program main
  implicit none
  call use_renamed()
  contains
    subroutine use_renamed()
      use mymod, alias1 => var1, alias2 => var2
      print *, alias1
      print *, alias2
      print *, var3
      print *, var4
      print *, var5
    end subroutine use_renamed
end program main

Please note that inside subroutine "use_renamed", entire module "mymod" is imported but "var1" and "var2" are identified as "alias1" and "alias2".

Currently this can be represented in DWARF as

0x0000000b: DW_TAG_compile_unit
              DW_AT_language    (DW_LANG_Fortran90)
              DW_AT_name        ("mymoduse.f90")

0x0000002a:   DW_TAG_module
                DW_AT_name      ("mymod")

0x00000031:     DW_TAG_variable
                  DW_AT_name    ("var1")
                  DW_AT_location        (DW_OP_addr 0x0)

0x00000046:     DW_TAG_variable
                  DW_AT_name    ("var2")
                  DW_AT_location        (DW_OP_addr 0x0, DW_OP_plus_uconst 0x4)

0x0000005d:     DW_TAG_variable
                  DW_AT_name    ("var3")
                  DW_AT_location        (DW_OP_addr 0x0, DW_OP_plus_uconst 0x8)

0x00000074:     DW_TAG_variable
                  DW_AT_name    ("var4")
                  DW_AT_location        (DW_OP_addr 0x0, DW_OP_plus_uconst 0xc)

0x0000008b:     DW_TAG_variable
                  DW_AT_name    ("var5")
                  DW_AT_location        (DW_OP_addr 0x0, DW_OP_plus_uconst 0x10)

0x000000a2:     NULL


0x000000d4:     DW_TAG_subprogram
                  DW_AT_low_pc  (0x0000000000000030)
                  DW_AT_high_pc (0x0000000000000274)
                  DW_AT_frame_base      (DW_OP_reg7 RSP)
                  DW_AT_name    ("use_renamed")

0x000000fe:       DW_TAG_imported_declaration
                    DW_AT_import        (0x0000008b)

0x00000105:       DW_TAG_imported_declaration
                    DW_AT_import        (0x00000074)

0x0000010c:       DW_TAG_imported_declaration
                    DW_AT_import        (0x0000005d)

0x00000113:       DW_TAG_imported_declaration
                    DW_AT_import        (0x00000046)
                    DW_AT_name  ("alias2")

0x0000011e:       DW_TAG_imported_declaration
                    DW_AT_import        (0x00000031)
                    DW_AT_name  ("alias1")

0x00000129:       NULL

Please note that subroutine "use_rename" need to include 5 import entities here, just think about a module having 100 variables with 2 renamed entities will have to inlcude 100 "DW_TAG_imported_declaration".

gfortran dumps below DWARF for same case as,

0x000000d4:     DW_TAG_subprogram
                  DW_AT_name    ("use_renamed")
                  DW_AT_low_pc  (0x0000000000000000)
                  DW_AT_high_pc (0x000000000000020e)
                  DW_AT_frame_base      (DW_OP_call_frame_cfa)
                  DW_AT_static_link     (DW_OP_fbreg -504, DW_OP_deref, DW_OP_deref)

0x00000100:       DW_TAG_imported_module
                    DW_AT_import        (0x0000002a)

0x0000010b:          DW_TAG_imported_declaration
                       DW_AT_name        ("alias1")
                       DW_AT_import      (0x00000031)

0x00000116:          DW_TAG_imported_declaration
                       DW_AT_name        ("alias2")
                       DW_AT_import      (0x00000046)

0x00000121:         NULL

Please note that DW_TAG_imported_module has two child entities (for renamed variable), which denotes all variables in module (renamed entities are overridden). The case of module having 100 members and 2 renames can be represented with only 3 DIEs. (saving 97 DIEs).

The new representation (with DIImportedEntity having child) is surely going to save us some DWARF space in case of huge modules with few renames getting imported.

Ah, OK, thanks for explaining - sorry I wasn't following the fortran constructs.

Maybe the patch description could be updated a bit - this bit: " This is needed to dump optimized debugging information when imported entity
has renamed entities (member variables, subprograms)" perhaps along the lines of "where all names in a module are imported, but a few names are imported with overriding aliases"?

But yeah, the general direction seems reasonable to me.

alok edited the summary of this revision. (Show Details)Tue, Sep 14, 9:51 PM
alok added a reviewer: dblaikie.
alok updated this revision to Diff 372635.Tue, Sep 14, 10:42 PM

Re-based.

dblaikie accepted this revision.Wed, Sep 15, 1:33 PM

Generally looks OK to me.

llvm/docs/LangRef.rst
5793–5795
This revision is now accepted and ready to land.Wed, Sep 15, 1:33 PM
alok added a comment.Wed, Sep 15, 10:01 PM

Generally looks OK to me.

Thanks a lot @dblaikie . I shall merge the changes after including your comments.

This revision was landed with ongoing or failed builds.Wed, Sep 15, 10:36 PM
This revision was automatically updated to reflect the committed changes.