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

[Steven Wu via llvm-commits]

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.

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 |	
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181115/5b412338/attachment.html>