[llvm] r293291 - Avoid using unspecified ordering in MetadataLoader::MetadataLoaderImpl::parseOneMetadata.
Ivan Krasin via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 27 07:54:49 PST 2017
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;
More information about the llvm-commits
mailing list