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

[Evgeny Leviant via llvm-commits]

​Teresa, does it make sense to fix the issue and recommit or it's better to wait for your patch?

________________________________
От: 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<mailto:tejohnson at google.com>> wrote:


On Thu, Nov 15, 2018 at 6:22 AM Evgeny Leviant <eleviant at accesssoftek.com<mailto: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<mailto:stevenwu at apple.com> <stevenwu at apple.com<mailto:stevenwu at apple.com>> от имени Steven Wu <stevenwu at apple.com<mailto:stevenwu at apple.com>>
Отправлено: 15 ноября 2018 г. 3:34
Кому: Teresa Johnson
Копия: Evgeny Leviant; Mehdi AMINI; reviews+D49362+public+6f0316d7b1456727 at reviews.llvm.org<mailto:reviews%2BD49362%2Bpublic%2B6f0316d7b1456727 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<mailto: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<mailto:tejohnson at google.com>> wrote:



On Wed, Nov 14, 2018 at 1:47 PM Steven Wu <stevenwu at apple.com<mailto: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<mailto:tejohnson at google.com> |



--
Teresa Johnson |         Software Engineer |     tejohnson at google.com<mailto:tejohnson at google.com> |

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181115/4d8048d8/attachment.html>