[PATCH] D29204: Avoid using unspecified ordering in MetadataLoader::MetadataLoaderImpl::parseOneMetadata.

[Ivan Krasin via Phabricator via llvm-commits]

krasin created this revision.
Herald added a subscriber: aprantl.

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.


https://reviews.llvm.org/D29204

Files:
  lib/Bitcode/Reader/MetadataLoader.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D29204.85985.patch
Type: text/x-patch
Size: 11448 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/de9f7f07/attachment.bin>