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

[Reid Kleckner via llvm-commits]

rnk marked 3 inline comments as done.
rnk added a comment.

In http://reviews.llvm.org/D19236#414087, @dblaikie wrote:

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


I changed local-variable.ll to have some 'unsigned' variables.


================
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);
----------------
dblaikie wrote:
> 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?)
IMO it is useful to allow users to emit codeview for IR that was generated with DWARF in mind. You'll still get line tables etc. Rejecting it in the verifier and asserting here would make that pretty hard.

================
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()))
----------------
dblaikie wrote:
> static_cast rather than C style cast?
You don't like C++ conversion-style casts? :)

================
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();
----------------
dblaikie wrote:
> Under what conditions would this fail? Is there a test case for it?
The verifier checks that this is a DISubroutineType, DITypeIndex, or null. 'resolve' internally casts to DIType, so if you attempt to generate DWARF from IR with type indices, you will crash here. You will also crash at every other 'resolve' call site, though.

Do you think we should apply the same logic that we have on the CV side and have 'resolve' return nullptr for a DITypeIndex?

================
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);
----------------
dblaikie wrote:
> 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?)
Oops, I assumed it was used for DI types, not LLVM IR types... Well, they have the same lifetimes. I'm going to update the doc comment to indicate that they also live here.

================
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 "}
+
----------------
dblaikie wrote:
> 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.
Fine. Personally, I find it convenient to be able to see the whole test case all at once, but it is not a common style and pretty confusing.

================
Comment at: unittests/IR/MetadataTest.cpp:83
@@ +82,3 @@
+  DITypeRef getSubroutineType() {
+    return DITypeRef(
+        DISubroutineType::getDistinct(Context, 0, getNode(nullptr)));
----------------
dblaikie wrote:
> Is this conversion implicit? (I think it is, but didn't entirely follow all the layers of macro goo, inheritance, etc)
No, it is not. I drafted a change to make it implicit, but I think Duncan said he'd rather not do that.

================
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 =
----------------
dblaikie wrote:
> 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)
Both instances need explicit conversion, though.


http://reviews.llvm.org/D19236