From da41af9e9423eeb435bbf64f94649726569ae45b Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Sat, 6 Dec 2014 01:26:49 +0000 Subject: [PATCH] IR: Disallow complicated function-local metadata Disallow complex types of function-local metadata. The only valid function-local metadata is an `MDNode` whose sole argument is a non-metadata function-local value. Part of PR21532. llvm-svn: 223564 --- llvm/lib/AsmParser/LLParser.cpp | 13 +++++ llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 36 ++++++++++--- llvm/lib/IR/Metadata.cpp | 75 ++++++++++----------------- llvm/test/Assembler/functionlocal-metadata.ll | 16 +++--- llvm/test/Feature/metadata.ll | 2 +- llvm/test/Transforms/GlobalOpt/metadata.ll | 5 +- 6 files changed, 81 insertions(+), 66 deletions(-) diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp index 3e56c8d..93eb490 100644 --- a/llvm/lib/AsmParser/LLParser.cpp +++ b/llvm/lib/AsmParser/LLParser.cpp @@ -4668,7 +4668,11 @@ bool LLParser::ParseMDNodeVector(SmallVectorImpl &Elts, if (Lex.getKind() == lltok::rbrace) return false; + bool IsLocal = false; do { + if (IsLocal) + return TokError("unexpected operand after function-local metadata"); + // Null is a special case since it is typeless. if (EatIfPresent(lltok::kw_null)) { Elts.push_back(nullptr); @@ -4678,6 +4682,15 @@ bool LLParser::ParseMDNodeVector(SmallVectorImpl &Elts, Value *V = nullptr; if (ParseTypeAndValue(V, PFS)) return true; Elts.push_back(V); + + if (isa(V) && cast(V)->isFunctionLocal()) + return TokError("unexpected nested function-local metadata"); + if (!V->getType()->isMetadataTy() && !isa(V)) { + assert(PFS && "Unexpected function-local metadata without PFS"); + if (Elts.size() > 1) + return TokError("unexpected function-local metadata"); + IsLocal = true; + } } while (EatIfPresent(lltok::comma)); return false; diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp index 25efa60..9f6d02f 100644 --- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp +++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp @@ -1072,7 +1072,6 @@ std::error_code BitcodeReader::ParseMetadata() { break; } - bool IsFunctionLocal = false; // Read a record. Record.clear(); unsigned Code = Stream.readRecord(Entry.ID, Record); @@ -1100,9 +1099,34 @@ std::error_code BitcodeReader::ParseMetadata() { } break; } - case bitc::METADATA_FN_NODE: - IsFunctionLocal = true; - // fall-through + case bitc::METADATA_FN_NODE: { + // This is a function-local node. + if (Record.size() % 2 == 1) + return Error(BitcodeError::InvalidRecord); + + // If this isn't a single-operand node that directly references + // non-metadata, we're dropping it. This used to be legal, but there's + // no upgrade path. + auto dropRecord = [&] { + MDValueList.AssignValue(MDNode::get(Context, None), NextMDValueNo++); + }; + if (Record.size() != 2) { + dropRecord(); + break; + } + + Type *Ty = getTypeByID(Record[0]); + if (Ty->isMetadataTy() || Ty->isVoidTy()) { + dropRecord(); + break; + } + + Value *Elts[] = {ValueList.getValueFwdRef(Record[1], Ty)}; + Value *V = MDNode::getWhenValsUnresolved(Context, Elts, + /*IsFunctionLocal*/ true); + MDValueList.AssignValue(V, NextMDValueNo++); + break; + } case bitc::METADATA_NODE: { if (Record.size() % 2 == 1) return Error(BitcodeError::InvalidRecord); @@ -1120,8 +1144,8 @@ std::error_code BitcodeReader::ParseMetadata() { else Elts.push_back(nullptr); } - Value *V = MDNode::getWhenValsUnresolved(Context, Elts, IsFunctionLocal); - IsFunctionLocal = false; + Value *V = MDNode::getWhenValsUnresolved(Context, Elts, + /*IsFunctionLocal*/ false); MDValueList.AssignValue(V, NextMDValueNo++); break; } diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp index 3a7c293..45d61a1 100644 --- a/llvm/lib/IR/Metadata.cpp +++ b/llvm/lib/IR/Metadata.cpp @@ -185,43 +185,20 @@ static const Function *getFunctionForValue(Value *V) { return nullptr; } -#ifndef NDEBUG -static const Function *assertLocalFunction(const MDNode *N) { - if (!N->isFunctionLocal()) return nullptr; - - // FIXME: This does not handle cyclic function local metadata. - const Function *F = nullptr, *NewF = nullptr; - for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i) { - if (Value *V = N->getOperand(i)) { - if (MDNode *MD = dyn_cast(V)) - NewF = assertLocalFunction(MD); - else - NewF = getFunctionForValue(V); - } - if (!F) - F = NewF; - else - assert((NewF == nullptr || F == NewF) && - "inconsistent function-local metadata"); - } - return F; -} -#endif - // getFunction - If this metadata is function-local and recursively has a // function-local operand, return the first such operand's parent function. // Otherwise, return null. getFunction() should not be used for performance- // critical code because it recursively visits all the MDNode's operands. const Function *MDNode::getFunction() const { -#ifndef NDEBUG - return assertLocalFunction(this); -#else - if (!isFunctionLocal()) return nullptr; - for (unsigned i = 0, e = getNumOperands(); i != e; ++i) - if (const Function *F = getFunctionForValue(getOperand(i))) - return F; - return nullptr; -#endif + if (!isFunctionLocal()) + return nullptr; + assert(getNumOperands() == 1 && + "Expected one operand for function-local metadata"); + assert(getOperand(0) && + "Expected non-null operand for function-local metadata"); + assert(!getOperand(0)->getType()->isMetadataTy() && + "Expected non-metadata as operand of function-local metadata"); + return getFunctionForValue(getOperand(0)); } /// \brief Check if the Value would require a function-local MDNode. @@ -260,6 +237,14 @@ MDNode *MDNode::getMDNode(LLVMContext &Context, ArrayRef Vals, break; } + if (isFunctionLocal) { + assert(Vals.size() == 1 && + "Expected exactly one operand for function-local metadata"); + assert(Vals[0] && "Expected non-null operand for function-local metadata"); + assert(!Vals[0]->getType()->isMetadataTy() && + "Expected non-metadata as operand of function-local metadata"); + } + // Coallocate space for the node and Operands together, then placement new. GenericMDNode *N = new (Vals.size()) GenericMDNode(Context, Vals, isFunctionLocal); @@ -323,6 +308,8 @@ void MDNode::replaceOperand(MDNodeOperand *Op, Value *To) { // Handle this case by implicitly dropping the MDNode reference to null. // Likewise if the MDNode is function-local but for a different function. if (To && isFunctionLocalValue(To)) { + assert(!To->getType()->isMetadataTy() && + "Expected non-metadata as operand of function-local metadata"); if (!isFunctionLocal()) To = nullptr; else { @@ -338,6 +325,14 @@ void MDNode::replaceOperand(MDNodeOperand *Op, Value *To) { if (From == To) return; + // If this MDValue was previously function-local but no longer is, clear + // its function-local flag. + if (isFunctionLocal() && !(To && isFunctionLocalValue(To))) { + assert(getNumOperands() == 1 && + "Expected function-local metadata to have exactly one operand"); + setValueSubclassData(getSubclassDataFromValue() & ~FunctionLocalBit); + } + // If this node is already not being uniqued (because one of the operands // already went to null), then there is nothing else to do here. if (isNotUniqued()) { @@ -377,22 +372,6 @@ void MDNode::replaceOperand(MDNodeOperand *Op, Value *To) { N->Hash = Key.Hash; Store.insert(N); - - // If this MDValue was previously function-local but no longer is, clear - // its function-local flag. - if (isFunctionLocal() && !isFunctionLocalValue(To)) { - bool isStillFunctionLocal = false; - for (unsigned i = 0, e = getNumOperands(); i != e; ++i) { - Value *V = getOperand(i); - if (!V) continue; - if (isFunctionLocalValue(V)) { - isStillFunctionLocal = true; - break; - } - } - if (!isStillFunctionLocal) - setValueSubclassData(getSubclassDataFromValue() & ~FunctionLocalBit); - } } MDNode *MDNode::concatenate(MDNode *A, MDNode *B) { diff --git a/llvm/test/Assembler/functionlocal-metadata.ll b/llvm/test/Assembler/functionlocal-metadata.ll index c46233a..ce0d770 100644 --- a/llvm/test/Assembler/functionlocal-metadata.ll +++ b/llvm/test/Assembler/functionlocal-metadata.ll @@ -13,19 +13,17 @@ entry: ; CHECK: call void @llvm.dbg.declare(metadata !{i32* %1}, metadata !{i32* %1}, metadata {{.*}}) call void @llvm.dbg.declare(metadata !{i32 %two}, metadata !{i32 %0}, metadata !{metadata !"0x102"}) ; CHECK: call void @llvm.dbg.declare(metadata !{i32 %two}, metadata !{i32 %0}, metadata {{.*}}) - call void @llvm.dbg.declare(metadata !{i32 %0}, metadata !{i32* %1, i32 %0}, metadata !{metadata !"0x102"}) -; CHECK: call void @llvm.dbg.declare(metadata !{i32 %0}, metadata !{i32* %1, i32 %0}, metadata {{.*}}) - call void @llvm.dbg.declare(metadata !{i32* %1}, metadata !{i32 %b, i32 %0}, metadata !{metadata !"0x102"}) -; CHECK: call void @llvm.dbg.declare(metadata !{i32* %1}, metadata !{i32 %b, i32 %0}, metadata {{.*}}) - call void @llvm.dbg.declare(metadata !{i32 %a}, metadata !{i32 %a, metadata !"foo"}, metadata !{metadata !"0x102"}) -; CHECK: call void @llvm.dbg.declare(metadata !{i32 %a}, metadata !{i32 %a, metadata !"foo"}, metadata {{.*}}) - call void @llvm.dbg.declare(metadata !{i32 %b}, metadata !{metadata !0, i32 %two}, metadata !{metadata !"0x102"}) -; CHECK: call void @llvm.dbg.declare(metadata !{i32 %b}, metadata !{metadata ![[ID0:[0-9]+]], i32 %two}, metadata {{.*}}) + call void @llvm.dbg.declare(metadata !{i32* %1}, metadata !{i32 %b}, metadata !{metadata !"0x102"}) +; CHECK: call void @llvm.dbg.declare(metadata !{i32* %1}, metadata !{i32 %b}, metadata {{.*}}) + call void @llvm.dbg.declare(metadata !{i32 %a}, metadata !{i32 %a}, metadata !{metadata !"0x102"}) +; CHECK: call void @llvm.dbg.declare(metadata !{i32 %a}, metadata !{i32 %a}, metadata {{.*}}) + call void @llvm.dbg.declare(metadata !{i32 %b}, metadata !{i32 %two}, metadata !{metadata !"0x102"}) +; CHECK: call void @llvm.dbg.declare(metadata !{i32 %b}, metadata !{i32 %two}, metadata {{.*}}) call void @llvm.dbg.value(metadata !{ i32 %a }, i64 0, metadata !1, metadata !{metadata !"0x102"}) ; CHECK: call void @llvm.dbg.value(metadata !{i32 %a}, i64 0, metadata ![[ID1:[0-9]+]], metadata {{.*}}) call void @llvm.dbg.value(metadata !{ i32 %0 }, i64 25, metadata !0, metadata !{metadata !"0x102"}) -; CHECK: call void @llvm.dbg.value(metadata !{i32 %0}, i64 25, metadata ![[ID0]], metadata {{.*}}) +; CHECK: call void @llvm.dbg.value(metadata !{i32 %0}, i64 25, metadata ![[ID0:[0-9]+]], metadata {{.*}}) call void @llvm.dbg.value(metadata !{ i32* %1 }, i64 16, metadata !3, metadata !{metadata !"0x102"}) ; CHECK: call void @llvm.dbg.value(metadata !{i32* %1}, i64 16, metadata ![[ID3:[0-9]+]], metadata {{.*}}) call void @llvm.dbg.value(metadata !3, i64 12, metadata !2, metadata !{metadata !"0x102"}) diff --git a/llvm/test/Feature/metadata.ll b/llvm/test/Feature/metadata.ll index 9856b37..2a857bb 100644 --- a/llvm/test/Feature/metadata.ll +++ b/llvm/test/Feature/metadata.ll @@ -4,7 +4,7 @@ define void @foo(i32 %x) { call void @llvm.zonk(metadata !1, i64 0, metadata !1) store i32 0, i32* null, !whatever !0, !whatever_else !{}, !more !{metadata !"hello"} - store i32 0, i32* null, !whatever !{i32 %x, metadata !"hello", metadata !1, metadata !{}, metadata !2} + store i32 0, i32* null, !whatever !{metadata !"hello", metadata !1, metadata !{}, metadata !2} ret void, !whatever !{i32 %x} } diff --git a/llvm/test/Transforms/GlobalOpt/metadata.ll b/llvm/test/Transforms/GlobalOpt/metadata.ll index ecf3f94..cfa2926 100644 --- a/llvm/test/Transforms/GlobalOpt/metadata.ll +++ b/llvm/test/Transforms/GlobalOpt/metadata.ll @@ -13,14 +13,15 @@ define i32 @main(i32 %argc, i8** %argv) { } define void @foo(i32 %x) { - call void @llvm.foo(metadata !{i8*** @G, i32 %x}) -; CHECK: call void @llvm.foo(metadata !{null, i32 %x}) + call void @llvm.foo(metadata !{i8*** @G}) +; CHECK: call void @llvm.foo(metadata !0) ret void } declare void @llvm.foo(metadata) nounwind readnone !named = !{!0} +; CHECK: !named = !{!0} !0 = metadata !{i8*** @G} ; CHECK: !0 = metadata !{null} -- 2.7.4