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

Wed Nov 14 16:28:03 PST 2018

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)?

>
> Steven
>
>
> On Nov 14, 2018, at 1:11 PM, Steven Wu <stevenwu at apple.com> wrote:
>
> I am making some low progress on reproducing it on trunk. It is not very
> easy to get into this state but this is what I found right now:
>
> ParseAST always has readonly reference to gCrashRecoveryEnabled and in
> both linkage unit, gCrashRecoveryEnabled is imported into ParseAST. The
> difference is that for libclang, there is a different module which modifies
> gCrashRecoveryEnabled. Maybe I am missing something but will there be any
> difference in hash value for ParseAST?
>
> After importing, I see:
>
> libclang ParseAST
> @_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 =
> available_externally hidden unnamed_addr global i1 false, align 1
>
> clang-func-mapping ParseAST
> @_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 = internal
> unnamed_addr global i1 false, align 1 #0
> attributes #0 = { "thinlto-internalize" }
>
> I am little suspecting about the libclang case. Is this semantically
> correct if it is inlined? It will not observe the changes to the value if
> modified.
>
> Steven
>
>
> On Nov 14, 2018, at 9:57 AM, Teresa Johnson <tejohnson at google.com> wrote:
>
>
>
> On Wed, Nov 14, 2018 at 9:42 AM Steven Wu <stevenwu at apple.com> wrote:
>
>> For the stage 2 build, just
>> use clang/cmake/caches/Apple-stage2-ThinLTO.cmake. I also throw a profile
>> data in my build just to match our builder but I suspect that might not be
>> needed. I will double check to confirm that.
>>
>> You also don't need to wait for the entire build to finish. Just run
>> `ninja libclang.dylib && ninja clang-func-mapping` is consistently
>> reproducing for me. Let me know if you need more help with this.
>>
>> Another note, it will be good to add assembly support for readonly bits.
>> It will be easier to debug.
>>
>
> Good point - I completely forgot about this when reviewing. Eugene - you
> will need to change AsmWriter.cpp, LLToken.h, LLParser.cpp and LLLexer.cpp
> (see the changes to those files in D53345 as an example, although that was
> adding a new function flag, but you would want to do something similar for
> the new gvar flag).
>
> Teresa
>
>
>> Steven
>>
>> On Nov 14, 2018, at 9:27 AM, Evgeny Leviant <eleviant at accesssoftek.com>
>> wrote:
>>
>> I couldn't reproduce this on Linux, now trying Mac OS X (will take the
>> whole night as it is slow)
>> Stewen, can you please send me CMake configuration string from your
>> buildbot. It would be helpful
>> if pending OS X build also succeed.
>>
>> What I'm doing is:
>> - build LLVM/clang with thin LTO cache
>> - When finished remove cache from disk, then remove and rebuild
>> libclang.so(dylib)
>> - Remove and rebuild clang-func-mapping
>>
>> Are those steps correct?
>> ------------------------------
>> *От:* Teresa Johnson via Phabricator <reviews at reviews.llvm.org>
>> *Отправлено:* 14 ноября 2018 г. 19:00
>> *Кому:* Evgeny Leviant; tejohnson at google.com; joker.eph at gmail.com
>> *Копия:* trent.xin.tong at gmail.com; arphaman at gmail.com;
>> dany.grumberg at gmail.com; jfbastien at apple.com; anton at korobeynikov.info;
>> llvm at inglorion.net; eraman at google.com; stevenwu at apple.com;
>> dexonsmith at apple.com; llvm-commits at lists.llvm.org; George Rimar; Igor
>> Kudrin; jun.l at samsung.com
>> *Тема:* [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.
>>
>> tejohnson added a comment.
>>
>> In https://reviews.llvm.org/D49362#1297188, @steven_wu wrote:
>>
>> > I reverted this in r346768.
>> >
>> > I think this is indeed a caching problem. There are some dylib/binary
>> in clang projects that toggles whether to enable the recovery context but
>> some does not. I can reproduce the issue by thin link libclang.dylib first,
>> then link clang-func-mapping on Darwin.
>> >  libclang.dylib thinks the file scope variable gCrashRecoveryEnabled
>> not read-only, so it promotes it.
>> >  clang-func-mapping thinks gCrashRecoveryEnabled read-only, so it
>> internalize and constant propagate the variable but ParseAST.o in the cache
>> is still expecting gCrashRecoveryEnabled to be available.
>> >
>> > Let me know if you need more information.
>>
>>
>> Thinking through this example, I'm not sure what is going on. This patch
>> did change the cache key computation to include the read only bit from the
>> global var summary of anything defined or imported. Since
>> gCrashRecoveryEnabled is a static in CrashRecoveryContext.cpp, it must have
>> been imported into ParseAST.o, which means that the read only bit on the
>> associated gvar summary should have been hashed into ParseAST.o's cache
>> key. So presumably we shouldn't have had a cache hit for ParseAST.o (built
>> when the read only bit is not set on gCrashRecoveryEnabled) when thin
>> linking clang-func-mapping which has this bit set for that variable.
>>
>> Even if the variable was originally externally visible, in order for it
>> to be marked read only by the thin link it would have had to have been
>> imported at all use sites. Which means that any referencing module had to
>> have it in the import set (and therefore would have hashed the read only
>> bit) in the thin link where it was marked read only. So I am not sure
>> offhand why we could ever share a referencing object between a link where
>> it was read only and another link where it is not...   Hopefully Eugene can
>> figure out what is going wrong here.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D49362
>>
>>
>>
>>
>>
>
> --
> 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/20181114/e26cd70a/attachment.html>