Verifier: accept enums as scopes

[durin42]

Summary

Rust allows enums to be scopes, as shown by the previous change. Sadly,
D111770 disallowed enums-as-scopes in the LLVM Verifier, which means
that LLVM HEAD stopped working for Rust compiles. As a result, we back
out the verifier part of D111770 with a modification to the testcase so
we don't break this in the future.

The testcase is now actual IR from rustc at commit 8f8092cc3, which is
the nightly as of 2021-09-28. I would expect rustc 1.57 to produce
similar or identical IR if someone wants to reproduce this IR in the
future with minimal changes. A recipe for reproducing the IR using rustc
is included in the test file.

[ychen]

Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?

[durin42]

No worries on the break, it happens. I'm not familiar enough with how the validation works to know if there's a way to have C++-specific validations.

[ychen]

llvm/lib/IR/Verifier.cpp

888

This should work.

if (dwarf::isCPlusPlus(CurrentSourceLang)) {
  verifyODRTypeAsScopeOperand<DIType>(MD);
  verifyODRTypeAsScopeOperand<DILocalScope>(MD);
}

[dblaikie]

In D115353#3179986, @ychen wrote:

Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?

Reckon it's worth the special case, though? I don't feel super strongly either way - I guess we already do have the "CurrentSourceLang" there and use it for some fortran special case, so we could test it here too. But I'm not sure how valuable it is?

