[DebugInfo] Skip ODRUniquing for mismatched tags

[ychen]

Summary

Otherwise, ODRUniquing would map some member method/variable MDNodes
to have enum type DIScope, resulting in invalid debug info and bad
DWARF.

• Add a Verifier check that when a 'scope:' operand is an ODR type that is not an enum.
• Makes ODRUniquing apply to only ODR types with the same tag so that the debuginfo/DWARF is well-formed.

[dblaikie]

I think this has come up before in llvm-dev threads, etc - could you link to those in the patch description for further context?

For other folks, bit of a design question: Perhaps we shouldn't special case enums here, and just say that if the ODR uniquing triggers on nodes with different tags, we don't deduplicate in that case? Clearly this is best-effort, I don't think we should go and do full structural equivalence but checking the tag seems cheap and if someone's violated the ODR by accident maybe this'd make it easier on them?

[ychen]

In D111770#3065057, @dblaikie wrote:

I think this has come up before in llvm-dev threads, etc - could you link to those in the patch description for further context?

Searched llvm-dev with "odr"/"LTO"/"uniquing" didn't find anything closely related. However, https://bugs.llvm.org/show_bug.cgi?id=24923 seems pretty relevant although it is of structure type ODR violation and this patch is for enum/non-enum type ODR violation.

[dblaikie]

In D111770#3065635, @ychen wrote:

In D111770#3065057, @dblaikie wrote:

I think this has come up before in llvm-dev threads, etc - could you link to those in the patch description for further context?

Searched llvm-dev with "odr"/"LTO"/"uniquing" didn't find anything closely related. However, https://bugs.llvm.org/show_bug.cgi?id=24923 seems pretty relevant although it is of structure type ODR violation and this patch is for enum/non-enum type ODR violation.

Ah, yeah, that's a generalization of the issue - might be worth fixing the general issue rather than only addressing this (more severe) subset of it. (@aprantl - thoughts?) Guess the generalization is checking both the tag type matches and the size of the type matches too?

Seems the case I was remembering was something @mtrofin came across internally at Google a couple of years ago ( Google internal: b/129559805 ) - yeah, I can't find lots of info on it other than the fact that we fixed the ODR violation by renaming an enum type, but doesn't look like we looked further into fixing (Thin)LTO to not fail in this particular way. (maybe @mtrofin remembers something more of that situation)

llvm/lib/IR/Verifier.cpp

864–870

Looks like this could be a function template rather than a macro?

[ychen]

• Skip ODR-type-uniquing for mismatched tags.
• Use function template for the verifier check.

[dblaikie]

This generally looks good to me - but I'd like @aprantl or similar to take a look/sign off from an "debug info verifier/invariants" perspective.

llvm/test/Linker/debug-info-bad-enum.ll

5

Probably also check that the resulting IR is good/correct? Like that the first CU has a reference to the enumeration type from the enum list, and the second CU has a reference to the structure_type from the retained types list?

[ychen]

• Fix unit tests

[ychen]

• Address David's feedback: add debug metadata checks.

[ychen]

llvm/test/Linker/debug-info-bad-enum.ll

5

Yep.

[aprantl]

From a pragmatic point of view this seems to make sense to me.

llvm/lib/IR/Verifier.cpp

847

Can you add a comment here explaining why we single out enum types?

llvm/test/Linker/debug-info-bad-enum.ll

3

Can you add a comment here explaining what behavior is being tested here?

[probinson]

I'm not a verifier expert, but I'm also not convinced the verifier change is doing quite the right thing. Yes, it finds the case we ran into, but it seems like it's just checking for that one bogus case where the containing scope is incorrectly an enum. It seems to me that proper scope-checking should be looking for the tag to be something that we agree should be allowed to be a scope, rather than looking for the tag to be one thing (of many?) that we agree should not be a scope.

[ychen]

• Address Adrian's comments.

[ychen]

In D111770#3081290, @probinson wrote:

