[PATCH] D36919: [ThinLTO] Fix ThinLTO crash

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: test/Transforms/FunctionImport/Inputs/funcimport_var2.ll:2
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
davidxl wrote:
> Are these necessary?
Yes, this is required when going through llvm-lto2.


================
Comment at: test/Transforms/FunctionImport/funcimport_var.ll:8
+; RUN: llvm-lto2 run %t.bc %t2.bc -o %t.out \
+; RUN:   -r %t.bc,_Z4LinkPKcS0_,plx \
+; RUN:   -r %t.bc,link,l \
----------------
davidxl wrote:
> line wrap intended?
A lot of llvm-lto2 test lines are structured this way so that it is easier to see the symbol resolutions.


================
Comment at: test/Transforms/FunctionImport/funcimport_var.ll:10
+; RUN:   -r %t.bc,link,l \
+; RUN:   -r %t2.bc,get_link,plx
+
----------------
davidxl wrote:
> what is the difference beween llvm-lto2 and llvm-lto? what are these  options for?
llvm-lto uses the legacy LTO interface, which is still used by ld64 and some other custom linkers. llvm-lto2 tests the new linker resolution based LTO interface, which is used by gold and lld. Therefore it is required that resolutions for each symbol are provided. In general I think it is good to test with llvm-lto2 as well. Note that the version of the bug I am fixing can also be tested via llvm-lto via the "run" action which passes the index in-memory, so I have added that as well. Additionally, I am piping an output ELF for each through llvm-nm with a simple CHECK, since someone once told me this was good form to make sure that the expected output was created.


https://reviews.llvm.org/D36919