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

[Duncan P. N. Exon Smith via llvm-commits]

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

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

> +; 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); });

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

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