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
llvm-svn: 293291
diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index 1216da2..ab9dd06 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -922,7 +922,8 @@
// 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 @@
MetadataList.assignValue(
LocalAsMetadata::get(ValueList.getValueFwdRef(Record[1], Ty)),
- NextMetadataNo++);
+ NextMetadataNo);
+ NextMetadataNo++;
break;
}
case bitc::METADATA_OLD_NODE: {
@@ -962,7 +964,8 @@
} 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 @@
MetadataList.assignValue(
ValueAsMetadata::get(ValueList.getValueFwdRef(Record[1], Ty)),
- NextMetadataNo++);
+ NextMetadataNo);
+ NextMetadataNo++;
break;
}
case bitc::METADATA_DISTINCT_NODE:
@@ -988,7 +992,8 @@
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 @@
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 @@
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 @@
MetadataList.assignValue(
GET_OR_DISTINCT(DISubrange,
(Context, Record[1], unrotateSign(Record[2]))),
- NextMetadataNo++);
+ NextMetadataNo);
+ NextMetadataNo++;
break;
}
case bitc::METADATA_ENUMERATOR: {
@@ -1044,7 +1052,8 @@
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 @@
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 @@
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 @@
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 @@
MetadataList.assignValue(
GET_OR_DISTINCT(DISubroutineType, (Context, Flags, CC, Types)),
- NextMetadataNo++);
+ NextMetadataNo);
+ NextMetadataNo++;
break;
}
@@ -1168,7 +1181,8 @@
(Context, getMDOrNull(Record[1]),
getMDString(Record[2]), getMDString(Record[3]),
getMDString(Record[4]), getMDString(Record[5]))),
- NextMetadataNo++);
+ NextMetadataNo);
+ NextMetadataNo++;
break;
}
@@ -1184,7 +1198,8 @@
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 @@
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 @@
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 @@
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 @@
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 @@
(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 @@
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 @@
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 @@
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 @@
(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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
(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 @@
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;