[llvm] e97aab8 - [ThinLTO] Fix import of multiply defined global variables

[Kristina Bessonova via llvm-commits]

Author: Kristina Bessonova
Date: 2021-02-21T18:34:12+02:00
New Revision: e97aab8d1510a484a19016cf079e1a8f913cf7cd

URL: https://github.com/llvm/llvm-project/commit/e97aab8d1510a484a19016cf079e1a8f913cf7cd
DIFF: https://github.com/llvm/llvm-project/commit/e97aab8d1510a484a19016cf079e1a8f913cf7cd.diff

LOG: [ThinLTO] Fix import of multiply defined global variables

Currently, if there is a module that contains a strong definition of
a global variable and a module that has both a weak definition for
the same global and a reference to it, it may result in an undefined symbol error
while linking with ThinLTO.

It happens because:
* the strong definition become internal because it is read-only and can be imported;
* the weak definition gets replaced by a declaration because it's non-prevailing;
* the strong definition failed to be imported because the destination module
  already contains another definition of the global yet this def is non-prevailing.

The patch adds a check to computeImportForReferencedGlobals() that allows
considering a global variable for being imported even if the module contains
a definition of it in the case this def has an interposable linkage type.

Note that currently the check is based only on the linkage type
(and this seems to be enough at the moment), but it might be worth to account
the information whether the def is prevailing or not.

Reviewed By: tejohnson

Differential Revision: https://reviews.llvm.org/D95943

Added: 
    llvm/test/ThinLTO/X86/weak_globals_import.ll

Modified: 
    llvm/lib/Transforms/IPO/FunctionImport.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index e1273f00e6e7..988f94a9ec46 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -275,6 +275,29 @@ updateValueInfoForIndirectCalls(const ModuleSummaryIndex &Index, ValueInfo VI) {
   return Index.getValueInfo(GUID);
 }
 
+static bool shouldImportGlobal(const ValueInfo &VI,
+                               const GVSummaryMapTy &DefinedGVSummaries) {
+  const auto &GVS = DefinedGVSummaries.find(VI.getGUID());
+  if (GVS == DefinedGVSummaries.end())
+    return true;
+  // We should not skip import if the module contains a definition with
+  // interposable linkage type. This is required for correctness in
+  // the situation with two following conditions:
+  // * the def with interposable linkage is non-prevailing,
+  // * there is a prevailing def available for import and marked read-only.
+  // In this case, the non-prevailing def will be converted to a declaration,
+  // while the prevailing one becomes internal, thus no definitions will be
+  // available for linking. In order to prevent undefined symbol link error,
+  // the prevailing definition must be imported.
+  // FIXME: Consider adding a check that the suitable prevailing definition
+  // exists and marked read-only.
+  if (VI.getSummaryList().size() > 1 &&
+      GlobalValue::isInterposableLinkage(GVS->second->linkage()))
+    return true;
+
+  return false;
+}
+
 static void computeImportForReferencedGlobals(
     const GlobalValueSummary &Summary, const ModuleSummaryIndex &Index,
     const GVSummaryMapTy &DefinedGVSummaries,
@@ -282,7 +305,7 @@ static void computeImportForReferencedGlobals(
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
   for (auto &VI : Summary.refs()) {
-    if (DefinedGVSummaries.count(VI.getGUID())) {
+    if (!shouldImportGlobal(VI, DefinedGVSummaries)) {
       LLVM_DEBUG(
           dbgs() << "Ref ignored! Target already in destination module.\n");
       continue;
@@ -378,6 +401,9 @@ static void computeImportForFunction(
       continue;
 
     if (DefinedGVSummaries.count(VI.getGUID())) {
+      // FIXME: Consider not skipping import if the module contains
+      // a non-prevailing def with interposable linkage. The prevailing copy
+      // can safely be imported (see shouldImportGlobal()).
       LLVM_DEBUG(dbgs() << "ignored! Target already in destination module.\n");
       continue;
     }

diff  --git a/llvm/test/ThinLTO/X86/weak_globals_import.ll b/llvm/test/ThinLTO/X86/weak_globals_import.ll
new file mode 100644
index 000000000000..90f2f831e6cc
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/weak_globals_import.ll
@@ -0,0 +1,33 @@
+; RUN: split-file %s %t.dir
+
+; RUN: opt -module-summary %t.dir/1.ll -o %t1.bc
+; RUN: opt -module-summary %t.dir/2.ll -o %t2.bc
+
+; RUN: llvm-lto2 run -save-temps %t1.bc %t2.bc -o %t.out \
+; RUN:               -r=%t1.bc,main,plx \
+; RUN:               -r=%t1.bc,G \
+; RUN:               -r=%t2.bc,G,pl
+; RUN: llvm-dis %t.out.1.3.import.bc -o -  | FileCheck %s
+; RUN: llvm-dis %t.out.2.3.import.bc -o -  | FileCheck %s
+
+; Test that a non-prevailing def with interposable linkage doesn't prevent
+; importing a suitable definition from a prevailing module.
+
+; CHECK: @G = internal local_unnamed_addr global i32 42
+
+;--- 1.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at G = weak dso_local local_unnamed_addr global i32 0, align 4
+
+define dso_local i32 @main() local_unnamed_addr {
+  %1 = load i32, i32* @G, align 4
+  ret i32 %1
+}
+
+;--- 2.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at G = dso_local local_unnamed_addr global i32 42, align 4