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

[Hans Wennborg via llvm-commits]

Thanks! I see the merge landed in r293292

On Fri, Jan 27, 2017 at 8:16 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> Ok will do!
>
> CC: Hans FYI.
>
>
> On Jan 27, 2017, at 8:15 AM, Ivan Krasin <krasin at google.com> wrote:
>
> 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