[llvm] 7c3fa52 - [DebugInfo] Skip ODRUniquing for mismatched tags
Yuanfang Chen via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 26 15:28:52 PDT 2021
Author: Yuanfang Chen
Date: 2021-10-26T15:28:25-07:00
New Revision: 7c3fa5278544d3830838148ac912cc4b927d1b5b
URL: https://github.com/llvm/llvm-project/commit/7c3fa5278544d3830838148ac912cc4b927d1b5b
DIFF: https://github.com/llvm/llvm-project/commit/7c3fa5278544d3830838148ac912cc4b927d1b5b.diff
LOG: [DebugInfo] Skip ODRUniquing for mismatched tags
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.
Reviewed By: probinson, aprantl
Differential Revision: https://reviews.llvm.org/D111770
Added:
llvm/test/Linker/debug-info-bad-enum.ll
llvm/test/Verifier/dbg-invalid-enum-as-scope.ll
Modified:
llvm/lib/IR/DebugInfoMetadata.cpp
llvm/lib/IR/Verifier.cpp
llvm/unittests/IR/DebugTypeODRUniquingTest.cpp
Removed:
################################################################################
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index 918c4dec64049..66d436520ee78 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -640,6 +640,9 @@ DICompositeType *DICompositeType::buildODRType(
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))
@@ -672,12 +675,16 @@ DICompositeType *DICompositeType::getODRType(
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;
}
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 5a84561758054..de092ec632f4c 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -549,6 +549,8 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
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);
@@ -839,6 +841,19 @@ void Verifier::visitNamedMDNode(const NamedMDNode &NMD) {
}
}
+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.
@@ -848,6 +863,12 @@ void Verifier::visitMDNode(const MDNode &MD, AreDebugLocsAllowed AllowLocs) {
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");
diff --git a/llvm/test/Linker/debug-info-bad-enum.ll b/llvm/test/Linker/debug-info-bad-enum.ll
new file mode 100644
index 0000000000000..3abee7f916f78
--- /dev/null
+++ b/llvm/test/Linker/debug-info-bad-enum.ll
@@ -0,0 +1,47 @@
+; Check that ODR type uniquing does not create invalid debug metadata.
+; RUN: split-file %s %t
+; RUN: llvm-link -S -o - %t/a.ll %t/b.ll 2>&1 | FileCheck %s
+
+; CHECK-DAG: distinct !DICompileUnit({{.*}}, enums: ![[ENUMLIST:[0-9]+]],
+; CHECK-DAG: ![[ENUMLIST]] = !{![[ENUM:[0-9]+]]}
+; CHECK-DAG: ![[ENUM]] = distinct !DICompositeType(tag: DW_TAG_enumeration_type, {{.*}}, identifier: "_ZTS5Stage"
+
+; CHECK-DAG: distinct !DICompileUnit({{.*}}, retainedTypes: ![[RETAIN:[0-9]+]],
+; CHECK-DAG: ![[RETAIN]] = !{![[VAR:[0-9]+]]}
+; CHECK-DAG: ![[CLASS:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_class_type, {{.*}}, identifier: "_ZTS5Stage"
+; CHECK-DAG: ![[VAR]] = !DIDerivedType(tag: DW_TAG_member, name: "Var", scope: ![[CLASS]],
+
+; 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_class_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}
diff --git a/llvm/test/Verifier/dbg-invalid-enum-as-scope.ll b/llvm/test/Verifier/dbg-invalid-enum-as-scope.ll
new file mode 100644
index 0000000000000..4053d4aede2e2
--- /dev/null
+++ b/llvm/test/Verifier/dbg-invalid-enum-as-scope.ll
@@ -0,0 +1,16 @@
+; 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}
diff --git a/llvm/unittests/IR/DebugTypeODRUniquingTest.cpp b/llvm/unittests/IR/DebugTypeODRUniquingTest.cpp
index ad7587497e3ed..d25f0d3485a52 100644
--- a/llvm/unittests/IR/DebugTypeODRUniquingTest.cpp
+++ b/llvm/unittests/IR/DebugTypeODRUniquingTest.cpp
@@ -81,38 +81,38 @@ TEST(DebugTypeODRUniquingTest, buildODRType) {
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,
- 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::FlagZero, nullptr, 0,
+ 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) {
@@ -136,7 +136,6 @@ TEST(DebugTypeODRUniquingTest, buildODRTypeFields) {
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) \
@@ -155,10 +154,10 @@ TEST(DebugTypeODRUniquingTest, buildODRTypeFields) {
// 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());
More information about the llvm-commits
mailing list