[ThinLTO] Fix ThinLTO crash

[tejohnson]

Summary

Follow up to fix in r311023, which fixed the case where the combined
index is written to disk. The same samplePGO logic exists for the
in-memory index when computing imports, so we need to filter out
GlobalVariable summaries there too.

[davidxl]

test/Transforms/FunctionImport/Inputs/funcimport_var2.ll

2 ↗

(On Diff #111802)

Are these necessary?

test/Transforms/FunctionImport/funcimport_var.ll

8 ↗

(On Diff #111802)

line wrap intended?

10 ↗

(On Diff #111802)

what is the difference beween llvm-lto2 and llvm-lto? what are these options for?

[tejohnson]

test/Transforms/FunctionImport/Inputs/funcimport_var2.ll

2 ↗

(On Diff #111802)

Yes, this is required when going through llvm-lto2.

test/Transforms/FunctionImport/funcimport_var.ll

8 ↗

(On Diff #111802)

A lot of llvm-lto2 test lines are structured this way so that it is easier to see the symbol resolutions.

10 ↗

(On Diff #111802)

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.

[tejohnson]

Address comments

[davidxl]

lgtm

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

 static const GlobalValueSummary *
 selectCallee(const ModuleSummaryIndex &Index,
              ArrayRef<std::unique_ptr<GlobalValueSummary>> CalleeSummaryList,
              unsigned Threshold, StringRef CallerModulePath) {
   auto It = llvm::find_if(
       CalleeSummaryList,
       [&](const std::unique_ptr<GlobalValueSummary> &SummaryPtr) {
         auto *GVSummary = SummaryPtr.get();
+        // For SamplePGO, in computeImportForFunction the OriginalId
+        // may have been used to locate the callee summary list (See
+        // comment there).
+        // The mapping from OriginalId to GUID may return a GUID
+        // that corresponds to a static variable. Filter it out here.
+        // This can happen when
+        // 1) There is a call to a library function which is not defined
+        // in the index.
+        // 2) There is a static variable with the  OriginalGUID identical
+        // to the GUID of the library function in 1);
+        // When this happens, the logic for SamplePGO kicks in and
+        // the static variable in 2) will be found, which needs to be
+        // filtered out.
+        if (GVSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind)
+          return false;
         if (GlobalValue::isInterposableLinkage(GVSummary->linkage()))
           // There is no point in importing these, we can't inline them
           return false;
         if (isa<AliasSummary>(GVSummary))
           // Aliases can't point to "available_externally".
           // FIXME: we should import alias as available_externally *function*,
           // the destination module does not need to know it is an alias.
           return false;

llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_var2.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
 @link = internal global i32 0, align 4
 
 ; Function Attrs: norecurse nounwind readnone uwtable
 define nonnull i32* @get_link() local_unnamed_addr {
   ret i32* @link
 }
 

llvm/trunk/test/Transforms/FunctionImport/funcimport_var.ll

 ; This test makes sure a static var is not selected as a callee target
 ; (which will crash compilation).
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/funcimport_var2.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=thinlink -o %t3 %t.bc %t2.bc
 ; RUN: llvm-lto -thinlto -thinlto-action=import -thinlto-index=%t3 %t.bc %t2.bc
+; RUN: llvm-lto -thinlto -thinlto-action=run %t.bc %t2.bc
+; RUN: llvm-nm %t.bc.thinlto.o | FileCheck %s
+; RUN: llvm-lto2 run %t.bc %t2.bc -o %t.out \
+; RUN:   -r %t.bc,_Z4LinkPKcS0_,plx \
+; RUN:   -r %t.bc,link,l \
+; RUN:   -r %t2.bc,get_link,plx
+; RUN: llvm-nm %t.out.0 | FileCheck %s
+; CHECK: U link
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
 define i32 @_Z4LinkPKcS0_(i8*, i8*) local_unnamed_addr  {
   %3 = tail call i32 @link(i8* %0, i8* %1) #2
   ret i32 %3
 }
 
 ; Function Attrs: nounwind
 declare i32 @link(i8*, i8*) local_unnamed_addr