[codeview] Improved array type support (multi dimension array)

[aaboud]

Summary

Added support for multi dimension array.

Also worked around some issues with:

1. array of incomplete structure type.
2. dynamic size array.

[majnemer]

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

859–862 ↗

(On Diff #61282)

If a type is incomplete, how can we emit a size for it?

[rnk]

Thanks, seems like the right approach.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

858–862 ↗

(On Diff #61282)

This shouldn't be a FIXME. There is nothing we can do in the frontend when the type definition is unavailable, as in the example you used in your test case. MSVC gives the array size zero, which is what your code does. You should reword the comment to document that this is intended behavior.

876 ↗

(On Diff #61282)

Why not just assert(Subrange->getLowerBound() == 0 && "codeview doesn't support subranges with lower bounds");? As is, your code will probably trigger unused variable warnings in NDEBUG builds.

881 ↗

(On Diff #61282)

This comment needs work. It's worth mentioning that a count of -1 means that this is a VLA, and that it is not currently representable in CodeView. Actually, why not just let it go through as ~0U?

894 ↗

(On Diff #61282)

I think without adding (void)UndefinedSubrange outside the assert, you will get -Wunused-but-set warnings in an NDEBUG build.

[majnemer]

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

881 ↗

(On Diff #61282)

Can't we use LF_DIMVARLU for VLAs?

[aaboud]

Thanks for the comments.
Answer inline.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

858–862 ↗

(On Diff #61282)

Actually, it is a bug. However, my example was not correct.
I fixed the example to show the issue.

It is true that incomplete structure has no size.
However if we:

1. Declare an incomplete structure.
2. Define the structure later.
3. Declare an array of that defined structure.

Then the array will have no size, though it will be pointing to the defined complete structure.

This leads the assertion to be hit. (for incomplete structure without definition we do not hit assertion)

881 ↗

(On Diff #61282)

This comment needs work. It's worth mentioning that a count of -1 means that this is a VLA, and that it is not currently representable in CodeView. Actually, why not just let it go through as ~0U?

I will fix the comment.
output ~0U will sign to the debugger that you have a large array what might cause accessing out-of-scope memory.
Better assign the minimum size of '1' till we fix the LLVM Ir and use LF_DIMVARLU as David suggested.

Can't we use LF_DIMVARLU for VLAs?

We should do that once front-end supports sub-ranges of variable values.
To do this we need to introduce a new debug intrinsic, as we cannot point to an instruction from external metadata.

[aaboud]

Applied changes according to comments from Reid and David.

[rnk]

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

858–862 ↗

(On Diff #61446)

Nice find! However, let's file that bug in http://llvm.org/bugs, and once we fix it, this code doesn't need to change, so I don't see why we need a comment here about it. It's correct as written, assuming the frontend does the right thing.

881 ↗

(On Diff #61446)

VLA stands for "variable length array", not "very large array". :)

Why do we need to change the frontend? Don't we already know that Count == -1 implies a VLA? This will be a backend change, not a frontend change.

[aaboud]

Thanks Reid for the comments.
Please see answers below.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

858–862 ↗

(On Diff #61446)

OK, will report a bug in LLVM.
I can remove the comment, however, if we fix this bug we will not need the check below, that is why I added the comment in the first place.
If you think that we should not assert that arrays with total size zero, does not sum into non-zero size, then we can let this check here exist. Otherwise, this check is a temporary one and should be removed once the above bug is fixed.

What is your preference?

881 ↗

(On Diff #61446)

VLA stands for "variable length array", not "very large array". :)

Indeed, I thought I wrote it right :) will fix it.

Why do we need to change the frontend? Don't we already know that Count == -1 implies a VLA? This will be a backend change, not a frontend change.

Because we should emit something like this:

; void foo(int x) {
;   int dyn_size_arr[x];
;   dyn_size_arr[0] = 0;
; }

REFSYM (0x1003) {
  TypeLeafKind: LF_REFSYM (0x20c)
  Constant {
    Type: long (0x112)
    Value: 0
  }
}

REFSYM (0x1004) {
  TypeLeafKind: LF_REFSYM (0x20c)
  RegRelativeSym {
    Offset: 0x40
    Type: int (0x74)
    Register: 0x14E
    VarName: x
  }
}

DIMVARLU (0x1005) {
  TypeLeafKind: LF_DIMVARLU (0x120a)
  Rank: 1
  IndexType: int (0x74)
  Bounds: (0x1003:0x1004)
}

Array (0x1006) {
  TypeLeafKind: LF_DIMARRAY (0x1508)
  ElementType: int (0x74)
  DimensionInfo: 0x1005
  Name:
}

We need to track dynamic array range, which will be an LLVM IR value.
Can we do this without adding a new dbg intrinsic?

[rnk]

lgtm with a better comment about arrays of incomplete type.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

858–862 ↗

(On Diff #61446)

OK, now I understand. I thought we needed this check to pass the assertion in the simple incomplete type case. I think the comment needs to elaborate that this is only a problem when the code builds an array of incomplete struct type, and then later completes the struct type. In fact, just putting the test case in the comments helps:

struct A (*p)[3];
struct A { int f; } a[3];

881 ↗

(On Diff #61446)

Thanks for the explanation, makes sense.

[aaboud]

Patch was reverted at revision (273815) due to PR28311.

[aaboud]

Fixed PR28311.
Now we take care of base element of the array, which is derived from other type (e.g. typedef).
These derived types might have no Size field, that is why we hit the assertion:
assert(UndefinedSubrange || ElementSize == (Ty->getSizeInBits() / 8));

[rnk]

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

918 ↗

(On Diff #62084)

use clang-format to align Tag.

933 ↗

(On Diff #62084)

use clang-format here

Can you give an example of when this would be needed? It would make a good test.

test/DebugInfo/COFF/types-array-advanced.ll

17 ↗

(On Diff #62084)

Maybe make it 'typedef const volatile int T_INT' to cover a few more codepaths?

[aaboud]

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

933 ↗

(On Diff #62084)

I am not sure there is an example where a base type of an array is a reference or rvalue reference.
I took this code from DwarfUnit.cpp, where it was used to determine base size of member types.

I would remove this code for now, unless you have other opinion.

[aaboud]

Improved LIT test according to Reid comment.
Applied clang-format.
Update revision to top of trunk.

[rnk]

Looks good after sharing common code

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1005 ↗

(On Diff #62743)

Oh, then let's move it to the common base class for both, DebugHandlerBase.

[aaboud]

Applied Reid comment and moved getBaseTypeSize to DebugHandlerBase as it used by DwarfDebug and CodeViewDebug.

Added David Blaikie to review the change in DwarfDebug.

[aaboud]

Updated getBaseTypeSize to match more the top of trunk version moved from DwarfUnit.cpp.

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

 TypeIndex CodeViewDebug::lowerTypeArray(const DICompositeType *Ty) {
   DITypeRef ElementTypeRef = Ty->getBaseType();
   TypeIndex ElementTypeIndex = getTypeIndex(ElementTypeRef);
   // IndexType is size_t, which depends on the bitness of the target.
   TypeIndex IndexType = Asm->MAI->getPointerSize() == 8
                             ? TypeIndex(SimpleTypeKind::UInt64Quad)
                             : TypeIndex(SimpleTypeKind::UInt32Long);
   uint64_t Size = Ty->getSizeInBits() / 8;
-  ArrayRecord Record(ElementTypeIndex, IndexType, Size, Ty->getName());
-  return TypeTable.writeArray(Record);
+  assert(ElementTypeRef.resolve());
+  uint64_t ElementSize = ElementTypeRef.resolve()->getSizeInBits() / 8;
+
+  bool UndefinedSubrange = false;
+
+  // FIXME:
+  // There is a bug in the front-end where an array of a structure, which was
+  // declared as incomplete structure first, ends up not getting a size assigned
+  // to it. (PR28303)
+  // Example:
+  //   struct A(*p)[3];
+  //   struct A { int f; } a[3];
+  //
+  // This needs to be fixed in the front-end, but in the meantime we don't want
+  // to trigger an assertion because of this.
+  if (Ty->getSizeInBits() == 0) {
+    UndefinedSubrange = true;
+  }
+
+  // Add subranges to array type.
+  DINodeArray Elements = Ty->getElements();
+  for (int i = Elements.size() - 1; i >= 0; --i) {
+    const DINode *Element = Elements[i];
+    assert(Element->getTag() == dwarf::DW_TAG_subrange_type);
+
+    const DISubrange *Subrange = cast<DISubrange>(Element);
+    assert(Subrange->getLowerBound() == 0 &&
+           "codeview doesn't support subranges with lower bounds");
+    int64_t Count = Subrange->getCount();
+
+    // Variable Length Array (VLA) has Count equal to '-1'.
+    // Replace with Count '1', assume it is the minimum VLA length.
+    // FIXME: Make front-end support VLA subrange and emit LF_DIMVARLU.
+    if (Count == -1) {
+      Count = 1;
+      UndefinedSubrange = true;
+    }
+
+    StringRef Name = (i == 0) ? Ty->getName() : "";
+    // Update the element size and element type index for subsequent subranges.
+    ElementSize *= Count;
+    ElementTypeIndex = TypeTable.writeArray(
+        ArrayRecord(ElementTypeIndex, IndexType, ElementSize, Name));
+  }
+
+  (void)UndefinedSubrange;
+  assert(UndefinedSubrange || ElementSize == Size);
+
+  return ElementTypeIndex;
 }
 
 TypeIndex CodeViewDebug::lowerTypeBasic(const DIBasicType *Ty) {
   TypeIndex Index;
   dwarf::TypeKind Kind;
   uint32_t ByteSize;
 
   Kind = static_cast<dwarf::TypeKind>(Ty->getEncoding());

llvm/trunk/test/DebugInfo/COFF/types-array-advanced.ll

+; RUN: llc < %s -filetype=obj | llvm-readobj - -codeview | FileCheck %s
+
+; C++ source to regenerate:
+; $ cat t.cpp
+; char multi_dim_arr[2][5][7];
+;
+; struct incomplete_struct(*p_incomplete_struct_arry)[3];
+; struct incomplete_struct {
+;   int s1;
+; } incomplete_struct_arry[3];
+;
+; void foo(int x) {
+;   int dyn_size_arr[x];
+;   dyn_size_arr[0] = 0;
+; }
+; $ clang t.cpp -S -emit-llvm -g -gcodeview -o t.ll
+
+; CHECK: CodeViewTypes [
+; CHECK:   Section: .debug$T (5)
+; CHECK:   Magic: 0x4
+; CHECK:   ArgList (0x1000) {
+; CHECK:     TypeLeafKind: LF_ARGLIST (0x1201)
+; CHECK:     NumArgs: 1
+; CHECK:     Arguments [
+; CHECK:       ArgType: int (0x74)
+; CHECK:     ]
+; CHECK:   }
+; CHECK:   Procedure (0x1001) {
+; CHECK:     TypeLeafKind: LF_PROCEDURE (0x1008)
+; CHECK:     ReturnType: void (0x3)
+; CHECK:     CallingConvention: NearC (0x0)
+; CHECK:     FunctionOptions [ (0x0)
+; CHECK:     ]
+; CHECK:     NumParameters: 1
+; CHECK:     ArgListType: (int) (0x1000)
+; CHECK:   }
+; CHECK:   FuncId (0x1002) {
+; CHECK:     TypeLeafKind: LF_FUNC_ID (0x1601)
+; CHECK:     ParentScope: 0x0
+; CHECK:     FunctionType: void (int) (0x1001)
+; CHECK:     Name: foo
+; CHECK:   }
+; CHECK:   Array (0x1003) {
+; CHECK:     TypeLeafKind: LF_ARRAY (0x1503)
+; CHECK:     ElementType: int (0x74)
+; CHECK:     IndexType: unsigned long (0x22)
+; CHECK:     SizeOf: 4
+; CHECK:     Name: 
+; CHECK:   }
+; CHECK:   Array (0x1004) {
+; CHECK:     TypeLeafKind: LF_ARRAY (0x1503)
+; CHECK:     ElementType: char (0x70)
+; CHECK:     IndexType: unsigned long (0x22)
+; CHECK:     SizeOf: 7
+; CHECK:     Name: 
+; CHECK:   }
+; CHECK:   Array (0x1005) {
+; CHECK:     TypeLeafKind: LF_ARRAY (0x1503)
+; CHECK:     ElementType: 0x1004
+; CHECK:     IndexType: unsigned long (0x22)
+; CHECK:     SizeOf: 35
+; CHECK:     Name: 
+; CHECK:   }
+; CHECK:   Array (0x1006) {
+; CHECK:     TypeLeafKind: LF_ARRAY (0x1503)
+; CHECK:     ElementType: 0x1005
+; CHECK:     IndexType: unsigned long (0x22)
+; CHECK:     SizeOf: 70
+; CHECK:     Name: 
+; CHECK:   }
+; CHECK:   Struct (0x1007) {
+; CHECK:     TypeLeafKind: LF_STRUCTURE (0x1505)
+; CHECK:     MemberCount: 0
+; CHECK:     Properties [ (0x280)
+; CHECK:       ForwardReference (0x80)
+; CHECK:       HasUniqueName (0x200)
+; CHECK:     ]
+; CHECK:     FieldList: 0x0
+; CHECK:     DerivedFrom: 0x0
+; CHECK:     VShape: 0x0
+; CHECK:     SizeOf: 0
+; CHECK:     Name: incomplete_struct
+; CHECK:     LinkageName: .?AUincomplete_struct@@
+; CHECK:   }
+; CHECK:   Array (0x1008) {
+; CHECK:     TypeLeafKind: LF_ARRAY (0x1503)
+; CHECK:     ElementType: incomplete_struct (0x1007)
+; CHECK:     IndexType: unsigned long (0x22)
+; CHECK:     SizeOf: 12
+; CHECK:     Name: 
+; CHECK:   }
+; CHECK:   Pointer (0x1009) {
+; CHECK:     TypeLeafKind: LF_POINTER (0x1002)
+; CHECK:     PointeeType: 0x1008
+; CHECK:     PointerAttributes: 0x800A
+; CHECK:     PtrType: Near32 (0xA)
+; CHECK:     PtrMode: Pointer (0x0)
+; CHECK:     IsFlat: 0
+; CHECK:     IsConst: 0
+; CHECK:     IsVolatile: 0
+; CHECK:     IsUnaligned: 0
+; CHECK:     SizeOf: 4
+; CHECK:   }
+; CHECK: ]
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc18.0.31101"
+
+%struct.incomplete_struct = type { i32 }
+
+@"\01?multi_dim_arr@@3PAY146DA" = global [2 x [5 x [7 x i8]]] zeroinitializer, align 1
+@"\01?p_incomplete_struct_arry@@3PAY02Uincomplete_struct@@A" = global [3 x i8]* null, align 4
+@"\01?incomplete_struct_arry@@3PAUincomplete_struct@@A" = global [3 x %struct.incomplete_struct] zeroinitializer, align 4
+
+; Function Attrs: nounwind
+define void @"\01?foo@@YAXH@Z"(i32 %x) #0 !dbg !24 {
+entry:
+  %x.addr = alloca i32, align 4
+  %saved_stack = alloca i8*, align 4
+  store i32 %x, i32* %x.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !27, metadata !28), !dbg !29
+  %0 = load i32, i32* %x.addr, align 4, !dbg !30
+  %1 = call i8* @llvm.stacksave(), !dbg !31
+  store i8* %1, i8** %saved_stack, align 4, !dbg !31
+  %vla = alloca i32, i32 %0, align 4, !dbg !31
+  call void @llvm.dbg.declare(metadata i32* %vla, metadata !32, metadata !36), !dbg !37
+  %arrayidx = getelementptr inbounds i32, i32* %vla, i32 0, !dbg !38
+  store i32 0, i32* %arrayidx, align 4, !dbg !39
+  %2 = load i8*, i8** %saved_stack, align 4, !dbg !40
+  call void @llvm.stackrestore(i8* %2), !dbg !40
+  ret void, !dbg !40
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind
+declare i8* @llvm.stacksave() #2
+
+; Function Attrs: nounwind
+declare void @llvm.stackrestore(i8*) #2
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!21, !22}
+!llvm.ident = !{!23}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.9.0 (trunk 273084)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3)
+!1 = !DIFile(filename: "t.cpp", directory: "/")
+!2 = !{}
+!3 = !{!4, !11, !20}
+!4 = distinct !DIGlobalVariable(name: "multi_dim_arr", linkageName: "\01?multi_dim_arr@@3PAY146DA", scope: !0, file: !1, line: 1, type: !5, isLocal: false, isDefinition: true, variable: [2 x [5 x [7 x i8]]]* @"\01?multi_dim_arr@@3PAY146DA")
+!5 = !DICompositeType(tag: DW_TAG_array_type, baseType: !6, size: 560, align: 8, elements: !7)
+!6 = !DIBasicType(name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)
+!7 = !{!8, !9, !10}
+!8 = !DISubrange(count: 2)
+!9 = !DISubrange(count: 5)
+!10 = !DISubrange(count: 7)
+!11 = distinct !DIGlobalVariable(name: "p_incomplete_struct_arry", linkageName: "\01?p_incomplete_struct_arry@@3PAY02Uincomplete_struct@@A", scope: !0, file: !1, line: 3, type: !12, isLocal: false, isDefinition: true, variable: [3 x i8]** @"\01?p_incomplete_struct_arry@@3PAY02Uincomplete_struct@@A")
+!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 32, align: 32)
+!13 = !DICompositeType(tag: DW_TAG_array_type, baseType: !14, elements: !18)
+!14 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "incomplete_struct", file: !1, line: 4, size: 32, align: 32, elements: !15, identifier: ".?AUincomplete_struct@@")
+!15 = !{!16}
+!16 = !DIDerivedType(tag: DW_TAG_member, name: "s1", scope: !14, file: !1, line: 5, baseType: !17, size: 32, align: 32)
+!17 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!18 = !{!19}
+!19 = !DISubrange(count: 3)
+!20 = distinct !DIGlobalVariable(name: "incomplete_struct_arry", linkageName: "\01?incomplete_struct_arry@@3PAUincomplete_struct@@A", scope: !0, file: !1, line: 6, type: !13, isLocal: false, isDefinition: true, variable: [3 x %struct.incomplete_struct]* @"\01?incomplete_struct_arry@@3PAUincomplete_struct@@A")
+!21 = !{i32 2, !"CodeView", i32 1}
+!22 = !{i32 2, !"Debug Info Version", i32 3}
+!23 = !{!"clang version 3.9.0 (trunk 273084)"}
+!24 = distinct !DISubprogram(name: "foo", linkageName: "\01?foo@@YAXH@Z", scope: !1, file: !1, line: 8, type: !25, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!25 = !DISubroutineType(types: !26)
+!26 = !{null, !17}
+!27 = !DILocalVariable(name: "x", arg: 1, scope: !24, file: !1, line: 8, type: !17)
+!28 = !DIExpression()
+!29 = !DILocation(line: 8, column: 14, scope: !24)
+!30 = !DILocation(line: 9, column: 20, scope: !24)
+!31 = !DILocation(line: 9, column: 3, scope: !24)
+!32 = !DILocalVariable(name: "dyn_size_arr", scope: !24, file: !1, line: 9, type: !33)
+!33 = !DICompositeType(tag: DW_TAG_array_type, baseType: !17, align: 32, elements: !34)
+!34 = !{!35}
+!35 = !DISubrange(count: -1)
+!36 = !DIExpression(DW_OP_deref)
+!37 = !DILocation(line: 9, column: 7, scope: !24)
+!38 = !DILocation(line: 10, column: 3, scope: !24)
+!39 = !DILocation(line: 10, column: 19, scope: !24)
+!40 = !DILocation(line: 11, column: 1, scope: !24)