<div dir="ltr">Hi Mehdi,<div><br></div><div>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.</div><div><br></div><div>Thank you!</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 27, 2017 at 8:11 AM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Are you cherry-picking this in clang-4.0 or do you want me to do?<br>
<br>
(Has been approved by Peter on Phab)<br>
<br>
—<br>
Mehdi<br>
<br>
> On Jan 27, 2017, at 7:54 AM, Ivan Krasin via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: krasin<br>
> Date: Fri Jan 27 09:54:49 2017<br>
> New Revision: 293291<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=293291&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=293291&view=rev</a><br>
> Log:<br>
> Avoid using unspecified ordering in MetadataLoader::<wbr>MetadataLoaderImpl::<wbr>parseOneMetadata.<br>
><br>
> Summary:<br>
> MetadataLoader::<wbr>MetadataLoaderImpl::<wbr>parseOneMetadata uses<br>
> the following construct in a number of places:<br>
><br>
> ```<br>
> MetadataList.assignValue(<...><wbr>, NextMetadataNo++);<br>
> ```<br>
><br>
> There, NextMetadataNo gets incremented, and since the order<br>
> of arguments evaluation is not specified, that can happen<br>
> before or after other arguments are evaluated.<br>
><br>
> In a few cases the other arguments indirectly use NextMetadataNo.<br>
> For instance, it's<br>
><br>
> ```<br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(DIModule,<br>
> (Context, getMDOrNull(Record[1]),<br>
> getMDString(Record[2]), getMDString(Record[3]),<br>
> getMDString(Record[4]), getMDString(Record[5]))),<br>
> NextMetadataNo++);<br>
> ```<br>
><br>
> getMDOrNull calls getMD that uses NextMetadataNo:<br>
><br>
> ```<br>
> MetadataList.<wbr>getMetadataFwdRef(<wbr>NextMetadataNo);<br>
> ```<br>
><br>
> Therefore, the order of evaluation becomes important. That caused<br>
> a very subtle LLD crash that only happens if compiled with GCC or<br>
> if LLD is built with LTO. In the case if LLD is compiled with Clang<br>
> and regular linking mode, everything worked as intended.<br>
><br>
> This change extracts incrementing of NextMetadataNo outside of<br>
> the arguments list to guarantee the correct order of evaluation.<br>
><br>
> For the record, this has taken 3 days to track to the origin. It all<br>
> started with a ThinLTO bot in Chrome not being able to link a target<br>
> if debug info is enabled.<br>
><br>
> Reviewers: pcc, mehdi_amini<br>
><br>
> Reviewed By: mehdi_amini<br>
><br>
> Subscribers: aprantl, llvm-commits<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D29204" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29204</a><br>
><br>
> Modified:<br>
> llvm/trunk/lib/Bitcode/Reader/<wbr>MetadataLoader.cpp<br>
><br>
> Modified: llvm/trunk/lib/Bitcode/Reader/<wbr>MetadataLoader.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp?rev=293291&r1=293290&r2=293291&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Bitcode/Reader/MetadataLoader.<wbr>cpp?rev=293291&r1=293290&r2=<wbr>293291&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Bitcode/Reader/<wbr>MetadataLoader.cpp (original)<br>
> +++ llvm/trunk/lib/Bitcode/Reader/<wbr>MetadataLoader.cpp Fri Jan 27 09:54:49 2017<br>
> @@ -922,7 +922,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> // If this isn't a LocalAsMetadata record, we're dropping it. This used<br>
> // to be legal, but there's no upgrade path.<br>
> auto dropRecord = [&] {<br>
> - MetadataList.assignValue(<wbr>MDNode::get(Context, None), NextMetadataNo++);<br>
> + MetadataList.assignValue(<wbr>MDNode::get(Context, None), NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> };<br>
> if (Record.size() != 2) {<br>
> dropRecord();<br>
> @@ -937,7 +938,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
><br>
> MetadataList.assignValue(<br>
> LocalAsMetadata::get(<wbr>ValueList.getValueFwdRef(<wbr>Record[1], Ty)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_OLD_NODE: {<br>
> @@ -962,7 +964,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> } else<br>
> Elts.push_back(nullptr);<br>
> }<br>
> - MetadataList.assignValue(<wbr>MDNode::get(Context, Elts), NextMetadataNo++);<br>
> + MetadataList.assignValue(<wbr>MDNode::get(Context, Elts), NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_VALUE: {<br>
> @@ -975,7 +978,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
><br>
> MetadataList.assignValue(<br>
> ValueAsMetadata::get(<wbr>ValueList.getValueFwdRef(<wbr>Record[1], Ty)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_DISTINCT_NODE:<br>
> @@ -988,7 +992,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> Elts.push_back(getMDOrNull(ID)<wbr>);<br>
> MetadataList.assignValue(<wbr>IsDistinct ? MDNode::getDistinct(Context, Elts)<br>
> : MDNode::get(Context, Elts),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_LOCATION: {<br>
> @@ -1002,7 +1007,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> Metadata *InlinedAt = getMDOrNull(Record[4]);<br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(DILocation, (Context, Line, Column, Scope, InlinedAt)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_GENERIC_DEBUG: {<br>
> @@ -1022,7 +1028,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> DwarfOps.push_back(<wbr>getMDOrNull(Record[I]));<br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(GenericDINode, (Context, Tag, Header, DwarfOps)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_SUBRANGE: {<br>
> @@ -1033,7 +1040,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(DISubrange,<br>
> (Context, Record[1], unrotateSign(Record[2]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_ENUMERATOR: {<br>
> @@ -1044,7 +1052,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(DIEnumerator, (Context, unrotateSign(Record[1]),<br>
> getMDString(Record[2]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_BASIC_TYPE: {<br>
> @@ -1056,7 +1065,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> GET_OR_DISTINCT(DIBasicType,<br>
> (Context, Record[1], getMDString(Record[2]), Record[3],<br>
> Record[4], Record[5])),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_DERIVED_TYPE: {<br>
> @@ -1072,7 +1082,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> getDITypeRefOrNull(Record[5]),<br>
> getDITypeRefOrNull(Record[6]), Record[7], Record[8],<br>
> Record[9], Flags, getDITypeRefOrNull(Record[11])<wbr>)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_COMPOSITE_TYPE: {<br>
> @@ -1137,7 +1148,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> if (!IsNotUsedInTypeRef && Identifier)<br>
> MetadataList.addTypeRef(*<wbr>Identifier, *cast<DICompositeType>(CT));<br>
><br>
> - MetadataList.assignValue(CT, NextMetadataNo++);<br>
> + MetadataList.assignValue(CT, NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_SUBROUTINE_<wbr>TYPE: {<br>
> @@ -1154,7 +1166,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
><br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(<wbr>DISubroutineType, (Context, Flags, CC, Types)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
><br>
> @@ -1168,7 +1181,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> (Context, getMDOrNull(Record[1]),<br>
> getMDString(Record[2]), getMDString(Record[3]),<br>
> getMDString(Record[4]), getMDString(Record[5]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
><br>
> @@ -1184,7 +1198,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> Record.size() == 3 ? DIFile::CSK_None<br>
> : static_cast<DIFile::<wbr>ChecksumKind>(Record[3]),<br>
> Record.size() == 3 ? nullptr : getMDString(Record[4]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_COMPILE_UNIT: {<br>
> @@ -1203,7 +1218,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> Record.size() <= 14 ? 0 : Record[14],<br>
> Record.size() <= 16 ? true : Record[16]);<br>
><br>
> - MetadataList.assignValue(CU, NextMetadataNo++);<br>
> + MetadataList.assignValue(CU, NextMetadataNo);<br>
> + NextMetadataNo++;<br>
><br>
> // Move the Upgrade the list of subprograms.<br>
> if (Metadata *SPs = getMDOrNullWithoutPlaceholders<wbr>(Record[11]))<br>
> @@ -1250,7 +1266,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> getMDOrNull(Record[16 + Offset]), // declaration<br>
> getMDOrNull(Record[17 + Offset]) // variables<br>
> ));<br>
> - MetadataList.assignValue(SP, NextMetadataNo++);<br>
> + MetadataList.assignValue(SP, NextMetadataNo);<br>
> + NextMetadataNo++;<br>
><br>
> // Upgrade sp->function mapping to function->sp mapping.<br>
> if (HasFn) {<br>
> @@ -1275,7 +1292,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> GET_OR_DISTINCT(<wbr>DILexicalBlock,<br>
> (Context, getMDOrNull(Record[1]),<br>
> getMDOrNull(Record[2]), Record[3], Record[4])),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_LEXICAL_BLOCK_<wbr>FILE: {<br>
> @@ -1287,7 +1305,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> GET_OR_DISTINCT(<wbr>DILexicalBlockFile,<br>
> (Context, getMDOrNull(Record[1]),<br>
> getMDOrNull(Record[2]), Record[3])),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_NAMESPACE: {<br>
> @@ -1301,7 +1320,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> (Context, getMDOrNull(Record[1]),<br>
> getMDOrNull(Record[2]), getMDString(Record[3]),<br>
> Record[4], ExportSymbols)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_MACRO: {<br>
> @@ -1313,7 +1333,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> GET_OR_DISTINCT(DIMacro,<br>
> (Context, Record[1], Record[2], getMDString(Record[3]),<br>
> getMDString(Record[4]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_MACRO_FILE: {<br>
> @@ -1325,7 +1346,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> GET_OR_DISTINCT(DIMacroFile,<br>
> (Context, Record[1], Record[2], getMDOrNull(Record[3]),<br>
> getMDOrNull(Record[4]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_TEMPLATE_TYPE: {<br>
> @@ -1336,7 +1358,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> MetadataList.assignValue(GET_<wbr>OR_DISTINCT(<wbr>DITemplateTypeParameter,<br>
> (Context, getMDString(Record[1]),<br>
> getDITypeRefOrNull(Record[2]))<wbr>),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_TEMPLATE_VALUE: {<br>
> @@ -1349,7 +1372,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> (Context, Record[1], getMDString(Record[2]),<br>
> getDITypeRefOrNull(Record[3]),<br>
> getMDOrNull(Record[4]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_GLOBAL_VAR: {<br>
> @@ -1367,7 +1391,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> getMDOrNull(Record[4]), Record[5],<br>
> getDITypeRefOrNull(Record[6]), Record[7], Record[8],<br>
> getMDOrNull(Record[10]), Record[11])),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> } else if (Version == 0) {<br>
> // Upgrade old metadata, which stored a global variable reference or a<br>
> // ConstantInt here.<br>
> @@ -1399,7 +1424,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> getMDOrNull(Record[10]), AlignInBits));<br>
><br>
> auto *DGVE = DIGlobalVariableExpression::<wbr>getDistinct(Context, DGV, Expr);<br>
> - MetadataList.assignValue(DGVE, NextMetadataNo++);<br>
> + MetadataList.assignValue(DGVE, NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> if (Attach)<br>
> Attach->addDebugInfo(DGVE);<br>
> } else<br>
> @@ -1432,7 +1458,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> getMDOrNull(Record[3 + HasTag]), Record[4 + HasTag],<br>
> getDITypeRefOrNull(Record[5 + HasTag]),<br>
> Record[6 + HasTag], Flags, AlignInBits)),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_EXPRESSION: {<br>
> @@ -1449,7 +1476,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
><br>
> MetadataList.assignValue(<br>
> GET_OR_DISTINCT(DIExpression, (Context, makeArrayRef(Record).slice(1))<wbr>),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_GLOBAL_VAR_<wbr>EXPR: {<br>
> @@ -1460,7 +1488,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> MetadataList.assignValue(GET_<wbr>OR_DISTINCT(<wbr>DIGlobalVariableExpression,<br>
> (Context, getMDOrNull(Record[1]),<br>
> getMDOrNull(Record[2]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_OBJC_PROPERTY: {<br>
> @@ -1474,7 +1503,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> getMDOrNull(Record[2]), Record[3],<br>
> getMDString(Record[4]), getMDString(Record[5]),<br>
> Record[6], getDITypeRefOrNull(Record[7]))<wbr>),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_IMPORTED_<wbr>ENTITY: {<br>
> @@ -1487,7 +1517,8 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> (Context, Record[1], getMDOrNull(Record[2]),<br>
> getDITypeRefOrNull(Record[3]), Record[4],<br>
> getMDString(Record[5]))),<br>
> - NextMetadataNo++);<br>
> + NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_STRING_OLD: {<br>
> @@ -1497,13 +1528,15 @@ Error MetadataLoader::<wbr>MetadataLoaderImpl<br>
> HasSeenOldLoopTags |= mayBeOldLoopAttachmentTag(<wbr>String);<br>
> ++NumMDStringLoaded;<br>
> Metadata *MD = MDString::get(Context, String);<br>
> - MetadataList.assignValue(MD, NextMetadataNo++);<br>
> + MetadataList.assignValue(MD, NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> break;<br>
> }<br>
> case bitc::METADATA_STRINGS: {<br>
> auto CreateNextMDString = [&](StringRef Str) {<br>
> ++NumMDStringLoaded;<br>
> - MetadataList.assignValue(<wbr>MDString::get(Context, Str), NextMetadataNo++);<br>
> + MetadataList.assignValue(<wbr>MDString::get(Context, Str), NextMetadataNo);<br>
> + NextMetadataNo++;<br>
> };<br>
> if (Error Err = parseMetadataStrings(Record, Blob, CreateNextMDString))<br>
> return Err;<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div><br></div>