I'm not a verifier expert, but I'm also not convinced the verifier change is doing quite the right thing. Yes, it finds the case we ran into, but it seems like it's just checking for that one bogus case where the containing scope is incorrectly an enum. It seems to me that proper scope-checking should be looking for the tag to be something that we agree should be allowed to be a scope, rather than looking for the tag to be one thing (of many?) that we agree should not be a scope.

Yeah, I think the function name "verifyScopeOperand" is misleading in that it was not meant to check scope operand in general, but only for when an ODR type is the scope operand. So I renamed it to "verifyODRTypeAsScopeOperand" instead and add a TODO for the general case. And all of the supported tags for DICompositeType, I'm only sure that enum could not be a scope. The modified unit test does the basic checking that mismatched tags do not unique with each other. Does this sound good to you?

[ychen]

• rebase

[probinson]

Given that enum is really the only problematic case, LGTM.

llvm/lib/IR/DebugInfoMetadata.cpp

DICompositeType *DICompositeType::buildODRType(
   auto *&CT = (*Context.pImpl->DITypeMap)[&Identifier];
   if (!CT)
     return CT = DICompositeType::getDistinct(
                Context, Tag, Name, File, Line, Scope, BaseType, SizeInBits,
                AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang,
                VTableHolder, TemplateParams, &Identifier, Discriminator,
                DataLocation, Associated, Allocated, Rank, Annotations);
 
+  if (CT->getTag() != Tag)
+    return nullptr;
+
   // Only mutate CT if it's a forward declaration and the new operands aren't.
   assert(CT->getRawIdentifier() == &Identifier && "Wrong ODR identifier?");
   if (!CT->isForwardDecl() || (Flags & DINode::FlagFwdDecl))
     return CT;
 
   // Mutate CT in place.  Keep this in sync with getImpl.
   CT->mutate(Tag, Line, RuntimeLang, SizeInBits, AlignInBits, OffsetInBits,
              Flags);
DICompositeType *DICompositeType::getODRType(
     DIFlags Flags, Metadata *Elements, unsigned RuntimeLang,
     Metadata *VTableHolder, Metadata *TemplateParams, Metadata *Discriminator,
     Metadata *DataLocation, Metadata *Associated, Metadata *Allocated,
     Metadata *Rank, Metadata *Annotations) {
   assert(!Identifier.getString().empty() && "Expected valid identifier");
   if (!Context.isODRUniquingDebugTypes())
     return nullptr;
   auto *&CT = (*Context.pImpl->DITypeMap)[&Identifier];
-  if (!CT)
+  if (!CT) {
     CT = DICompositeType::getDistinct(
         Context, Tag, Name, File, Line, Scope, BaseType, SizeInBits,
         AlignInBits, OffsetInBits, Flags, Elements, RuntimeLang, VTableHolder,
         TemplateParams, &Identifier, Discriminator, DataLocation, Associated,
         Allocated, Rank, Annotations);
+  } else {
+    if (CT->getTag() != Tag)
+      return nullptr;
+  }
   return CT;
 }
 
 DICompositeType *DICompositeType::getODRTypeIfExists(LLVMContext &Context,
                                                      MDString &Identifier) {
   assert(!Identifier.getString().empty() && "Expected valid identifier");
   if (!Context.isODRUniquingDebugTypes())
     return nullptr;

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 verifyScopeOperand(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::verifyScopeOperand(const MDNode &MD, T *) {
+  if (isa<T>(MD)) {
+    if (DIScope *N = cast<T>(MD).getScope())
+      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);
 
+  verifyScopeOperand<DIType>(MD);
+  verifyScopeOperand<DILocalScope>(MD);
+  verifyScopeOperand<DIVariable>(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));                                             \
     break;
 #include "llvm/IR/Metadata.def"
   }
 

llvm/test/Linker/debug-info-bad-enum.ll

+; RUN: split-file %s %t
+; RUN: llvm-link -o - %t/a.ll %t/b.ll 2>&1 | FileCheck %s
+
+; CHECK-NOT: enum type is not a scope; check enum type ODR violation
+
+;--- a.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang", isOptimized: true, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "a.cpp", directory: "file")
+!2 = !{!3}
+!3 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "Stage", file: !1, line: 3, baseType: !4, size: 32, elements: !5, identifier: "_ZTS5Stage")
+!4 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!5 = !{!6}
+!6 = !DIEnumerator(name: "A1", value: 0, isUnsigned: true)
+!9 = !{i32 2, !"Debug Info Version", i32 3}
+
+
+;--- b.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!11}
+
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !3, producer: "clang", isOptimized: true, emissionKind: FullDebug, retainedTypes: !10, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "b.cpp", directory: "file")
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!6 = !DIDerivedType(tag: DW_TAG_member, name: "Var", scope: !8, file: !3, line: 5, baseType: !5, flags: DIFlagStaticMember)
+!8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "Stage", file: !3, line: 3, size: 64, flags: DIFlagTypePassByValue, elements: !9, identifier: "_ZTS5Stage")
+!9 = !{!6}
+!10 = !{!6}
+!11 = !{i32 2, !"Debug Info Version", i32 3}

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}

