[CodeView] Properly handle a DISubprogram in getScopeIndex.

[cchen15]

Summary

There's no need to handle DISubprogram in getScopeIndex for C/C++ as a function can't be nested in another function. That's not the case for Fortran-- a routine can be nested in another routine through the 'contains' keyword. This patch skips the emission of the debug info for the containing function.

Please note that before this patch, getScopeIndex would emit a LF_STRING_ID when given a DISubprogram. That's not only incorrect, but would also cause link-time error with the linker from a more recent version (Aug. 20219) of VS2019.

[rnk]

Please note that before this patch, getScopeIndex would emit a LF_STRING_ID when given a DISubprogram. That's not only incorrect, but would also cause link-time error with the linker from a more recent version (Aug. 20219) of VS2019.

Really? It's what we do for C++ namespaces, though, right? I can't imagine why that would result in linker errors. Anyway, it's not important, I trust the new behavior is correct.

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

I think the test should look at IF_TEST_ip_SUB, since that's the nested routine, not the main routine. The nested routine should use the main routine as its parent scope, right?

[cchen15]

Thanks for the feedback, Reid!

Here's the linker error I am seeing with VS2019 16.11.2 before this patch is applied:

[cchen15@f90srv05 jr14335]$ llc fortran-contained-proc.ll -filetype=obj
[cchen15@f90srv05 jr14335]$ link -out:a.out -debug -pdb:a.pdb -subsystem:console fortran-contained-proc.obj
Microsoft (R) Incremental Linker Version 14.29.30133.0
Copyright (C) Microsoft Corporation.  All rights reserved.

libcmt.lib(commit_mode.obj) : fatal error LNK1318: Unexpected PDB error; OK (0) ''

My very uneducated guess is that the new versions of linker try to disallow a S_GPROC32_ID to have a type of LF_STRING_ID, but it's not done properly, resulting in this 'unexpected PDB error' as opposed to a more helpful diagnostic.

[cchen15]

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

IF_TEST (the parent scope of SUB) is the subroutine I'd like to check here, because it is the one that getScopeIndex is called with, when the FuncId for SUB is being looked up in getFuncIdForSubprogram. And the test here is to ensure that IF_TEST points at the right type (two lines below).

That being said, your question prompted me to look at the rest of the dump a bit closer. Here are some observations:

1. SUB is inlined in the test. The S_INLINESITE sits nicely inside S_GPROC32_ID for IF_TEST. The type for that S_INLINESITE points at a LF_FUNC_ID whose parent scope points at the LF_FUNC_ID of IF_TEST.

0x1000 | LF_ARGLIST [size = 8]
0x1001 | LF_PROCEDURE [size = 16]
         return type = 0x0003 (void), # args = 0, param list = 0x1000
         calling conv = cdecl, options = None
0x1002 | LF_FUNC_ID [size = 20]
         name = IF_TEST, type = 0x1001, parent scope = <no type>
0x1003 | LF_FUNC_ID [size = 16]
         name = SUB, type = 0x1001, parent scope = 0x1002
<elided>
       0 | S_GPROC32_ID [size = 48] `IF_TEST`
           parent = 0, end = 0, addr = 0000:0000, code size = 69
           type = `0x1002 (IF_TEST)`, debug start = 0, debug end = 0, flags = none
       0 | S_FRAMEPROC [size = 32]
           size = 40, padding size = 0, offset to padding = 0
           bytes of callee saved registers = 0, exception handler addr = 0000:0000
           local fp reg = RSP, param fp reg = RSP
           flags = opt speed
       0 | S_LDATA32 [size = 16] `A`
           type = 0x0674 (int*), addr = 0000:0000
       0 | S_INLINESITE [size = 24]
           inlinee = 0x1003 (SUB), parent = 0, end = 0
             0608      line 4 (+4)
             033A      code 0x3A (+0x3A)
             0406      code end 0x40 (+0x6)
             0000

2. There is a separate S_GPROC32_ID emitted for IF_TEST::SUB. with its parent set to 0:

0 | S_GPROC32_ID [size = 52] `IF_TEST::SUB`
    parent = 0, end = 0, addr = 0000:0000, code size = 7
    type = `0x1003 (SUB)`, debug start = 0, debug end = 0, flags = none

1. looks fine but I am not sure about 2. I will look into this further. If you have any insight, please share with me.

Note, however, the entry in 2. is also emitted before the patch, so that would be a separate issue from the one I am addressing in this review.

[rnk]

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

