[DWARFv5] Verify all-or-nothing constraint on DIFile source

[scott.linder]

Summary

Update IR verifier to check the constraint that DIFile source is present on all files or no files.

This may be the wrong approach, but it prevents a nasty cantFail during DWARF emission. After writing this patch I noticed MD5 checksums (which have the same restriction in DWARF) take a different approach of dropping the attribute and warning, see https://reviews.llvm.org/D48135.

[scott.linder]

Actually, after thinking about this I agree more with the approach of dropping or somehow warning/failing in the DWARF backend, rather than verifying at the IR level. Conceivably another DI backend which implements embedded source might not have the same restriction as DWARF, so restricting the IR doesn't seem right.

In the case of https://reviews.llvm.org/D53329 the dropping might be surprising, however, as the MD5 handling only seems to warn in the AsmParser.

[dblaikie]

In D54953#1309829, @scott.linder wrote:

Actually, after thinking about this I agree more with the approach of dropping or somehow warning/failing in the DWARF backend, rather than verifying at the IR level. Conceivably another DI backend which implements embedded source might not have the same restriction as DWARF, so restricting the IR doesn't seem right.

Yeah, I'd rather keep the restriction until we have a use case that merits removing it.

I'll leave the sign-off here to @aprantl or @JDevlieghere who have more familiarity/investment in the verifier.

[aprantl]

Is this constraint per-DICompileUnit or per llvm::Module? If it is per DICompileUnit then this implementation is too strict.

lib/IR/Verifier.cpp

1026 ↗

(On Diff #175490)

is there a less clever and more clear way to write this?

[scott.linder]

The constraint is only per-CU. I've attempted to capture this in the latest patch, although I don't quite understand the interaction of all of the DI* metadata, so I may have still missed cases.

[aprantl]

I'm assuming that with "per-CU" you mean a DICompileUnit, this LGTM.

[scott.linder]

I posted to the commit first by accident; copying here:

I realize now that this is insufficient. The other DI* metadata which have a file: (e.g. DILocalVariable) can point to an arbitrary DIFile, and I don't confirm that it matches the source: of all other files in the same DICompileUnit.

llvm/trunk/lib/IR/Verifier.cpp

class Verifier : public InstVisitor<Verifier>, VerifierSupport {
 
   /// Whether we've seen a call to @llvm.localescape in this function
   /// already.
   bool SawFrameEscape;
 
   /// Whether the current function has a DISubprogram attached to it.
   bool HasDebugInfo = false;
 
+  /// Whether source was present on the first DIFile encountered in each CU.
+  DenseMap<const DICompileUnit *, bool> HasSourceDebugInfo;
+
   /// Stores the count of how many objects were passed to llvm.localescape for a
   /// given function and the largest index passed to llvm.localrecover.
   DenseMap<Function *, std::pair<unsigned, unsigned>> FrameEscapeInfo;
 
   // Maps catchswitches and cleanuppads that unwind to siblings to the
   // terminators that indicate the unwind, used to detect cycles therein.
   MapVector<Instruction *, Instruction *> SiblingFuncletInfo;
 
#include "llvm/IR/Metadata.def"
   void verifyFnArgs(const DbgVariableIntrinsic &I);
 
   /// Module-level debug info verification...
   void verifyCompileUnits();
 
   /// Module-level verification that all @llvm.experimental.deoptimize
   /// declarations share the same calling convention.
   void verifyDeoptimizeCallingConvs();
+
+  /// Verify all-or-nothing property of DIFile source attribute within a CU.
+  void verifySourceDebugInfo(const DICompileUnit &U, const DIFile &F);
 };
 
 } // end anonymous namespace
 
 /// We know that cond should be true, if not print an error message.
 #define Assert(C, ...) \
   do { if (!(C)) { CheckFailed(__VA_ARGS__); return; } } while (false)
 
