<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 27, 2016 at 12:47 PM, Reid Kleckner via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">rnk marked 3 inline comments as done.<br>
rnk added a comment.<br>
<span class=""><br>
In <a href="http://reviews.llvm.org/D19236#414087" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19236#414087</a>, @dblaikie wrote:<br>
<br>
> 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?)<br>
<br>
<br>
</span>I changed local-variable.ll to have some 'unsigned' variables.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:712-713<br>
@@ -711,1 +711,4 @@<br>
<br>
+ // Pull the type index out of the type field. If the frontend did not provide<br>
+ // a DITypeIndex, emit zero for "no type".<br>
+ unsigned TI = unsigned(SimpleTypeKind::None);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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?)<br>
</span>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.<br></blockquote><div><br></div><div>I'm not sure that's a middle ground we really want to sign up to support/figure out, though. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:714<br>
@@ +713,3 @@<br>
+ // a DITypeIndex, emit zero for "no type".<br>
+ unsigned TI = unsigned(SimpleTypeKind::None);<br>
+ if (auto *DTI = dyn_cast_or_null<DITypeIndex>(Var.DIVar->getType()))<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> static_cast rather than C style cast?<br>
</span>You don't like C++ conversion-style casts? :)<br></blockquote><div><br></div><div>I'd be hard pressed to actually demonstrate the relative use of c-style versus C++ casts across the codebase (only because it's really hard to search for the C ones), but I'd guess we're pretty consistent about using the C++ style casts & consider them safer & more self documenting (because they do a strict subset).<br><br>At least I'm pretty sure if we do use C-style casts we write them this way "(T)t" rather than in the function-style syntax, fairly regularly?<br><br>It also looks extra weird in an initialization, you could just write it as: "unsigned TI(SimpleTypeKind::None)"; (or use auto, if the type's explicit on the RHS anyway)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:591-592<br>
@@ +590,4 @@<br>
+ DITypeRefArray FnArgs;<br>
+ if (DISubroutineType *FnTy =<br>
+ cast_or_null<DISubroutineType>(resolve(Sub->getType())))<br>
+ FnArgs = FnTy->getTypeArray();<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Under what conditions would this fail? Is there a test case for it?<br>
</span>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.<br></blockquote><div><br></div><div>I'm not quite following - I mean when does this conditional evaluate to 'false', not when does it crash. Does this ever evaluate to false? If the type reference is a null pointer?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Do you think we should apply the same logic that we have on the CV side and have 'resolve' return nullptr for a DITypeIndex?<br></blockquote><div><br></div><div>No, I don't think so - (& I think we should probably be symmetric and make it invalid the other way too - referring to the above discussion on the CV side)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/IR/DebugInfoMetadata.cpp:639-640<br>
@@ +638,4 @@<br>
+ if (!Slot) {<br>
+ void *Mem = Context.pImpl->TypeAllocator.Allocate(sizeof(DITypeIndex),<br>
+ alignof(DITypeIndex));<br>
+ Slot = new (Mem) DITypeIndex(Index);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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?)<br>
</span>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.<br></blockquote><div><br></div><div>OK</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: test/Linker/ditypeindex.ll:33-36<br>
@@ +32,6 @@<br>
+; SECONDFILE: !5 = !DIGlobalVariable(name: "c", scope: !0, file: !1, line: 1, type: !DITypeIndex(u0x13), isLocal: false, isDefinition: true, variable: i64* @c)<br>
+; SECONDFILE: !6 = !{i32 2, !"CodeView", i32 1}<br>
+; SECONDFILE: !7 = !{i32 2, !"Debug Info Version", i32 3}<br>
+; SECONDFILE: !8 = !{i32 1, !"PIC Level", i32 2}<br>
+; SECONDFILE: !9 = !{!"clang version 3.9.0 "}<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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.<br>
><br>
> 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)<br>
><br>
> I'd just write it as a second file - at least that's what we've usually done, I think.<br>
</span>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.<br>
<span class=""><br>
================<br>
Comment at: unittests/IR/MetadataTest.cpp:83<br>
@@ +82,3 @@<br>
+ DITypeRef getSubroutineType() {<br>
+ return DITypeRef(<br>
+ DISubroutineType::getDistinct(Context, 0, getNode(nullptr)));<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Is this conversion implicit? (I think it is, but didn't entirely follow all the layers of macro goo, inheritance, etc)<br>
</span>No, it is not. I drafted a change to make it implicit, but I think Duncan said he'd rather not do that.<br></blockquote><div><br></div><div>OK</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: unittests/Transforms/Utils/Cloning.cpp:422<br>
@@ -422,3 +421,3 @@<br>
DITypeRefArray ParamTypes = DBuilder.getOrCreateTypeArray(None);<br>
- DISubroutineType *DFuncType = DBuilder.createSubroutineType(ParamTypes);<br>
+ DITypeRef DFuncType(DBuilder.createSubroutineType(ParamTypes));<br>
auto *CU =<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Prefer copy init over direct init where both are valid:<br>
><br>
> DITypeRef DFuncType = DBuilder.createSubroutineType(ParamTypes);<br>
><br>
> (this avoids any accidental explicit conversions from being performed)<br>
> (same comment on the previous instance of the same construct in this file)<br>
</span>Both instances need explicit conversion, though.<br></blockquote><div><br></div><div>Ah, my mistake - thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D19236" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19236</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>