I see, I think I understand what is going wrong and where my misunderstanding was. The issue is that getScopeIndex is inserting incorrect information into the TypeIndices table, which is later used for the main IF_TEST subprogram. That, understandably, leads to PDB linker errors.

However, as the reviewer, I'm expecting the change in behavior to appear in the 0x1003 record in your pasted dump:

0x1000 | LF_ARGLIST [size = 8]
0x1001 | LF_PROCEDURE [size = 16]
         return type = 0x0003 (void), # args = 0, param list = 0x1000
         calling conv = cdecl, options = None
0x1002 | LF_FUNC_ID [size = 20]
         name = IF_TEST, type = 0x1001, parent scope = <no type>
0x1003 | LF_FUNC_ID [size = 16]
         name = SUB, type = 0x1001, parent scope = 0x1002   #### parent scope is now 0x1002 instead of a string

What is the desired behavior for the SUB func id record? What should the parent scope be? I'm not aware of a way to make MSVC emit nested function id records like this, so we have no implementation to compare again. Should we create trees of LF_FUNC_ID records, or should we create a separate LF_STRING_ID record to use as the parent?

[cchen15]

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

I see, I think I understand what is going wrong and where my misunderstanding was. The issue is that getScopeIndex is inserting incorrect information into the TypeIndices table, which is later used for the main IF_TEST subprogram. That, understandably, leads to PDB linker errors.

Yes, exactly. My apologies. I should have done a better job explaining this. One oddity is that older linkers (say from VS2019 16.8.2 and earlier) would not issue the link error with IF_TEST having a LF_STRING_ID type.

However, as the reviewer, I'm expecting the change in behavior to appear in the 0x1003 record in your pasted dump:

0x1000 | LF_ARGLIST [size = 8]
0x1001 | LF_PROCEDURE [size = 16]
         return type = 0x0003 (void), # args = 0, param list = 0x1000
         calling conv = cdecl, options = None
0x1002 | LF_FUNC_ID [size = 20]
         name = IF_TEST, type = 0x1001, parent scope = <no type>
0x1003 | LF_FUNC_ID [size = 16]
         name = SUB, type = 0x1001, parent scope = 0x1002   #### parent scope is now 0x1002 instead of a string

What is the desired behavior for the SUB func id record? What should the parent scope be? I'm not aware of a way to make MSVC emit nested function id records like this, so we have no implementation to compare again. Should we create trees of LF_FUNC_ID records, or should we create a separate LF_STRING_ID record to use as the parent?

CodeVIew is very new to me, and I don't know what the proper way to describe the nested-ness of function scopes is. But, coming from DWARF, it makes sense to me to have the parent scope of 0x1003 pointing at 0x1002, reason being that a LD_FUNC_ID has a 'parent scope' field, which readily enables nesting of scopes. In contrast, LF_STRING_ID doesn't have a 'parent scope' (at least in the samples I have seen).

I can't find any document publicly on CodeView format. If you have any pointer, please share with me. Thanks!

[rnk]

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

I will forward this question to some contacts at Microsoft. In the meantime, I think it's safest to use the LF_STRING_ID record as the parent scope. This will make it look like the subroutine is in a C++ namespace, which is probably fine for the debugger.

Functionally, you can implement this by skipping the type table index insertion in getScopeIndex when the scope is a subprogram. Or you could change the key to differentiate it some other way, but I think skipping is simplest. There is another hash table at a lower level that deduplicates equivalent LF_STRING_ID records.

[cchen15]

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

Thank you for your suggestion, Reid. I have updated the code to return zero index when encountering a DISubprogram (i.e. ekipping), and updated the test accordingly.

Thank you too for forwarding the question on encoding nested functions to MS. I appreciate it.

[rnk]

Looks good

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

366

My suggestion was to return TI here but skip recordTypeIndexForDINode when Scope is a subprogram, but this is fine too.

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

26

This test looks good, thanks.

[cchen15]

