[llvm] r293291 - Avoid using unspecified ordering in MetadataLoader::MetadataLoaderImpl::parseOneMetadata.

Ivan Krasin via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 27 08:15:36 PST 2017


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> 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> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/52078361/attachment-0001.html>


More information about the llvm-commits mailing list