llvm/unittests/IR/DebugTypeODRUniquingTest.cpp

TEST(DebugTypeODRUniquingTest, buildODRType) {
   auto &CT = *DICompositeType::buildODRType(
       Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0, nullptr,
       nullptr, 0, 0, 0, DINode::FlagFwdDecl, nullptr, 0, nullptr, nullptr,
       nullptr, nullptr, nullptr, nullptr, nullptr, nullptr);
   EXPECT_EQ(&CT, DICompositeType::getODRTypeIfExists(Context, UUID));
   EXPECT_EQ(dwarf::DW_TAG_class_type, CT.getTag());
 
   // Update with another forward decl.  This should be a no-op.
-  EXPECT_EQ(&CT,
-            DICompositeType::buildODRType(
-                Context, UUID, dwarf::DW_TAG_structure_type, nullptr, nullptr,
-                0, nullptr, nullptr, 0, 0, 0, DINode::FlagFwdDecl, nullptr, 0,
-                nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
-                nullptr));
-  EXPECT_EQ(dwarf::DW_TAG_class_type, CT.getTag());
+  EXPECT_EQ(&CT, DICompositeType::buildODRType(
+                     Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr,
+                     0, nullptr, nullptr, 0, 0, 0, DINode::FlagFwdDecl, nullptr,
+                     0, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+                     nullptr, nullptr));
+
+  EXPECT_FALSE(DICompositeType::buildODRType(
+      Context, UUID, dwarf::DW_TAG_structure_type, nullptr, nullptr, 0, nullptr,
+      nullptr, 0, 0, 0, DINode::FlagFwdDecl, nullptr, 0, nullptr, nullptr,
+      nullptr, nullptr, nullptr, nullptr, nullptr, nullptr));
 
   // Update with a definition.  This time we should see a change.
-  EXPECT_EQ(&CT,
-            DICompositeType::buildODRType(
-                Context, UUID, dwarf::DW_TAG_structure_type, nullptr, nullptr,
+  EXPECT_EQ(&CT, DICompositeType::buildODRType(
+                     Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr,
                      0, nullptr, nullptr, 0, 0, 0, DINode::FlagZero, nullptr, 0,
-                nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
-                nullptr));
-  EXPECT_EQ(dwarf::DW_TAG_structure_type, CT.getTag());
+                     nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+                     nullptr, nullptr));
+  EXPECT_FALSE(CT.isForwardDecl());
 
   // Further updates should be ignored.
-  EXPECT_EQ(&CT,
-            DICompositeType::buildODRType(
-                Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0,
-                nullptr, nullptr, 0, 0, 0, DINode::FlagFwdDecl, nullptr, 0,
-                nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
-                nullptr));
-  EXPECT_EQ(dwarf::DW_TAG_structure_type, CT.getTag());
-  EXPECT_EQ(&CT,
-            DICompositeType::buildODRType(
-                Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr, 0,
-                nullptr, nullptr, 0, 0, 0, DINode::FlagZero, nullptr, 0,
-                nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
-                nullptr));
-  EXPECT_EQ(dwarf::DW_TAG_structure_type, CT.getTag());
+  EXPECT_EQ(&CT, DICompositeType::buildODRType(
+                     Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr,
+                     0, nullptr, nullptr, 0, 0, 0, DINode::FlagFwdDecl, nullptr,
+                     0, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+                     nullptr, nullptr));
+  EXPECT_FALSE(CT.isForwardDecl());
+  EXPECT_EQ(&CT, DICompositeType::buildODRType(
+                     Context, UUID, dwarf::DW_TAG_class_type, nullptr, nullptr,
+                     111u, nullptr, nullptr, 0, 0, 0, DINode::FlagZero, nullptr,
+                     0, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+                     nullptr, nullptr));
+  EXPECT_NE(111u, CT.getLine());
 }
 
 TEST(DebugTypeODRUniquingTest, buildODRTypeFields) {
   LLVMContext Context;
   Context.enableDebugTypeODRUniquing();
 
   // Build an ODR type that's a forward decl with no other fields set.
   MDString &UUID = *MDString::get(Context, "UUID");
   auto &CT = *DICompositeType::buildODRType(
       Context, UUID, 0, nullptr, nullptr, 0, nullptr, nullptr, 0, 0, 0,
       DINode::FlagFwdDecl, nullptr, 0, nullptr, nullptr, nullptr, nullptr,
       nullptr, nullptr, nullptr, nullptr);
 
 // Create macros for running through all the fields except Identifier and Flags.
 #define FOR_EACH_MDFIELD()                                                     \
   DO_FOR_FIELD(Name)                                                           \
   DO_FOR_FIELD(File)                                                           \
   DO_FOR_FIELD(Scope)                                                          \
   DO_FOR_FIELD(BaseType)                                                       \
   DO_FOR_FIELD(Elements)                                                       \
   DO_FOR_FIELD(VTableHolder)                                                   \
   DO_FOR_FIELD(TemplateParams)
 #define FOR_EACH_INLINEFIELD()                                                 \
-  DO_FOR_FIELD(Tag)                                                            \
   DO_FOR_FIELD(Line)                                                           \
   DO_FOR_FIELD(SizeInBits)                                                     \
   DO_FOR_FIELD(AlignInBits)                                                    \
   DO_FOR_FIELD(OffsetInBits)                                                   \
   DO_FOR_FIELD(RuntimeLang)
 
 // Create all the fields.
 #define DO_FOR_FIELD(X) auto *X = MDString::get(Context, #X);
   FOR_EACH_MDFIELD();
 #undef DO_FOR_FIELD
   unsigned NonZeroInit = 0;
 #define DO_FOR_FIELD(X) auto X = ++NonZeroInit;
   FOR_EACH_INLINEFIELD();
 #undef DO_FOR_FIELD
 
   // Replace all the fields with new values that are distinct from each other.
   EXPECT_EQ(&CT,
             DICompositeType::buildODRType(
-                Context, UUID, Tag, Name, File, Line, Scope, BaseType,
-                SizeInBits, AlignInBits, OffsetInBits, DINode::FlagArtificial,
-                Elements, RuntimeLang, VTableHolder, TemplateParams, nullptr,
-                nullptr, nullptr, nullptr, nullptr, nullptr));
+                Context, UUID, 0, Name, File, Line, Scope, BaseType, SizeInBits,
+                AlignInBits, OffsetInBits, DINode::FlagArtificial, Elements,
+                RuntimeLang, VTableHolder, TemplateParams, nullptr, nullptr,
+                nullptr, nullptr, nullptr, nullptr));
 
   // Confirm that all the right fields got updated.
 #define DO_FOR_FIELD(X) EXPECT_EQ(X, CT.getRaw##X());
   FOR_EACH_MDFIELD();
 #undef DO_FOR_FIELD
 #undef FOR_EACH_MDFIELD
 #define DO_FOR_FIELD(X) EXPECT_EQ(X, CT.get##X());
   FOR_EACH_INLINEFIELD();
 #undef DO_FOR_FIELD
 #undef FOR_EACH_INLINEFIELD
   EXPECT_EQ(DINode::FlagArtificial, CT.getFlags());
   EXPECT_EQ(&UUID, CT.getRawIdentifier());
 }
 
 } // end namespace