Thanks for the review, Reid.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

 
 std::string CodeViewDebug::getFullyQualifiedName(const DIScope *Ty) {
   const DIScope *Scope = Ty->getScope();
   return getFullyQualifiedName(Scope, getPrettyScopeName(Ty));
 }
 
 TypeIndex CodeViewDebug::getScopeIndex(const DIScope *Scope) {
   // No scope means global scope and that uses the zero index.
-  if (!Scope || isa<DIFile>(Scope))
+  //
+  // We also use zero index when the scope is a DISubprogram
+  // to suppress the emission of LF_STRING_ID for the function,
+  // which can trigger a link-time error with the linker in
+  // VS2019 version 16.11.2 or newer.
+  // Note, however, skipping the debug info emission for the DISubprogram
+  // is a temporary fix. The root issue here is that we need to figure out
+  // the proper way to encode a function nested in another function
+  // (as introduced by the Fortran 'contains' keyword) in CodeView.
+  if (!Scope || isa<DIFile>(Scope) || isa<DISubprogram>(Scope))
     return TypeIndex();
 
   assert(!isa<DIType>(Scope) && "shouldn't make a namespace scope for a type");
 
   // Check if we've already translated this scope.
   auto I = TypeIndices.find({Scope, nullptr});
   if (I != TypeIndices.end())
     return I->second;
 
   // Build the fully qualified name of the scope.
   std::string ScopeName = getFullyQualifiedName(Scope);
   StringIdRecord SID(TypeIndex(), ScopeName);
   auto TI = TypeTable.writeLeafType(SID);
   return recordTypeIndexForDINode(Scope, TI);
 }
 
 static StringRef removeTemplateArgs(StringRef Name) {
   // Remove template args from the display name. Assume that the template args
   // are the last thing in the name.
   if (Name.empty() || Name.back() != '>')
     return Name;

llvm/test/DebugInfo/COFF/fortran-contained-proc.ll

+; RUN: llc -o %t.obj %s -filetype=obj
+; RUN: llvm-pdbutil dump -symbols -types %t.obj | FileCheck %s
+;
+; The IR in this test derives from the following Fortran program
+; with inlining enabled:
+; program if_test
+;   implicit none
+;   integer, allocatable :: a
+;   allocate(a)
+;   call sub(a)
+; contains
+;   subroutine sub(aa)
+;     implicit none
+;     integer :: aa, bb
+;     bb = 1
+;     aa = bb
+;   end subroutine sub
+; end program if_test
+;
+; CHECK: [[proc_t:.*]] | LF_PROCEDURE
+;
+; CHECK: [[func_id_sub:.*]] | LF_FUNC_ID
+; CHECK-NEXT: name = SUB, type = [[proc_t]], parent scope = <no type>
+;
+; CHECK: [[func_id_if_test:.*]] | LF_FUNC_ID
+; CHECK-NEXT: name = IF_TEST, type = [[proc_t]], parent scope = <no type>
+;
+; CHECK: S_GPROC32_ID [size = {{.*}}] `IF_TEST`
+; CHECK-NEXT: parent
+; CHECK-NEXT: type = `[[func_id_if_test]] (IF_TEST)`
+;
+; ModuleID = 'tr1.f90'
+source_filename = "tr1.f90"
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+@"IF_TEST$A" = internal global i32* null, align 8, !dbg !0
+@0 = internal unnamed_addr constant i32 65536
+@1 = internal unnamed_addr constant i32 2
+
+; Function Attrs: nounwind uwtable
+define void @MAIN__() local_unnamed_addr #0 !dbg !2 {
+alloca_0:
+  %func_result = tail call i32 @for_set_fpe_(i32* nonnull @0) #4, !dbg !22
+  %func_result2 = tail call i32 @for_set_reentrancy(i32* nonnull @1) #4, !dbg !22
+  %func_result4 = tail call i32 @for_alloc_allocatable(i64 4, i8** bitcast (i32** @"IF_TEST$A" to i8**), i32 262144) #4, !dbg !23
+  %"IF_TEST$A_fetch.1" = load i32*, i32** @"IF_TEST$A", align 8, !dbg !24, !tbaa !25
+  call void @llvm.dbg.declare(metadata i32* %"IF_TEST$A_fetch.1", metadata !29, metadata !DIExpression()), !dbg !33
+  call void @llvm.dbg.value(metadata i32 1, metadata !32, metadata !DIExpression()), !dbg !35
+  store i32 1, i32* %"IF_TEST$A_fetch.1", align 1, !dbg !36, !tbaa !37, !alias.scope !41
+  ret void, !dbg !44
+}
+
+; Function Attrs: mustprogress nofree norecurse nosync nounwind uwtable willreturn writeonly
+define void @IF_TEST_ip_SUB(i32* noalias nocapture dereferenceable(4) %AA) local_unnamed_addr #1 !dbg !30 {
+alloca_1:
+  call void @llvm.dbg.declare(metadata i32* %AA, metadata !29, metadata !DIExpression()), !dbg !45
+  call void @llvm.dbg.value(metadata i32 1, metadata !32, metadata !DIExpression()), !dbg !46
+  store i32 1, i32* %AA, align 1, !dbg !47, !tbaa !37
+  ret void, !dbg !48
+}
+
+declare i32 @for_set_fpe_(i32* nocapture readonly) local_unnamed_addr
+
+; Function Attrs: nofree
+declare i32 @for_set_reentrancy(i32* nocapture readonly) local_unnamed_addr #2
+
+; Function Attrs: nofree
+declare i32 @for_alloc_allocatable(i64, i8** nocapture, i32) local_unnamed_addr #2
+
+; Function Attrs: mustprogress nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #3
+
+; Function Attrs: mustprogress nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #3
+
+attributes #0 = { nounwind uwtable "denormal-fp-math"="preserve_sign,preserve_sign" "frame-pointer"="none" "intel-lang"="fortran" "min-legal-vector-width"="0" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" }
+attributes #1 = { mustprogress nofree norecurse nosync nounwind uwtable willreturn writeonly "denormal-fp-math"="preserve_sign,preserve_sign" "frame-pointer"="none" "intel-lang"="fortran" "min-legal-vector-width"="0" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" }
+attributes #2 = { nofree "intel-lang"="fortran" }
+attributes #3 = { mustprogress nofree nosync nounwind readnone speculatable willreturn }
+attributes #4 = { nounwind }
+
+!llvm.module.flags = !{!11, !12, !13}
+!llvm.dbg.cu = !{!6}
+!omp_offload.info = !{}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "A", linkageName: "IF_TEST$A", scope: !2, file: !3, line: 11, type: !9, isLocal: true, isDefinition: true)
+!2 = distinct !DISubprogram(name: "IF_TEST", linkageName: "MAIN__", scope: !3, file: !3, line: 9, type: !4, scopeLine: 9, spFlags: DISPFlagDefinition | DISPFlagMainSubprogram, unit: !6, retainedNodes: !8)
+!3 = !DIFile(filename: "tr1.f90", directory: "d:\\iusers\\cchen15\\examples\\tests\\jr14335")
+!4 = !DISubroutineType(types: !5)
+!5 = !{null}
+!6 = distinct !DICompileUnit(language: DW_LANG_Fortran95, file: !3, producer: "Intel(R) Fortran 22.0-1087", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, globals: !7, splitDebugInlining: false, nameTableKind: None)
+!7 = !{!0}
+!8 = !{}
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!10 = !DIBasicType(name: "INTEGER*4", size: 32, encoding: DW_ATE_signed)
+!11 = !{i32 7, !"PIC Level", i32 2}
+!12 = !{i32 2, !"Debug Info Version", i32 3}
+!13 = !{i32 2, !"CodeView", i32 1}
+!22 = !DILocation(line: 9, scope: !2)
+!23 = !DILocation(line: 12, scope: !2)
+!24 = !DILocation(line: 14, scope: !2)
+!25 = !{!26, !26, i64 0}
+!26 = !{!"ifx$unique_sym$1", !27, i64 0}
+!27 = !{!"Generic Fortran Symbol", !28, i64 0}
+!28 = !{!"ifx$root$1$MAIN__"}
+!29 = !DILocalVariable(name: "AA", arg: 1, scope: !30, file: !3, line: 17, type: !10)
+!30 = distinct !DISubprogram(name: "SUB", linkageName: "IF_TEST_ip_SUB", scope: !2, file: !3, line: 17, type: !4, scopeLine: 17, spFlags: DISPFlagDefinition, unit: !6, retainedNodes: !31)
+!31 = !{!29, !32}
+!32 = !DILocalVariable(name: "BB", scope: !30, file: !3, line: 19, type: !10)
+!33 = !DILocation(line: 17, scope: !30, inlinedAt: !34)
+!34 = distinct !DILocation(line: 14, scope: !2)
+!35 = !DILocation(line: 0, scope: !30, inlinedAt: !34)
+!36 = !DILocation(line: 21, scope: !30, inlinedAt: !34)
+!37 = !{!38, !38, i64 0}
+!38 = !{!"ifx$unique_sym$3", !39, i64 0}
+!39 = !{!"Generic Fortran Symbol", !40, i64 0}
+!40 = !{!"ifx$root$2$IF_TEST_ip_SUB"}
+!41 = !{!42}
+!42 = distinct !{!42, !43, !"IF_TEST_ip_SUB: %AA"}
+!43 = distinct !{!43, !"IF_TEST_ip_SUB"}
+!44 = !DILocation(line: 16, scope: !2)
+!45 = !DILocation(line: 17, scope: !30)
+!46 = !DILocation(line: 0, scope: !30)
+!47 = !DILocation(line: 21, scope: !30)
+!48 = !DILocation(line: 22, scope: !30)