[llvm] r293291 - Avoid using unspecified ordering in MetadataLoader::MetadataLoaderImpl::parseOneMetadata.
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 27 08:11:53 PST 2017
Are you cherry-picking this in clang-4.0 or do you want me to do?
(Has been approved by Peter on Phab)
—
Mehdi
> On Jan 27, 2017, at 7:54 AM, Ivan Krasin via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: krasin
> Date: Fri Jan 27 09:54:49 2017
> New Revision: 293291
>
> URL: http://llvm.org/viewvc/llvm-project?rev=293291&view=rev
> Log:
> 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
>
> Modified:
> llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp
>
> Modified: llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp?rev=293291&r1=293290&r2=293291&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp (original)
> +++ llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp Fri Jan 27 09:54:49 2017
> @@ -922,7 +922,8 @@ Error MetadataLoader::MetadataLoaderImpl
> // 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
>
> 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
> } 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
>
> 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
> 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
> 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
> 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
> 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
> 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
> 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
> 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
> if (!IsNotUsedInTypeRef && Identifier)
> MetadataList.addTypeRef(*Identifier, *cast<DICompositeType>(CT));
>
> - MetadataList.assignValue(CT, NextMetadataNo++);
> + MetadataList.assignValue(CT, NextMetadataNo);
> + NextMetadataNo++;
> break;
> }
> case bitc::METADATA_SUBROUTINE_TYPE: {
> @@ -1154,7 +1166,8 @@ Error MetadataLoader::MetadataLoaderImpl
>
> MetadataList.assignValue(
> GET_OR_DISTINCT(DISubroutineType, (Context, Flags, CC, Types)),
> - NextMetadataNo++);
> + NextMetadataNo);
> + NextMetadataNo++;
> break;
> }
>
> @@ -1168,7 +1181,8 @@ Error MetadataLoader::MetadataLoaderImpl
> (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
> Record.size() == 3 ? DIFile::CSK_None
> : static_cast<DIFile::ChecksumKind>(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
> 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
> 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
> 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
> 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
> (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
> 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
> 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
> 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
> (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
> 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
> 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
> 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
>
> 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
> 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
> 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
> (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
> 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;
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list