From ccbf1f72a6bb103ca6fb701e713a93bbd657f7fc Mon Sep 17 00:00:00 2001 From: Ivan Krasin Date: Fri, 27 Jan 2017 15:54:49 +0000 Subject: [PATCH] Avoid using unspecified ordering in MetadataLoader::MetadataLoaderImpl::parseOneMetadata. Summary: MetadataLoader::MetadataLoaderImpl::parseOneMetadata uses the following construct in a number of places: ``` MetadataList.assignValue(<...>, NextMetadataNo++); ``` There, NextMetadataNo gets incremented, and since the order of arguments evaluation is not specified, that can happen before or after other arguments are evaluated. In a few cases the other arguments indirectly use NextMetadataNo. For instance, it's ``` MetadataList.assignValue( GET_OR_DISTINCT(DIModule, (Context, getMDOrNull(Record[1]), getMDString(Record[2]), getMDString(Record[3]), getMDString(Record[4]), getMDString(Record[5]))), NextMetadataNo++); ``` getMDOrNull calls getMD that uses NextMetadataNo: ``` MetadataList.getMetadataFwdRef(NextMetadataNo); ``` Therefore, the order of evaluation becomes important. That caused a very subtle LLD crash that only happens if compiled with GCC or if LLD is built with LTO. In the case if LLD is compiled with Clang and regular linking mode, everything worked as intended. This change extracts incrementing of NextMetadataNo outside of the arguments list to guarantee the correct order of evaluation. For the record, this has taken 3 days to track to the origin. It all started with a ThinLTO bot in Chrome not being able to link a target if debug info is enabled. Reviewers: pcc, mehdi_amini Reviewed By: mehdi_amini Subscribers: aprantl, llvm-commits Differential Revision: https://reviews.llvm.org/D29204 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@293291 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Bitcode/Reader/MetadataLoader.cpp | 99 ++++++++++++++++++--------- 1 file changed, 66 insertions(+), 33 deletions(-) diff --git a/lib/Bitcode/Reader/MetadataLoader.cpp b/lib/Bitcode/Reader/MetadataLoader.cpp index 1216da24996..ab9dd06d4f0 100644 --- a/lib/Bitcode/Reader/MetadataLoader.cpp +++ b/lib/Bitcode/Reader/MetadataLoader.cpp @@ -922,7 +922,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( // If this isn't a LocalAsMetadata record, we're dropping it. This used // to be legal, but there's no upgrade path. auto dropRecord = [&] { - MetadataList.assignValue(MDNode::get(Context, None), NextMetadataNo++); + MetadataList.assignValue(MDNode::get(Context, None), NextMetadataNo); + NextMetadataNo++; }; if (Record.size() != 2) { dropRecord(); @@ -937,7 +938,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue( LocalAsMetadata::get(ValueList.getValueFwdRef(Record[1], Ty)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_OLD_NODE: { @@ -962,7 +964,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( } else Elts.push_back(nullptr); } - MetadataList.assignValue(MDNode::get(Context, Elts), NextMetadataNo++); + MetadataList.assignValue(MDNode::get(Context, Elts), NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_VALUE: { @@ -975,7 +978,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue( ValueAsMetadata::get(ValueList.getValueFwdRef(Record[1], Ty)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_DISTINCT_NODE: @@ -988,7 +992,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( Elts.push_back(getMDOrNull(ID)); MetadataList.assignValue(IsDistinct ? MDNode::getDistinct(Context, Elts) : MDNode::get(Context, Elts), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_LOCATION: { @@ -1002,7 +1007,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( Metadata *InlinedAt = getMDOrNull(Record[4]); MetadataList.assignValue( GET_OR_DISTINCT(DILocation, (Context, Line, Column, Scope, InlinedAt)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_GENERIC_DEBUG: { @@ -1022,7 +1028,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( DwarfOps.push_back(getMDOrNull(Record[I])); MetadataList.assignValue( GET_OR_DISTINCT(GenericDINode, (Context, Tag, Header, DwarfOps)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_SUBRANGE: { @@ -1033,7 +1040,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue( GET_OR_DISTINCT(DISubrange, (Context, Record[1], unrotateSign(Record[2]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_ENUMERATOR: { @@ -1044,7 +1052,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue( GET_OR_DISTINCT(DIEnumerator, (Context, unrotateSign(Record[1]), getMDString(Record[2]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_BASIC_TYPE: { @@ -1056,7 +1065,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( GET_OR_DISTINCT(DIBasicType, (Context, Record[1], getMDString(Record[2]), Record[3], Record[4], Record[5])), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_DERIVED_TYPE: { @@ -1072,7 +1082,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( getDITypeRefOrNull(Record[5]), getDITypeRefOrNull(Record[6]), Record[7], Record[8], Record[9], Flags, getDITypeRefOrNull(Record[11]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_COMPOSITE_TYPE: { @@ -1137,7 +1148,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( if (!IsNotUsedInTypeRef && Identifier) MetadataList.addTypeRef(*Identifier, *cast(CT)); - MetadataList.assignValue(CT, NextMetadataNo++); + MetadataList.assignValue(CT, NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_SUBROUTINE_TYPE: { @@ -1154,7 +1166,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue( GET_OR_DISTINCT(DISubroutineType, (Context, Flags, CC, Types)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } @@ -1168,7 +1181,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( (Context, getMDOrNull(Record[1]), getMDString(Record[2]), getMDString(Record[3]), getMDString(Record[4]), getMDString(Record[5]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } @@ -1184,7 +1198,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( Record.size() == 3 ? DIFile::CSK_None : static_cast(Record[3]), Record.size() == 3 ? nullptr : getMDString(Record[4]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_COMPILE_UNIT: { @@ -1203,7 +1218,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( Record.size() <= 14 ? 0 : Record[14], Record.size() <= 16 ? true : Record[16]); - MetadataList.assignValue(CU, NextMetadataNo++); + MetadataList.assignValue(CU, NextMetadataNo); + NextMetadataNo++; // Move the Upgrade the list of subprograms. if (Metadata *SPs = getMDOrNullWithoutPlaceholders(Record[11])) @@ -1250,7 +1266,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( getMDOrNull(Record[16 + Offset]), // declaration getMDOrNull(Record[17 + Offset]) // variables )); - MetadataList.assignValue(SP, NextMetadataNo++); + MetadataList.assignValue(SP, NextMetadataNo); + NextMetadataNo++; // Upgrade sp->function mapping to function->sp mapping. if (HasFn) { @@ -1275,7 +1292,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( GET_OR_DISTINCT(DILexicalBlock, (Context, getMDOrNull(Record[1]), getMDOrNull(Record[2]), Record[3], Record[4])), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_LEXICAL_BLOCK_FILE: { @@ -1287,7 +1305,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( GET_OR_DISTINCT(DILexicalBlockFile, (Context, getMDOrNull(Record[1]), getMDOrNull(Record[2]), Record[3])), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_NAMESPACE: { @@ -1301,7 +1320,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( (Context, getMDOrNull(Record[1]), getMDOrNull(Record[2]), getMDString(Record[3]), Record[4], ExportSymbols)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_MACRO: { @@ -1313,7 +1333,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( GET_OR_DISTINCT(DIMacro, (Context, Record[1], Record[2], getMDString(Record[3]), getMDString(Record[4]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_MACRO_FILE: { @@ -1325,7 +1346,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( GET_OR_DISTINCT(DIMacroFile, (Context, Record[1], Record[2], getMDOrNull(Record[3]), getMDOrNull(Record[4]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_TEMPLATE_TYPE: { @@ -1336,7 +1358,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue(GET_OR_DISTINCT(DITemplateTypeParameter, (Context, getMDString(Record[1]), getDITypeRefOrNull(Record[2]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_TEMPLATE_VALUE: { @@ -1349,7 +1372,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( (Context, Record[1], getMDString(Record[2]), getDITypeRefOrNull(Record[3]), getMDOrNull(Record[4]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_GLOBAL_VAR: { @@ -1367,7 +1391,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( getMDOrNull(Record[4]), Record[5], getDITypeRefOrNull(Record[6]), Record[7], Record[8], getMDOrNull(Record[10]), Record[11])), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; } else if (Version == 0) { // Upgrade old metadata, which stored a global variable reference or a // ConstantInt here. @@ -1399,7 +1424,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( getMDOrNull(Record[10]), AlignInBits)); auto *DGVE = DIGlobalVariableExpression::getDistinct(Context, DGV, Expr); - MetadataList.assignValue(DGVE, NextMetadataNo++); + MetadataList.assignValue(DGVE, NextMetadataNo); + NextMetadataNo++; if (Attach) Attach->addDebugInfo(DGVE); } else @@ -1432,7 +1458,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( getMDOrNull(Record[3 + HasTag]), Record[4 + HasTag], getDITypeRefOrNull(Record[5 + HasTag]), Record[6 + HasTag], Flags, AlignInBits)), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_EXPRESSION: { @@ -1449,7 +1476,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue( GET_OR_DISTINCT(DIExpression, (Context, makeArrayRef(Record).slice(1))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_GLOBAL_VAR_EXPR: { @@ -1460,7 +1488,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( MetadataList.assignValue(GET_OR_DISTINCT(DIGlobalVariableExpression, (Context, getMDOrNull(Record[1]), getMDOrNull(Record[2]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_OBJC_PROPERTY: { @@ -1474,7 +1503,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( getMDOrNull(Record[2]), Record[3], getMDString(Record[4]), getMDString(Record[5]), Record[6], getDITypeRefOrNull(Record[7]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_IMPORTED_ENTITY: { @@ -1487,7 +1517,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( (Context, Record[1], getMDOrNull(Record[2]), getDITypeRefOrNull(Record[3]), Record[4], getMDString(Record[5]))), - NextMetadataNo++); + NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_STRING_OLD: { @@ -1497,13 +1528,15 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata( HasSeenOldLoopTags |= mayBeOldLoopAttachmentTag(String); ++NumMDStringLoaded; Metadata *MD = MDString::get(Context, String); - MetadataList.assignValue(MD, NextMetadataNo++); + MetadataList.assignValue(MD, NextMetadataNo); + NextMetadataNo++; break; } case bitc::METADATA_STRINGS: { auto CreateNextMDString = [&](StringRef Str) { ++NumMDStringLoaded; - MetadataList.assignValue(MDString::get(Context, Str), NextMetadataNo++); + MetadataList.assignValue(MDString::get(Context, Str), NextMetadataNo); + NextMetadataNo++; }; if (Error Err = parseMetadataStrings(Record, Blob, CreateNextMDString)) return Err; -- 2.50.1