[PATCH] D49362: [ThinLTO] Internalize read only globals

Fri Nov 16 10:01:10 PST 2018

On Thu, Nov 15, 2018 at 7:21 AM Steven Wu <stevenwu at apple.com> wrote:

> Thanks Teresa and Evgeny! That is why I was confusing yesterday as well
> because I cant see how the hash can be different.
>
> Anyway, I think it would be good to refactor and call common code for the
>> cache key computation, but this can be done as a follow on. Steven - is
>> this something you would have time to do? I can do it, but can't test with
>> ld64. If you want I can create a patch and you can test it there.
>>
>
> I think that is a good idea as well. I don’t think I have time to do that
> until next year but I am happy to review and test patches. It will be fine
> just do that as a follow on. At a meantime, a spot fix to unblock this
> commit is fine with me.
>

D54635

Teresa

>
> Steven
>
>
> On Nov 15, 2018, at 6:55 AM, Teresa Johnson <tejohnson at google.com> wrote:
>
>
>
> On Thu, Nov 15, 2018 at 6:46 AM Evgeny Leviant <eleviant at accesssoftek.com>
> wrote:
>
>> Teresa, does it make sense to fix the issue and recommit or it's better
>> to wait for your patch?
>>
>
> Go ahead and just fix in the old LTO API cache key computation so you can
> recommit.
> Thanks
>
>
>> ------------------------------
>> *От:* Teresa Johnson <tejohnson at google.com>
>> *Отправлено:* 15 ноября 2018 г. 17:44
>> *Кому:* Evgeny Leviant
>> *Копия:* Steven Wu; Mehdi AMINI;
>> reviews+D49362+public+6f0316d7b1456727 at reviews.llvm.org; Xin Tong; Alex
>> L; Daniel Grumberg; jfbastien at apple.com; Anton Korobeynikov; Bob
>> Haarman; Easwaran Raman; Duncan P. N. Exon Smith; llvm-commits; George
>> Rimar; Igor Kudrin; jun.l at samsung.com
>> *Тема:* Re: [PATCH] D49362: [ThinLTO] Internalize read only globals
>>
>>
>>
>> On Thu, Nov 15, 2018 at 6:35 AM Teresa Johnson <tejohnson at google.com>
>> wrote:
>>
>>>
>>>
>>> On Thu, Nov 15, 2018 at 6:22 AM Evgeny Leviant <
>>> eleviant at accesssoftek.com> wrote:
>>>
>>>> Thanks for making reproducer, Steven!
>>>>
>>>>
>>>> It turned out cache key is computed differently by newer API (LTO.cpp)
>>>> and legacy API (ThinLTOCodeGenerator).
>>>>
>>> Arg - such a simple reason! I didn't remember that they were using
>>> separate code. Looking at what appears to be the cache key computation for
>>> the old API (ModuleCacheEntry constructor), I don't see it hashing other
>>> things we compute during the thin link beyond import/exports/resolved
>>> linkage types. I.e. I don't see it hashing the isLive bit. So I'm surprised
>>> we haven't hit an issue here before. Wonder if this code and
>>> computeCacheKey can be refactored to do the same computation for most of
>>> the info?? Maybe as a follow on patch. But I'm really curious how things
>>> are working given e.g. the missing hash of the live bit etc.
>>>
>>
>> Answering my own question: The isLive bit is only used downstream of the
>> thin link in the CFI/WPD case, which aren't supported via the old LTO API.
>> Ditto for the DSOLocal bit, which is only set in the new LTO API.
>>
>> Anyway, I think it would be good to refactor and call common code for the
>> cache key computation, but this can be done as a follow on. Steven - is
>> this something you would have time to do? I can do it, but can't test with
>> ld64. If you want I can create a patch and you can test it there.
>>
>> Teresa
>>
>>
>>>
>>> I was always wondering what piece of software uses legacy API and
>>>> finally found this out - it is Mac OS X linker ld64.
>>>>
>>>
>>> Yep, along with at least one proprietary linker that I am aware of.
>>>
>>> Teresa
>>>
>>>>
>>>> On Linux lld and gold plugin use newer API, so the bug can't be
>>>> reproduced.
>>>> ------------------------------
>>>> *От:* stevenwu at apple.com <stevenwu at apple.com> от имени Steven Wu <
>>>> stevenwu at apple.com>
>>>> *Отправлено:* 15 ноября 2018 г. 3:34
>>>> *Кому:* Teresa Johnson
>>>> *Копия:* Evgeny Leviant; Mehdi AMINI;
>>>> reviews+D49362+public+6f0316d7b1456727 at reviews.llvm.org; Xin Tong;
>>>> Alex L; Daniel Grumberg; JF Bastien; Anton Korobeynikov; Bob Haarman;
>>>> Easwaran Raman; Duncan Exon Smith; llvm-commits; George Rimar; Igor Kudrin;
>>>> jun.l at samsung.com
>>>> *Тема:* Re: [PATCH] D49362: [ThinLTO] Internalize read only globals
>>>>
>>>> CAUTION: This email originated from outside of the organization. Do
>>>> not click links or open attachments unless you recognize the sender and
>>>> know the content is safe.
>>>>
>>>>
>>>> On Nov 14, 2018, at 4:28 PM, Teresa Johnson <tejohnson at google.com>
>>>> wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Nov 14, 2018 at 1:47 PM Steven Wu <stevenwu at apple.com> wrote:
>>>>
>>>>> Here is a small reduced test case using the same main.c and foo.c from
>>>>> the review:
>>>>> cat main.c
>>>>>
>>>>> int foo();
>>>>> int main() {
>>>>>   return foo();
>>>>> }
>>>>>
>>>>>
>>>>> cat foo.c
>>>>>
>>>>> #include <stdlib.h>
>>>>> static int gFoo = 1;
>>>>> int foo() {
>>>>>   return gFoo;
>>>>> }
>>>>> void bar() {
>>>>>   gFoo = rand();
>>>>> }
>>>>>
>>>>>
>>>>> cat test1.c
>>>>>
>>>>> int foo();
>>>>> int test() {
>>>>>   return foo();
>>>>> }
>>>>>
>>>>>
>>>>> cat test2.c
>>>>>
>>>>> int foo();
>>>>> int bar();
>>>>> int test() {
>>>>>   return foo() + bar();
>>>>> }
>>>>>
>>>>>
>>>>> To reproduce:
>>>>> $ clang -O3 -flto=thin -c main.c test1.c test2.c foo.c
>>>>> $ clang -O3 -flto=thin -Wl,-cache_path_lto,lto.cache main.o test2.o
>>>>> foo.o
>>>>> $ clang -O3 -flto=thin -Wl,-cache_path_lto,lto.cache main.o test1.o
>>>>> foo.o
>>>>> Undefined symbols for architecture x86_64:
>>>>>   "_gFoo.llvm.17695746433417383459", referenced from:
>>>>>       _main in lto.o
>>>>>
>>>>
>>>> I can't reproduce this, although I am using gold
>>>> (with  -Wl,-plugin-opt,cache-dir=lto.cache) since I don't have ld64. I
>>>> notice that test() is dead, so with gold at least it is removed completely.
>>>>
>>>> Do you have any temp files you can share (i.e. IR before and after
>>>> importing)?
>>>>
>>>>
>>>> Attach all the intermediate file. I haven't had time to look in detail
>>>> yet.
>>>>
>>>>
>>>
>>> --
>>> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>>>
>>
>>
>> --
>> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>
>

-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181116/5051b4cf/attachment-0001.html>