[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:16:34 PST 2017
Ok will do!
CC: Hans FYI.
> On Jan 27, 2017, at 8:15 AM, Ivan Krasin <krasin at google.com> wrote:
>
> Hi Mehdi,
>
> I have never done that (cherry picking into an LLVM release branch) before and probably won't have this need often. I would appreciate if you do it.
>
> Thank you!
>
> On Fri, Jan 27, 2017 at 8:11 AM, Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
> 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 <mailto: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 <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 <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 <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 <mailto:llvm-commits at lists.llvm.org>
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/18d3784a/attachment.html>
More information about the llvm-commits
mailing list