void Verifier::visitDICompileUnit(const DICompileUnit &N) {
 
   // Don't bother verifying the compilation directory or producer string
   // as those could be empty.
   AssertDI(N.getRawFile() && isa<DIFile>(N.getRawFile()), "invalid file", &N,
            N.getRawFile());
   AssertDI(!N.getFile()->getFilename().empty(), "invalid filename", &N,
            N.getFile());
 
+  verifySourceDebugInfo(N, *N.getFile());
+
   AssertDI((N.getEmissionKind() <= DICompileUnit::LastEmissionKind),
            "invalid emission kind", &N);
 
   if (auto *Array = N.getRawEnumTypes()) {
     AssertDI(isa<MDTuple>(Array), "invalid enum list", &N, Array);
     for (Metadata *Op : N.getEnumTypes()->operands()) {
       auto *Enum = dyn_cast_or_null<DICompositeType>(Op);
       AssertDI(Enum && Enum->getTag() == dwarf::DW_TAG_enumeration_type,
AssertDI(!hasConflictingReferenceFlags(N.getFlags()),
            "invalid reference flags", &N);
 
   auto *Unit = N.getRawUnit();
   if (N.isDefinition()) {
     // Subprogram definitions (not part of the type hierarchy).
     AssertDI(N.isDistinct(), "subprogram definitions must be distinct", &N);
     AssertDI(Unit, "subprogram definitions must have a compile unit", &N);
     AssertDI(isa<DICompileUnit>(Unit), "invalid unit type", &N, Unit);
+    if (N.getFile())
+      verifySourceDebugInfo(*N.getUnit(), *N.getFile());
   } else {
     // Subprogram declarations (part of the type hierarchy).
     AssertDI(!Unit, "subprogram declarations must not have a compile unit", &N);
   }
 
   if (auto *RawThrownTypes = N.getRawThrownTypes()) {
     auto *ThrownTypes = dyn_cast<MDTuple>(RawThrownTypes);
     AssertDI(ThrownTypes, "invalid thrown types list", &N, RawThrownTypes);
void Verifier::verifyDeoptimizeCallingConvs() {
   for (auto *F : makeArrayRef(DeoptimizeDeclarations).slice(1)) {
     Assert(First->getCallingConv() == F->getCallingConv(),
            "All llvm.experimental.deoptimize declarations must have the same "
            "calling convention",
            First, F);
   }
 }
 
+void Verifier::verifySourceDebugInfo(const DICompileUnit &U, const DIFile &F) {
+  bool HasSource = F.getSource().hasValue();
+  if (!HasSourceDebugInfo.count(&U))
+    HasSourceDebugInfo[&U] = HasSource;
+  AssertDI(HasSource == HasSourceDebugInfo[&U],
+           "inconsistent use of embedded source");
+}
+
 //===----------------------------------------------------------------------===//
 //  Implement the public interfaces to this file...
 //===----------------------------------------------------------------------===//
 
 bool llvm::verifyFunction(const Function &f, raw_ostream *OS) {
   Function &F = const_cast<Function &>(f);
 
   // Don't use a raw_null_ostream.  Printing IR is expensive.

llvm/trunk/test/Assembler/debug-info-source-invalid.ll

+; RUN: llvm-as < %s 2>&1 >/dev/null | FileCheck %s
+
+; Ensure we reject debug info where the DIFiles of a DICompileUnit mix source
+; and no-source.
+
+define dso_local void @foo() !dbg !5 {
+  ret void
+}
+
+define dso_local void @bar() !dbg !6 {
+  ret void
+}
+
+!llvm.dbg.cu = !{!4}
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 2, !"Dwarf Version", i32 5}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+
+!2 = !DIFile(filename: "foo.c", directory: "dir", source: "void foo() { }\0A")
+; CHECK: inconsistent use of embedded source
+; CHECK: warning: ignoring invalid debug info
+!3 = !DIFile(filename: "bar.h", directory: "dir")
+
+!4 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2)
+!5 = distinct !DISubprogram(name: "foo", file: !2, unit: !4)
+!6 = distinct !DISubprogram(name: "bar", file: !3, unit: !4)

llvm/trunk/test/Assembler/debug-info-source.ll

+; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
+; RUN: verify-uselistorder %s
+
+; Ensure we accept debug info where DIFiles within a DICompileUnit either all
+; have source, or none have source.
+
+define dso_local void @foo() !dbg !6 {
+  ret void
+}
+
+define dso_local void @bar() !dbg !7 {
+  ret void
+}
+
+define dso_local void @baz() !dbg !9 {
+  ret void
+}
+
+define dso_local void @qux() !dbg !11 {
+  ret void
+}
+
+!llvm.dbg.cu = !{!0, !2}
+!llvm.module.flags = !{!4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+; CHECK: !1 = !DIFile(filename: "foo.c", directory: "dir", source: "void foo() { }\0A")
+!1 = !DIFile(filename: "foo.c", directory: "dir", source: "void foo() { }\0A")
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3)
+; CHECK: !3 = !DIFile(filename: "qux.h", directory: "dir")
+!3 = !DIFile(filename: "qux.h", directory: "dir")
+!4 = !{i32 2, !"Dwarf Version", i32 5}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "foo", file: !1, unit: !0)
+!7 = distinct !DISubprogram(name: "bar", file: !8, unit: !0)
+; CHECK: !8 = !DIFile(filename: "bar.h", directory: "dir", source: "void bar() { }\0A")
+!8 = !DIFile(filename: "bar.h", directory: "dir", source: "void bar() { }\0A")
+!9 = distinct !DISubprogram(name: "baz", file: !10, unit: !2)
+; CHECK: !10 = !DIFile(filename: "baz.c", directory: "dir")
+!10 = !DIFile(filename: "baz.c", directory: "dir")
+!11 = distinct !DISubprogram(name: "qux", file: !3, unit: !2)