[PATCH] D19236: Add DITypeIndex for CodeView and emit it for locals

[David Blaikie via llvm-commits]

dblaikie added a comment.

Generally looks good - should there be more test coverage, though? At least testing one case of a type other than int being generated into the code view? (to demonstrate the new codepath, etc - it looks like the only things testing the CodeView type reference emission are testing the old "everything is int" implementation?)


================
Comment at: lib/Bitcode/Writer/ValueEnumerator.cpp:623
@@ -624,1 +622,3 @@
+      (isa<MDNode>(MD) || isa<MDString>(MD) || isa<ConstantAsMetadata>(MD)) ||
+      isa<DITypeIndex>(MD) && "Invalid metadata kind");
 
----------------
That's probably going to trigger the parens precedence warning - I think you need to put the || x inside the original outer ()

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:712-713
@@ -711,1 +711,4 @@
 
+  // Pull the type index out of the type field. If the frontend did not provide
+  // a DITypeIndex, emit zero for "no type".
+  unsigned TI = unsigned(SimpleTypeKind::None);
----------------
Is this a supported case? Sounds like if that happens it's corrupt debug info, no? (catch this in the verifier & then just assert that it'll always be present here?)

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:714
@@ +713,3 @@
+  // a DITypeIndex, emit zero for "no type".
+  unsigned TI = unsigned(SimpleTypeKind::None);
+  if (auto *DTI = dyn_cast_or_null<DITypeIndex>(Var.DIVar->getType()))
----------------
static_cast rather than C style cast?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:591-592
@@ +590,4 @@
+  DITypeRefArray FnArgs;
+  if (DISubroutineType *FnTy =
+          cast_or_null<DISubroutineType>(resolve(Sub->getType())))
+    FnArgs = FnTy->getTypeArray();
----------------
Under what conditions would this fail? Is there a test case for it?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1209-1210
@@ -1208,3 +1208,4 @@
   DITypeRefArray Args;
-  if (const DISubroutineType *SPTy = SP->getType())
+  if (const DISubroutineType *SPTy =
+          cast_or_null<DISubroutineType>(resolve(SP->getType())))
     Args = SPTy->getTypeArray();
----------------
Can use 'auto*' on the LHS when the type is explicit in the cast on the RHS (similarly elswhere in this change) - style guide suggests this as acceptable, but not sure it explicitly favors it, up to you.

================
Comment at: lib/IR/DebugInfoMetadata.cpp:639-640
@@ +638,4 @@
+  if (!Slot) {
+    void *Mem = Context.pImpl->TypeAllocator.Allocate(sizeof(DITypeIndex),
+                                                      alignof(DITypeIndex));
+    Slot = new (Mem) DITypeIndex(Index);
----------------
Is this the first use of TypeAllocator for something other than types? Should it be renamed, or at least a comment added? (or a different allocator used?)

================
Comment at: test/DebugInfo/COFF/local-variables.ll:285
@@ -284,3 +284,3 @@
 !6 = !{null, !7}
 !7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
 !8 = distinct !DISubprogram(name: "will_be_inlined", linkageName: "\01?will_be_inlined@@YAXXZ", scope: !1, file: !1, line: 3, type: !9, isLocal: true, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
----------------
Presumably this type should be pruned from all the codeview test cases now, as unreferenced metadata?

================
Comment at: test/Linker/ditypeindex.ll:33-36
@@ +32,6 @@
+; SECONDFILE: !5 = !DIGlobalVariable(name: "c", scope: !0, file: !1, line: 1, type: !DITypeIndex(u0x13), isLocal: false, isDefinition: true, variable: i64* @c)
+; SECONDFILE: !6 = !{i32 2, !"CodeView", i32 1}
+; SECONDFILE: !7 = !{i32 2, !"Debug Info Version", i32 3}
+; SECONDFILE: !8 = !{i32 1, !"PIC Level", i32 2}
+; SECONDFILE: !9 = !{!"clang version 3.9.0 "}
+
----------------
Presumably you don't need to check that all this other stuff roundtrips - just the indexes, right? Your change didn't have anything to do with those other bits.

Oh, I see, these aren't CHECK lines, they're for mutating this file. - not sure if that's ideal/necessary. (& won't that mean this test "REQUIRES: shell"? Which is probably best avoided (I assume we generally try to avoid that, but don't know for sure), but should be added if you're going to keep the grep/sed/cat/etc)

I'd just write it as a second file - at least that's what we've usually done, I think.

================
Comment at: unittests/IR/MetadataTest.cpp:83
@@ +82,3 @@
+  DITypeRef getSubroutineType() {
+    return DITypeRef(
+        DISubroutineType::getDistinct(Context, 0, getNode(nullptr)));
----------------
Is this conversion implicit? (I think it is, but didn't entirely follow all the layers of macro goo, inheritance, etc)

================
Comment at: unittests/Transforms/Utils/Cloning.cpp:422
@@ -422,3 +421,3 @@
     DITypeRefArray ParamTypes = DBuilder.getOrCreateTypeArray(None);
-    DISubroutineType *DFuncType = DBuilder.createSubroutineType(ParamTypes);
+    DITypeRef DFuncType(DBuilder.createSubroutineType(ParamTypes));
     auto *CU =
----------------
Prefer copy init over direct init where both are valid:

  DITypeRef DFuncType = DBuilder.createSubroutineType(ParamTypes);

(this avoids any accidental explicit conversions from being performed)
(same comment on the previous instance of the same construct in this file)


http://reviews.llvm.org/D19236