[llvm] r255909 - [ThinLTO] Metadata linking for imported functions

[Teresa Johnson via llvm-commits]

On Mon, Jan 4, 2016 at 1:22 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>>>>
>>>> Finally, there's an important missing assertion that I'm not sure how
>>>> to provide.  But it's important.
>>>>
>>>> IIUC, it's vital that this *doesn't* happen:
>>>>
>>>> 1. Temporary metadata T is put into MetadataToIDs.
>>>> 2. T.replaceAllUsesWith(MD) gets called.
>>>> 3. MetadataToIDs.count/find/etc.(T) or (MD).
>>>>
>>>> First case (T): since `T` has been freed, some new temporary (or normal)
>>>> metadata might get allocated in the same spot.  Someone could be
>>>> checking for this new `N`, and get the ID for the old `T`.
>>>>
>>>> Second case (MD): the map contains the ID for `MD`, but it's indexed
>>>> under the old pointer `T`.  Checking for `MD` will give the wrong answer.
>>>>
>>>> As I mentioned, I don't know how to add an assertion or verification for
>>>> this, but I think it's really important.  Please look into how to
>>>> verify that these cases can't occur.
>>>
>>> In the case where we are saving temporary metadata in the
>>> MetadataToIDs map, we should never replace it once we save it to that
>>> map, until after we delete that instance of the MetadataToIDs map when
>>> the IRLinker is destructed.
>>>
>>> So what I could do is when we save temp metadata to the map, set a
>>> flag on the temp MD's ReplaceableMetadataImpl, and assert in
>>> ReplaceableMetadataImpl::replaceAllUsesWith if the flag is set, then
>>> clear the flag when we destruct the IRLinker and its associated
>>> MetadataToIDs map. Does that seem reasonable? I'll give it a shot.
>>
>> This works. I also confirmed that I get an assert during metadata
>> linking if I skip resetting the flag during IRLinker destruction, so
>> it does catch a RAUW on temporary data with the flag set.
>>
>> Will commit these new asserts and fix the other issues you pointed out
>> during your review.
>
> Awesome, thanks.  You might be able to add a unit test using
> `EXPECT_DEATH()`.

Done in r256938.

Thanks,
Teresa



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413