[ThinLTO] Fix import of multiply defined global variables

[krisb]

Summary

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 got internalized because it is read-only and can be imported;
• the weak definition got 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), but it might be worth to account the information
whether the def is prevailing or not.

[tejohnson]

Thanks. AFAICT this is a reasonable fix. Suggestion below for a better comment and FIXME to add.

llvm/lib/Transforms/IPO/FunctionImport.cpp

288

Can you add a comment here about how this is required for correctness?
Also, add a FIXME that we can refine this further if we pass down the prevailing symbol information, and only need to allow imports if this is not the prevailing copy, and further if the prevailing copy is marked read only?

[krisb]

Add the comment and FIXME.

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

294

Can you add that this only affects interposable symbols since they are converted to declarations.

295

I think we would still want to check linkage type, since it is only interposable that are converted to declarations. The prevailing information would be useful to refine the extra importing further. Hmm, actually I think there might be a correctness issue with this fix as is. I notice in your test below that the imported symbol becomes internalized. I believe this will need to stay interposable. Normally interposable symbols are blocked from inlining and other IPO since they can be overridden at link time. By making the imported copy internalized, it can now be inlined and no longer overridden at link time. If we did ensure that we imported the prevailing copy this might be ok. However, there are going to be cases where that is not possible (e.g. doing ThinLTO on a shared library link, or if the prevailing copy is in an ELF object/shared library and therefore not available for importing). So I think the best thing here is to ensure that when it gets imported it retains the weak linkage type. I'm not sure if is being internalized in this particular test case because the imported copy is read only - can you confirm where/how that is happening?

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

295

(ignore the comment about inlining, since we're dealing with variables, but other IPO applies)

[krisb]

Thanks, @tejohnson, for reviewing this!
I'll make the comment more accurate.

I think we would still want to check linkage type, since it is only interposable that are converted to declarations. The prevailing information would be useful to refine the extra importing further. Hmm, actually I think there might be a correctness issue with this fix as is. I notice in your test below that the imported symbol becomes internalized. I believe this will need to stay interposable. Normally interposable symbols are blocked from inlining and other IPO since they can be overridden at link time. By making the imported copy internalized, it can now be inlined and no longer overridden at link time. If we did ensure that we imported the prevailing copy this might be ok. However, there are going to be cases where that is not possible (e.g. doing ThinLTO on a shared library link, or if the prevailing copy is in an ELF object/shared library and therefore not available for importing). So I think the best thing here is to ensure that when it gets imported it retains the weak linkage type. I'm not sure if is being internalized in this particular test case because the imported copy is read only - can you confirm where/how that is happening?

The imported copy becomes internal because the source def is read-only indeed (the imported copy inherits 'thinlto-internalize' attribute from the source one thus it got internalized in internalizeGVsAfterImport()). This seems okay since access from an ELF object/shared library prevents the source copy to be marked ReadOnly and at that time we already have attributes propagated.
In general, allow importing here we let it follow the normal importing rules as if the destination module doesn't contain a def at all. So that we will not import another copy with interposable linkage (it doesn't pass the check from canImportGlobalVar()). For other cases, the final linkage type will be whatever FunctionImportGlobalProcessing::getLinkage() returns for the source copy. Unless I'm missing something, this seems to be safe.

[tejohnson]

In D95943#2548382, @krisb wrote:

Thanks, @tejohnson, for reviewing this!
I'll make the comment more accurate.

I think we would still want to check linkage type, since it is only interposable that are converted to declarations. The prevailing information would be useful to refine the extra importing further. Hmm, actually I think there might be a correctness issue with this fix as is. I notice in your test below that the imported symbol becomes internalized. I believe this will need to stay interposable. Normally interposable symbols are blocked from inlining and other IPO since they can be overridden at link time. By making the imported copy internalized, it can now be inlined and no longer overridden at link time. If we did ensure that we imported the prevailing copy this might be ok. However, there are going to be cases where that is not possible (e.g. doing ThinLTO on a shared library link, or if the prevailing copy is in an ELF object/shared library and therefore not available for importing). So I think the best thing here is to ensure that when it gets imported it retains the weak linkage type. I'm not sure if is being internalized in this particular test case because the imported copy is read only - can you confirm where/how that is happening?

The imported copy becomes internal because the source def is read-only indeed (the imported copy inherits 'thinlto-internalize' attribute from the source one thus it got internalized in internalizeGVsAfterImport()). This seems okay since access from an ELF object/shared library prevents the source copy to be marked ReadOnly and at that time we already have attributes propagated.
In general, allow importing here we let it follow the normal importing rules as if the destination module doesn't contain a def at all. So that we will not import another copy with interposable linkage (it doesn't pass the check from canImportGlobalVar()). For other cases, the final linkage type will be whatever FunctionImportGlobalProcessing::getLinkage() returns for the source copy. Unless I'm missing something, this seems to be safe.

Ok I think I have convinced myself that this is correct:

1. If the prevailing copy is not available for import (e.g. in an ELF object), these IR copies would have been marked dead and therefore we would not have marked them as read only.
2. Any copy available for import, i.e. not interposable, would have to be a linkage type that cannot be overridden. So it would have to either be the prevailing copy, or one odr equivalent.

