[PATCH] D18518: Add an IR Verifier check for orphaned DICompileUnits.

[Adrian Prantl via llvm-commits]

> On Mar 28, 2016, at 11:40 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2016-Mar-28, at 11:16, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>> aprantl created this revision.
>> aprantl added reviewers: dexonsmith, dblaikie.
>> aprantl added subscribers: llvm-commits, loladiro.
>> 
>> Add an IR Verifier check for orphaned DICompileUnits. A DICompileUnit that is not listed in llvm.dbg.cu will cause assertion failures and/or crashes in the backend. The Verifier should reject this.
>> 
>> http://reviews.llvm.org/D18518
>> 
>> Files:
>> lib/IR/Verifier.cpp
>> test/Assembler/dicompileunit.ll
>> test/Transforms/LoopIdiom/debug-line.ll
>> test/Verifier/dbg-orphaned-compileunit.ll
>> tools/bugpoint/CrashDebugger.cpp
>> 
>> <D18518.51808.patch>
> 
>> Index: tools/bugpoint/CrashDebugger.cpp
>> ===================================================================
>> --- tools/bugpoint/CrashDebugger.cpp
>> +++ tools/bugpoint/CrashDebugger.cpp
>> @@ -552,7 +552,8 @@
>>   std::vector<NamedMDNode *> ToDelete;
>>   ToDelete.reserve(M->named_metadata_size() - Names.size());
>>   for (auto &NamedMD : M->named_metadata())
>> -    if (!Names.count(NamedMD.getName()))
>> +    // Always keep llvm.dbg.cu because the Verifier would complain.
>> +    if (!Names.count(NamedMD.getName()) && NamedMD.getName() != "llvm.dbg.cu")
> 
> How does this interact with Keno's changes to make bugpoint reduce debug
> info?  Will the compile units still get pruned?

Yes, there is a testcase BugPoint/named-mds.ll that verifies that DICompileUnits are still pruned. It’s just the named llvm.dbg.cu node that will no longer get removed.
I shall add a check for an empty CU list, which should be safe to remove.

> 
>>       ToDelete.push_back(&NamedMD);
>> 
>>   for (auto *NamedMD : ToDelete)
>> Index: test/Assembler/dicompileunit.ll
>> ===================================================================
>> --- test/Assembler/dicompileunit.ll
>> +++ test/Assembler/dicompileunit.ll
>> @@ -1,8 +1,10 @@
>> ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
>> ; RUN: verify-uselistorder %s
>> 
>> -; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9}
>> -!named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9}
>> +; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7}
>> +!named = !{!0, !1, !2, !3, !4, !5, !6, !7}
> 
> ^ You could leave !8 and !9 in !named.  Do you think that helps make it
> obvious that the numbering will be stable?  (Up to you.)

Yes, I can leave them in and comment why they are listed.

> 
>> +; CHECK: !llvm.dbg.cu = !{!8, !9}
>> +!llvm.dbg.cu = !{!8, !9}
>> 
>> !0 = distinct !{}
>> !1 = !DIFile(filename: "path/to/file", directory: "/path/to/dir")
>> @@ -24,3 +26,5 @@
>> !9 = distinct !DICompileUnit(language: 12, file: !1, producer: "",
>>                              isOptimized: false, flags: "", runtimeVersion: 0,
>>                              splitDebugFilename: "", emissionKind: 0)
>> +!llvm.module.flags = !{!10}
>> +!10 = !{i32 2, !"Debug Info Version", i32 3}
>> Index: lib/IR/Verifier.cpp
>> ===================================================================
>> --- lib/IR/Verifier.cpp
>> +++ lib/IR/Verifier.cpp
>> @@ -302,7 +306,9 @@
>>     visitModuleFlags(M);
>>     visitModuleIdents(M);
>> 
>> -    // Verify type referneces last.
> 
> s/referneces/references/
> 
>> +    verifyCompileUnits();
>> +
>> +    // Verify type references last.
>>     verifyTypeRefs();
>> 
>>     return !Broken;
>> @@ -4251,6 +4261,16 @@
>>   Assert(false, "unresolved type ref", S, N);
>> }
>> 
>> +void Verifier::verifyCompileUnits() {
>> +  auto *CUs = M->getNamedMetadata("llvm.dbg.cu");
>> +  SmallPtrSet<const Metadata *, 2> Listed;
>> +  if (CUs)
>> +    Listed.insert(CUs->op_begin(), CUs->op_end());
>> +  Assert(set_difference(CUVisited, Listed).empty(),
>> +         "All DICompileUnits must be listed in llvm.dbg.cu");
> 
> It seems more efficient to do this directly, rather than creating a
> third new set and checking that it's empty:
> 
>    Assert(CUVisited.size() <= Listed.size());
>    Assert(std::all_of(CUVisited.begin(), CUVisited.end(),
>               [&Listed](const Metadata *CU) { return Listed.count(CU); });
> 

Thanks!

> Maybe it's worth adding this to SetOperations.h (I'm not sure), but I
> think keeping the Verifier efficient is important.

I don’t know what a good name for such a function would be.


-- adrian
>> +  CUVisited.clear();
>> +}
>> +
>> void Verifier::verifyTypeRefs() {
>>   auto *CUs = M->getNamedMetadata("llvm.dbg.cu");
>>   if (!CUs)
>> 
>