The original bug/issue should be caught during IR merging instead - and since the goal is to support the case of member functions in enums, it's not like the LLVM codegen can't handle this case (when it's C++ or any other language).

(I assume the verifier check was added originally because members in enums broke LLVM in some way? But that bug has been/is being fixed so Rust keeps working? Or did it never break LLVM but instead just produce "weird" DWARF?)

[ychen]

In D115353#3183713, @dblaikie wrote:

In D115353#3179986, @ychen wrote:

Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?

Reckon it's worth the special case, though? I don't feel super strongly either way - I guess we already do have the "CurrentSourceLang" there and use it for some fortran special case, so we could test it here too. But I'm not sure how valuable it is?

The original bug/issue should be caught during IR merging instead - and since the goal is to support the case of member functions in enums, it's not like the LLVM codegen can't handle this case (when it's C++ or any other language).

(I assume the verifier check was added originally because members in enums broke LLVM in some way? But that bug has been/is being fixed so Rust keeps working? Or did it never break LLVM but instead just produce "weird" DWARF?)

It didn't break LLVM, just the debug IR does not make sense for C++. I was suggesting the "CurrentSourceLang" method so there is no need to think about if the issue may happen in any other way than IR merging. IMHO, it is valuable than losing a few lines of code. Since it is cheap enough to keep and maintain, I'd prefer to keep the check.

[dblaikie]

In D115353#3183820, @ychen wrote:

In D115353#3183713, @dblaikie wrote:

In D115353#3179986, @ychen wrote:

Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?

Reckon it's worth the special case, though? I don't feel super strongly either way - I guess we already do have the "CurrentSourceLang" there and use it for some fortran special case, so we could test it here too. But I'm not sure how valuable it is?

The original bug/issue should be caught during IR merging instead - and since the goal is to support the case of member functions in enums, it's not like the LLVM codegen can't handle this case (when it's C++ or any other language).

(I assume the verifier check was added originally because members in enums broke LLVM in some way? But that bug has been/is being fixed so Rust keeps working? Or did it never break LLVM but instead just produce "weird" DWARF?)

It didn't break LLVM, just the debug IR does not make sense for C++. I was suggesting the "CurrentSourceLang" method so there is no need to think about if the issue may happen in any other way than IR merging. IMHO, it is valuable than losing a few lines of code. Since it is cheap enough to keep and maintain, I'd prefer to keep the check.

I guess the counterargument might be that there's probably lots of invariants for what constitutes valid/reasonable DWARF for C++ - but we don't comprehensively try to check for all those things, why would we check for this thing in particular?

Generally the IR verifier is more about: "If the IR is verified it won't crash LLVM/will produce valid DWARF"

[ychen]

In D115353#3183911, @dblaikie wrote:

In D115353#3183820, @ychen wrote:

In D115353#3183713, @dblaikie wrote:

In D115353#3179986, @ychen wrote:

Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?

Reckon it's worth the special case, though? I don't feel super strongly either way - I guess we already do have the "CurrentSourceLang" there and use it for some fortran special case, so we could test it here too. But I'm not sure how valuable it is?

The original bug/issue should be caught during IR merging instead - and since the goal is to support the case of member functions in enums, it's not like the LLVM codegen can't handle this case (when it's C++ or any other language).

(I assume the verifier check was added originally because members in enums broke LLVM in some way? But that bug has been/is being fixed so Rust keeps working? Or did it never break LLVM but instead just produce "weird" DWARF?)

It didn't break LLVM, just the debug IR does not make sense for C++. I was suggesting the "CurrentSourceLang" method so there is no need to think about if the issue may happen in any other way than IR merging. IMHO, it is valuable than losing a few lines of code. Since it is cheap enough to keep and maintain, I'd prefer to keep the check.

I guess the counterargument might be that there's probably lots of invariants for what constitutes valid/reasonable DWARF for C++ - but we don't comprehensively try to check for all those things, why would we check for this thing in particular?

Generally the IR verifier is more about: "If the IR is verified it won't crash LLVM/will produce valid DWARF"

Yeah, agreed that this is a rare special casing for C++, if it is preferred to be avoided, I think it makes sense to remove the check. Feel free to accept the patch assuming it looks good to you. Thanks for chiming in :-).

[dblaikie]

In D115353#3183991, @ychen wrote:

In D115353#3183911, @dblaikie wrote:

In D115353#3183820, @ychen wrote:

In D115353#3183713, @dblaikie wrote:

In D115353#3179986, @ychen wrote:

Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?

Reckon it's worth the special case, though? I don't feel super strongly either way - I guess we already do have the "CurrentSourceLang" there and use it for some fortran special case, so we could test it here too. But I'm not sure how valuable it is?

The original bug/issue should be caught during IR merging instead - and since the goal is to support the case of member functions in enums, it's not like the LLVM codegen can't handle this case (when it's C++ or any other language).

(I assume the verifier check was added originally because members in enums broke LLVM in some way? But that bug has been/is being fixed so Rust keeps working? Or did it never break LLVM but instead just produce "weird" DWARF?)

It didn't break LLVM, just the debug IR does not make sense for C++. I was suggesting the "CurrentSourceLang" method so there is no need to think about if the issue may happen in any other way than IR merging. IMHO, it is valuable than losing a few lines of code. Since it is cheap enough to keep and maintain, I'd prefer to keep the check.

I guess the counterargument might be that there's probably lots of invariants for what constitutes valid/reasonable DWARF for C++ - but we don't comprehensively try to check for all those things, why would we check for this thing in particular?

Generally the IR verifier is more about: "If the IR is verified it won't crash LLVM/will produce valid DWARF"

Yeah, agreed that this is a rare special casing for C++, if it is preferred to be avoided, I think it makes sense to remove the check. Feel free to accept the patch assuming it looks good to you. Thanks for chiming in :-).

Fair - I appreciate the discussion, but think I'll lean towards not including these checks.

[durin42]

I don't have commit access, so could someone push this for me?

Thanks!

llvm/lib/IR/Verifier.cpp

#include "llvm/IR/Metadata.def"
   bool verifyAttributeCount(AttributeList Attrs, unsigned Params);
   void verifyAttributeTypes(AttributeSet Attrs, const Value *V);
   void verifyParameterAttrs(AttributeSet Attrs, Type *Ty, const Value *V);
   void checkUnsignedBaseTenFuncAttr(AttributeList Attrs, StringRef Attr,
                                     const Value *V);
   void verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
                            const Value *V, bool IsIntrinsic);
   void verifyFunctionMetadata(ArrayRef<std::pair<unsigned, MDNode *>> MDs);
-  template <typename T>
-  void verifyODRTypeAsScopeOperand(const MDNode &MD, T * = nullptr);
 
   void visitConstantExprsRecursively(const Constant *EntryC);
   void visitConstantExpr(const ConstantExpr *CE);
   void verifyStatepoint(const CallBase &Call);
   void verifyFrameRecoverIndices();
   void verifySiblingFuncletUnwinds();
 
   void verifyFragmentExpression(const DbgVariableIntrinsic &I);
for (const MDNode *MD : NMD.operands()) {
 
     if (!MD)
       continue;
 
     visitMDNode(*MD, AreDebugLocsAllowed::Yes);
   }
 }
 
-template <typename T>
-void Verifier::verifyODRTypeAsScopeOperand(const MDNode &MD, T *) {
-  if (isa<T>(MD)) {
-    if (auto *N = dyn_cast_or_null<DICompositeType>(cast<T>(MD).getScope()))
-      // Of all the supported tags for DICompositeType(see visitDICompositeType)
-      // we know that enum type cannot be a scope.
-      AssertDI(N->getTag() != dwarf::DW_TAG_enumeration_type,
-               "enum type is not a scope; check enum type ODR "
-               "violation",
-               N, &MD);
-  }
-}
-
 void Verifier::visitMDNode(const MDNode &MD, AreDebugLocsAllowed AllowLocs) {
   // Only visit each node once.  Metadata can be mutually recursive, so this
   // avoids infinite recursion here, as well as being an optimization.
   if (!MDNodes.insert(&MD).second)
     return;
 
   Assert(&MD.getContext() == &Context,
          "MDNode context does not match Module context!", &MD);
 
-  // Makes sure when a scope operand is a ODR type, the ODR type uniquing does
-  // not create invalid debug metadata.
-  // TODO: check that the non-ODR-type scope operand is valid.
-  verifyODRTypeAsScopeOperand<DIType>(MD);
-  verifyODRTypeAsScopeOperand<DILocalScope>(MD);
-
   switch (MD.getMetadataID()) {
   default:
     llvm_unreachable("Invalid MDNode subclass");
   case Metadata::MDTupleKind:
     break;
 #define HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS)                                  \
   case Metadata::CLASS##Kind:                                                  \
     visit##CLASS(cast<CLASS>(MD));                                             \

llvm/test/DebugInfo/dbg-rust-valid-enum-as-scope.ll

+; RUN: %llc_dwarf -filetype=obj -o - %s | llvm-dwarfdump -| FileCheck --implicit-check-not=DW_TAG --implicit-check-not=NULL %s
+; CHECK: DW_TAG_compile_unit
+; CHECK:   DW_AT_language (DW_LANG_Rust)
+; CHECK:   DW_TAG_namespace
+; CHECK:     DW_TAG_enumeration_type
+; CHECK:     DW_AT_name ("E")
+; CHECK:       DW_TAG_enumerator
+; CHECK:       DW_TAG_enumerator
+; CHECK:       DW_TAG_subprogram
+; CHECK:         DW_AT_name ("f")
+; CHECK:         DW_TAG_formal_parameter
+; CHECK:         NULL
+; CHECK:       NULL
+; CHECK:     NULL
+; CHECK:   DW_TAG_pointer_type
+; CHECK:   NULL
+
+; This file comes from rustc output, with the input program
+;         pub enum E { A, B }
+;         impl E {
+;             pub fn f(&self) {}
+;         }
+; compiled with `rustc --crate-type=lib a.rs --emit llvm-ir -g` and
+; copying the resulting `a.ll` file to here. This was done with rustc
+; at nightly from 2021-09-28 (git 8f8092cc3), but rustc 1.57 should
+; produce similar or identical output.
+
+; ModuleID = 'a.a146b597-cgu.0'
+source_filename = "a.a146b597-cgu.0"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.7.0"
+
+; a::E::f
+; Function Attrs: uwtable
+define void @_ZN1a1E1f17h4fcb50ce732fb2a7E(i8* align 1 dereferenceable(1) %self) unnamed_addr #0 !dbg !13 {
+start:
+  %self.dbg.spill = alloca i8*, align 8
+  store i8* %self, i8** %self.dbg.spill, align 8
+  call void @llvm.dbg.declare(metadata i8** %self.dbg.spill, metadata !19, metadata !DIExpression()), !dbg !21
+  ret void, !dbg !22
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { uwtable "frame-pointer"="all" "probe-stack"="__rust_probestack" "target-cpu"="core2" }
+attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.module.flags = !{!0, !1, !2}
+!llvm.dbg.cu = !{!3}
+
+!0 = !{i32 7, !"PIC Level", i32 2}
+!1 = !{i32 2, !"Dwarf Version", i32 2}
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DICompileUnit(language: DW_LANG_Rust, file: !4, producer: "clang LLVM (rustc version 1.57.0-nightly (8f8092cc3 2021-09-28))", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5)
+!4 = !DIFile(filename: "a.rs/@/a.a146b597-cgu.0", directory: "/Users/augie")
+!5 = !{!6}
+!6 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "E", scope: !8, file: !7, baseType: !9, size: 8, align: 8, flags: DIFlagEnumClass, elements: !10)
+!7 = !DIFile(filename: "<unknown>", directory: "")
+!8 = !DINamespace(name: "a", scope: null)
+!9 = !DIBasicType(name: "u8", size: 8, encoding: DW_ATE_unsigned)
+!10 = !{!11, !12}
+!11 = !DIEnumerator(name: "A", value: 0)
+!12 = !DIEnumerator(name: "B", value: 1)
+!13 = distinct !DISubprogram(name: "f", linkageName: "_ZN1a1E1f17h4fcb50ce732fb2a7E", scope: !6, file: !14, line: 3, type: !15, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !3, templateParams: !20, retainedNodes: !18)
+!14 = !DIFile(filename: "a.rs", directory: "/Users/augie", checksumkind: CSK_MD5, checksum: "ab4ce84c27ef6fd0be1ef78e8131faa8")
+!15 = !DISubroutineType(types: !16)
+!16 = !{null, !17}
+!17 = !DIDerivedType(tag: DW_TAG_pointer_type, name: "&E", baseType: !6, size: 64, align: 64, dwarfAddressSpace: 0)
+!18 = !{!19}
+!19 = !DILocalVariable(name: "self", arg: 1, scope: !13, file: !14, line: 3, type: !17)
+!20 = !{}
+!21 = !DILocation(line: 3, column: 14, scope: !13)
+!22 = !DILocation(line: 3, column: 23, scope: !13)

llvm/test/Verifier/dbg-invalid-enum-as-scope.ll

-; RUN: llvm-as -disable-output <%s 2>&1 | FileCheck %s
-; CHECK: enum type is not a scope; check enum type ODR violation
-; CHECK: warning: ignoring invalid debug info
-
-!llvm.module.flags = !{!0}
-!0 = !{i32 2, !"Debug Info Version", i32 3}
-!llvm.dbg.cu = !{!1}
-!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, retainedTypes: !13, enums: !3)
-!2 = !DIFile(filename: "file.c", directory: "dir")
-!3 = !{!4}
-!4 = distinct !DICompositeType(tag: DW_TAG_enumeration_type, name: "Stage", file: !2, line: 3, baseType: !10, size: 32, elements: !11, identifier: "_ZTS5Stage")
-!6 = !DIDerivedType(tag: DW_TAG_member, name: "Var", scope: !4, file: !2, line: 5, baseType: !10)
-!10 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
-!11 = !{!12}
-!12 = !DIEnumerator(name: "A1", value: 0, isUnsigned: true)
-!13 = !{!6}