Fix thinlto cache key computation for cfi-icall.

[eugenis]

Summary

Fixes PR33966.

CFI code generation for users (not just callers) of a function depends
on whether this function has a jumptable entry or not. This
information needs to be encoded in thinlto cache key.

We filter the jumptable list against functions that are actually
referenced in the current module.

[pcc]

llvm/lib/LTO/LTO.cpp

233

We are recomputing this map (and the one below) once for each input file, which would seem to create O(NM) complexity. Can we avoid that by moving the map computation into the InProcessThinBackend? See for example how we handle TypeIdSummariesByGuid.

256

I think you can move the body of this loop into AddTypeIdSummary (after giving it a better name).

258

I think you also need to include calls().

llvm/test/ThinLTO/X86/Inputs/cache-icall-no.ll

1

Do you need this file as part of your test case? I think you just need to *not* provide an LTO definition of f. That's basically what the reproducer in the PR is doing.

[eugenis]

Addressed pcc's comments.

[pcc]

LGTM

llvm/lib/LTO/LTO.cpp

184

So GS can't be null here? If so, you can use dyn_cast below.

887

I think you need to call GlobalValue::dropLLVMManglingEscape on the function name here (same below).

[eugenis]

add a null check && dropLLVMManglingEscape

[eugenis]

llvm/lib/LTO/LTO.cpp

184

in fact, it can (but not in the test suite)

[eugenis]

typo

llvm/lib/LTO/LTO.cpp

for (auto &TT : FS->type_checked_load_const_vcalls())
       UsedTypeIds.insert(TT.VFunc.GUID);
   };
 
   // Include the hash for the linkage type to reflect internalization and weak
   // resolution, and collect any used type identifier resolutions.
   for (auto &GS : DefinedGlobals) {
     GlobalValue::LinkageTypes Linkage = GS.second->linkage();
     Hasher.update(
         ArrayRef<uint8_t>((const uint8_t *)&Linkage, sizeof(Linkage)));
     AddUsedTypeIds(GS.second);
   }
 
   // Imported functions may introduce new uses of type identifier resolutions,
   // so we need to collect their used resolutions as well.
   for (auto &ImpM : ImportList)
     for (auto &ImpF : ImpM.second)
       AddUsedTypeIds(Index.findSummaryInModule(ImpF.first, ImpM.first()));
#endif
   }
 
   if (!Conf.SampleProfile.empty()) {
     auto FileOrErr = MemoryBuffer::getFile(Conf.SampleProfile);
     if (FileOrErr)
       Hasher.update(FileOrErr.get()->getBuffer());
   }
 