Hmm, it seems like we could do something similar for function importing, to allow importing and inlining of the prevailing copy when the copy in the module is interposable. Can you add a FIXME where we check "if (DefinedGVSummaries.count(VI.getGUID()))" in computeImportForFunction?

[krisb]

Made comments more detailed and acccurate.

[tejohnson]

lgtm with some comment tweaks suggested below for clarity. Thanks!

llvm/lib/Transforms/IPO/FunctionImport.cpp

292

Since the next two points (about the non-prevailing interposable becoming a decl, and then the read-only prevailing being internalized) are both about the correctness and need to go together to create the failure, can you reorder this to:
"This is required for the correctness purpose: First of all, if this copy is non-prevailing...."
Then reorder the second point slightly:
"On the other side, if the prevailing copy is read-only and suitable for import, it will be later internalized and thus not available for linking, and therefore must be imported to satisfy any references."

397

Nit: For clarity, add: "The prevailing copy can safely be imported."

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

288

Nit: flip the condition so that the "then" branch can be deleted, and the comment can be dedented by one level.

[MaskRay]

llvm/test/ThinLTO/X86/weak_globals_import.ll

2

Option: you may consider the split-file utility to keep the auxiliary file in this file. It may help readers in some cases as they don't need to open another file.

[krisb]

Applied review comments, reduced the test.

llvm/lib/Transforms/IPO/FunctionImport.cpp

288

Extracted the check into a separate function. Hope it can be reused for computeImportForFunction() as well.

292

Thank you for the suggestion! I rephrased the comment and hope it's more clear now.

llvm/test/ThinLTO/X86/weak_globals_import.ll

2

Thank you! I haven't heard about split-file.

[tejohnson]

lgtm - thanks!

llvm/lib/Transforms/IPO/FunctionImport.cpp

 
 static void computeImportForReferencedGlobals(
     const GlobalValueSummary &Summary, const ModuleSummaryIndex &Index,
     const GVSummaryMapTy &DefinedGVSummaries,
     SmallVectorImpl<EdgeInfo> &Worklist,
     FunctionImporter::ImportMapTy &ImportList,
     StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
   for (auto &VI : Summary.refs()) {
-    if (DefinedGVSummaries.count(VI.getGUID())) {
+    // Find if the module already contains a suitable definition.
+    const auto &GVS = DefinedGVSummaries.find(VI.getGUID());
+    if (GVS != DefinedGVSummaries.end()) {
+      if (VI.getSummaryList().size() > 1 &&
+          GlobalValue::isInterposableLinkage(GVS->second->linkage())) {
+        // If the module contains a non-prevailing def of a multiply defined
+        // global, it still requires a prevailing one to be imported. Otherwise,
+        // it may result in an undefined symbol because if the prevailing copy
+        // is read-only it will be internalized thus become not available for
+        // linking.
+        // FIXME: Instead of checking for linkage type, use prevailing symbol
+        // information and ensure the prevailing copy is read-only before allow
+        // importing.
+        ;
+      } else {
         LLVM_DEBUG(
             dbgs() << "Ref ignored! Target already in destination module.\n");
         continue;
       }
+    }
 
     LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
 
     // If this is a local variable, make sure we import the copy
     // in the caller's module. The only time a local variable can
     // share an entry in the index is if there is a local with the same name
     // in another module that had the same source file name (in a different
     // directory), where each was compiled in their own directory so there
for (auto &Edge : Summary.calls()) {
     }
 
     VI = updateValueInfoForIndirectCalls(Index, VI);
     if (!VI)
       continue;
 
     if (DefinedGVSummaries.count(VI.getGUID())) {
       LLVM_DEBUG(dbgs() << "ignored! Target already in destination module.\n");
       continue;
     }
 
     auto GetBonusMultiplier = [](CalleeInfo::HotnessType Hotness) -> float {
       if (Hotness == CalleeInfo::HotnessType::Hot)
         return ImportHotMultiplier;
       if (Hotness == CalleeInfo::HotnessType::Cold)
         return ImportColdMultiplier;
       if (Hotness == CalleeInfo::HotnessType::Critical)

llvm/test/ThinLTO/X86/Inputs/weak_globals_import.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"
+
+@G = dso_local local_unnamed_addr global i32 42, align 4
+
+; Function Attrs: noinline
+define dso_local i32 @_Z3foov() local_unnamed_addr #0 {
+  ret i32 0
+}
+
+attributes #0 = { noinline }

llvm/test/ThinLTO/X86/weak_globals_import.ll

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/weak_globals_import.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,_Z3foov,l \
+; RUN:               -r=%t1.bc,G \
+; RUN:               -r=%t2.bc,_Z3foov,pl \
+; 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 prevailing def for a global variable got imported even if
+; a destination module already contains non-prevailing one.
+
+; CHECK: @G = internal local_unnamed_addr global i32 42
+
+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"
+
+@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
+  %2 = tail call i32 @_Z3foov()
+  %3 = add nsw i32 %2, %1
+  ret i32 %3
+}
+
+declare dso_local i32 @_Z3foov() local_unnamed_addr