+  // Include the hashes of all members of cfiFunctionDefs and cfiFunctionDecls
+  // that are referenced or defined in this module.
+  DenseMap<GlobalValue::GUID, bool> UsedCfiDefs;
+  DenseMap<GlobalValue::GUID, bool> UsedCfiDecls;
+  unsigned UsedCfiDefsCount = 0;
+  unsigned UsedCfiDeclsCount = 0;
+
+  for (const std::string& Name : Index.cfiFunctionDefs())
+    UsedCfiDefs[GlobalValue::getGUID(Name)] = false;
+  for (const std::string& Name : Index.cfiFunctionDecls())
+    UsedCfiDecls[GlobalValue::getGUID(Name)] = false;
+
+  auto AddUsedCfiGlobal = [&](GlobalValue::GUID ValueGUID) {
+    auto DefsIt = UsedCfiDefs.find(ValueGUID);
+    if (DefsIt != UsedCfiDefs.end()) {
+      UsedCfiDefsCount += !DefsIt->second;
+      DefsIt->second = true;
+    }
+    auto DeclsIt = UsedCfiDecls.find(ValueGUID);
+    if (DeclsIt != UsedCfiDecls.end()) {
+      UsedCfiDeclsCount += !DeclsIt->second;
+      DeclsIt->second = true;
+    }
+  };
+
+  for (auto &GSM : DefinedGlobals) {
+    AddUsedCfiGlobal(GSM.first);
+    for (const ValueInfo &VI : GSM.second->refs())
+      AddUsedCfiGlobal(VI.getGUID());
+  }
+
+  for (auto &ImpM : ImportList)
+    for (auto &ImpF : ImpM.second) {
+      AddUsedCfiGlobal(ImpF.first); // FIXME: is this necessary?
+      GlobalValueSummary *GS = Index.findSummaryInModule(ImpF.first, ImpM.first());
+      for (const ValueInfo &VI : GS->refs())
+        AddUsedCfiGlobal(VI.getGUID());
+    }
+
+  AddUnsigned(UsedCfiDefsCount);
+  for (auto &KV : UsedCfiDefs)
+    if (KV.second)
+      AddUint64(KV.first);
+
+  AddUnsigned(UsedCfiDeclsCount);
+  for (auto &KV : UsedCfiDecls)
+    if (KV.second)
+      AddUint64(KV.first);
+
   Key = toHex(Hasher.result());
 }
 
 static void thinLTOResolveWeakForLinkerGUID(
     GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
     DenseSet<GlobalValueSummary *> &GlobalInvolvedWithAlias,
     function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
         isPrevailing,
InProcessThinBackend(
         AddStream(std::move(AddStream)), Cache(std::move(Cache)) {
     // Create a mapping from type identifier GUIDs to type identifier summaries.
     // This allows backends to use the type identifier GUIDs stored in the
     // function summaries to determine which type identifier summaries affect
     // each function without needing to compute GUIDs in each backend.
     for (auto &TId : CombinedIndex.typeIds())
       TypeIdSummariesByGuid[GlobalValue::getGUID(TId.first)].push_back(&TId);
   }
 
   Error runThinLTOBackendThread(
       AddStreamFn AddStream, NativeObjectCache Cache, unsigned Task,
       BitcodeModule BM, ModuleSummaryIndex &CombinedIndex,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
       const GVSummaryMapTy &DefinedGlobals,
       MapVector<StringRef, BitcodeModule> &ModuleMap,

llvm/test/ThinLTO/X86/Inputs/cache-icall-no.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @f() {
+entry:
+  ret void
+}

llvm/test/ThinLTO/X86/Inputs/cache-icall-yes.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @f() !type !0 {
+entry:
+  ret void
+}
+
+!0 = !{i64 0, !"_ZTSFvvE"}

llvm/test/ThinLTO/X86/cache-icall.ll

+; Test that the list of CFI jumptable entries is part of ThinLTO cache key.
+
+; Linking module -yes results in f() being added to CFI jumptable; with -no it is not.
+; This affects code generated for any users of f(). Make sure that we don't pull a stale object
+; file for %t.o from the cache when -no is replaced with -yes in the link.
+
+; RUN: opt -module-hash -module-summary -thinlto-bc %s -o %t.bc
+; RUN: opt -module-hash -module-summary -thinlto-bc %p/Inputs/cache-icall-no.ll -o %t-no.bc
+; RUN: opt -module-hash -module-summary -thinlto-bc %p/Inputs/cache-icall-yes.ll -o %t-yes.bc
+
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+
+; RUN: llvm-lto2 run -o %t-no.o %t.bc %t-no.bc -cache-dir %t.cache \
+; RUN:   -r=%t.bc,_start,px \
+; RUN:   -r=%t.bc,f, \
+; RUN:   -r=%t-no.bc,f,p
+
+; RUN: llvm-readelf -symbols %t-no.o.* | FileCheck %s --check-prefix=SYMBOLS-NO
+
+; RUN: llvm-lto2 run -o %t-yes.o %t.bc %t-yes.bc -cache-dir %t.cache \
+; RUN:   -r=%t.bc,_start,px \
+; RUN:   -r=%t.bc,f, \
+; RUN:   -r=%t-yes.bc,f,p
+
+; RUN: llvm-readelf -symbols %t-yes.o.* | FileCheck %s --check-prefix=SYMBOLS-YES
+
+; SYMBOLS-NO-DAG: {{FUNC .* f.cfi_jt$}}
+; SYMBOLS-NO-DAG: {{NOTYPE .* UND f.cfi_jt$}}
+
+; SYMBOLS-YES-NOT: f.cfi_jt
+; SYMBOLS-YES-DAG: {{FUNC .* f.cfi$}}
+; SYMBOLS-YES-DAG: {{NOTYPE .* UND f.cfi$}}
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i8* @_start(void ()* %p) !type !0 {
+entry:
+  %0 = bitcast void ()* %p to i8*
+  %1 = tail call i1 @llvm.type.test(i8* %0, metadata !"_ZTSFvvE")
+  br i1 %1, label %cont, label %trap
+
+trap:                                             ; preds = %entry
+  tail call void @llvm.trap()
+  unreachable
+
+cont:                                             ; preds = %entry
+  tail call void %p()
+  ret i8* bitcast (void ()* @f to i8*)
+}
+
+declare i1 @llvm.type.test(i8*, metadata)
+declare void @llvm.trap()
+declare !type !1 void @f()
+
+!0 = !{i64 0, !"_ZTSFPvPFvvEE"}
+!1 = !{i64 0, !"_ZTSFvvE"}