[ThinLTO] Drop dso_local if a GlobalVariable satisfies isDeclarationF…

…orLinker() dso_local leads to direct access even if the definition is not within this compilation unit (it is still in the same linkage unit). On ELF, such a relocation (e.g. R_X86_64_PC32) referencing a STB_GLOBAL STV_DEFAULT object can cause a linker error in a -shared link. If the linkage is changed to available_externally, the dso_local flag should be dropped, so that no direct access will be generated. The current behavior is benign, because -fpic does not assume dso_local (clang/lib/CodeGen/CodeGenModule.cpp:shouldAssumeDSOLocal). If we do that for -fno-semantic-interposition (D73865), there will be an R_X86_64_PC32 linker error without this patch. Reviewed By: tejohnson Differential Revision: https://reviews.llvm.org/D74751
llvm · Apr 7, 2020 · d2ef8c1 · d2ef8c1
1 parent 2f8fb4d
commit d2ef8c1
Show file tree

Hide file tree

Showing 19 changed files with 156 additions and 50 deletions.
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -105,8 +105,10 @@ class FunctionImporter {
       std::function<Expected<std::unique_ptr<Module>>(StringRef Identifier)>;
 
   /// Create a Function Importer.
-  FunctionImporter(const ModuleSummaryIndex &Index, ModuleLoaderTy ModuleLoader)
-      : Index(Index), ModuleLoader(std::move(ModuleLoader)) {}
+  FunctionImporter(const ModuleSummaryIndex &Index, ModuleLoaderTy ModuleLoader,
+                   bool ClearDSOLocalOnDeclarations)
+      : Index(Index), ModuleLoader(std::move(ModuleLoader)),
+        ClearDSOLocalOnDeclarations(ClearDSOLocalOnDeclarations) {}
 
   /// Import functions in Module \p M based on the supplied import list.
   Expected<bool> importFunctions(Module &M, const ImportMapTy &ImportList);
@@ -117,6 +119,10 @@ class FunctionImporter {
 
   /// Factory function to load a Module for a given identifier
   ModuleLoaderTy ModuleLoader;
+
+  /// See the comment of ClearDSOLocalOnDeclarations in
+  /// Utils/FunctionImportUtils.h.
+  bool ClearDSOLocalOnDeclarations;
 };
 
 /// The function importing pass

diff --git a/llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h b/llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h
@@ -39,6 +39,19 @@ class FunctionImportGlobalProcessing {
   /// as part of a different backend compilation process.
   bool HasExportedFunctions = false;
 
+  /// Set to true (only applicatable to ELF -fpic) if dso_local should be
+  /// dropped for a declaration.
+  ///
+  /// On ELF, the assembler is conservative and assumes a global default
+  /// visibility symbol can be interposable. No direct access relocation is
+  /// allowed, if the definition is not in the translation unit, even if the
+  /// definition is available in the linkage unit. Thus we need to clear
+  /// dso_local to disable direct access.
+  ///
+  /// This flag should not be set for -fno-pic or -fpie, which would
+  /// unnecessarily disable direct access.
+  bool ClearDSOLocalOnDeclarations;
+
   /// Set of llvm.*used values, in order to validate that we don't try
   /// to promote any non-renamable values.
   SmallPtrSet<GlobalValue *, 8> Used;
@@ -85,10 +98,11 @@ class FunctionImportGlobalProcessing {
   GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV, bool DoPromote);
 
 public:
-  FunctionImportGlobalProcessing(
-      Module &M, const ModuleSummaryIndex &Index,
-      SetVector<GlobalValue *> *GlobalsToImport = nullptr)
-      : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport) {
+  FunctionImportGlobalProcessing(Module &M, const ModuleSummaryIndex &Index,
+                                 SetVector<GlobalValue *> *GlobalsToImport,
+                                 bool ClearDSOLocalOnDeclarations)
+      : M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport),
+        ClearDSOLocalOnDeclarations(ClearDSOLocalOnDeclarations) {
     // If we have a ModuleSummaryIndex but no function to import,
     // then this is the primary module being compiled in a ThinLTO
     // backend compilation, and we need to see if it has functions that
@@ -111,6 +125,7 @@ class FunctionImportGlobalProcessing {
 /// exported local functions renamed and promoted for ThinLTO.
 bool renameModuleForThinLTO(
     Module &M, const ModuleSummaryIndex &Index,
+    bool ClearDSOLocalOnDeclarations,
     SetVector<GlobalValue *> *GlobalsToImport = nullptr);
 
 /// Compute synthetic function entry counts.

diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
@@ -521,7 +521,13 @@ Error lto::thinBackend(const Config &Conf, unsigned Task, AddStreamFn AddStream,
   if (Conf.PreOptModuleHook && !Conf.PreOptModuleHook(Task, Mod))
     return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
 
-  renameModuleForThinLTO(Mod, CombinedIndex);
+  // When linking an ELF shared object, dso_local should be dropped. We
+  // conservatively do this for -fpic.
+  bool ClearDSOLocalOnDeclarations =
+      TM->getTargetTriple().isOSBinFormatELF() &&
+      TM->getRelocationModel() != Reloc::Static &&
+      Mod.getPIELevel() == PIELevel::Default;
+  renameModuleForThinLTO(Mod, CombinedIndex, ClearDSOLocalOnDeclarations);
 
   dropDeadSymbols(Mod, DefinedGlobals, CombinedIndex);
 
@@ -547,7 +553,8 @@ Error lto::thinBackend(const Config &Conf, unsigned Task, AddStreamFn AddStream,
                                    /*IsImporting*/ true);
   };
 
-  FunctionImporter Importer(CombinedIndex, ModuleLoader);
+  FunctionImporter Importer(CombinedIndex, ModuleLoader,
+                            ClearDSOLocalOnDeclarations);
   if (Error Err = Importer.importFunctions(Mod, ImportList).takeError())
     return Err;
 

diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -152,8 +152,9 @@ generateModuleMap(std::vector<std::unique_ptr<lto::InputFile>> &Modules) {
   return ModuleMap;
 }
 
-static void promoteModule(Module &TheModule, const ModuleSummaryIndex &Index) {
-  if (renameModuleForThinLTO(TheModule, Index))
+static void promoteModule(Module &TheModule, const ModuleSummaryIndex &Index,
+                          bool ClearDSOLocalOnDeclarations) {
+  if (renameModuleForThinLTO(TheModule, Index, ClearDSOLocalOnDeclarations))
     report_fatal_error("renameModuleForThinLTO failed");
 }
 
@@ -205,15 +206,16 @@ static std::unique_ptr<Module> loadModuleFromInput(lto::InputFile *Input,
 
 static void
 crossImportIntoModule(Module &TheModule, const ModuleSummaryIndex &Index,
-                      StringMap<lto::InputFile*> &ModuleMap,
-                      const FunctionImporter::ImportMapTy &ImportList) {
+                      StringMap<lto::InputFile *> &ModuleMap,
+                      const FunctionImporter::ImportMapTy &ImportList,
+                      bool ClearDSOLocalOnDeclarations) {
   auto Loader = [&](StringRef Identifier) {
     auto &Input = ModuleMap[Identifier];
     return loadModuleFromInput(Input, TheModule.getContext(),
                                /*Lazy=*/true, /*IsImporting*/ true);
   };
 
-  FunctionImporter Importer(Index, Loader);
+  FunctionImporter Importer(Index, Loader, ClearDSOLocalOnDeclarations);
   Expected<bool> Result = Importer.importFunctions(TheModule, ImportList);
   if (!Result) {
     handleAllErrors(Result.takeError(), [&](ErrorInfoBase &EIB) {
@@ -411,8 +413,15 @@ ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
   // "Benchmark"-like optimization: single-source case
   bool SingleModule = (ModuleMap.size() == 1);
 
+  // When linking an ELF shared object, dso_local should be dropped. We
+  // conservatively do this for -fpic.
+  bool ClearDSOLocalOnDeclarations =
+      TM.getTargetTriple().isOSBinFormatELF() &&
+      TM.getRelocationModel() != Reloc::Static &&
+      TheModule.getPIELevel() == PIELevel::Default;
+
   if (!SingleModule) {
-    promoteModule(TheModule, Index);
+    promoteModule(TheModule, Index, ClearDSOLocalOnDeclarations);
 
     // Apply summary-based prevailing-symbol resolution decisions.
     thinLTOResolvePrevailingInModule(TheModule, DefinedGlobals);
@@ -432,7 +441,8 @@ ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
   saveTempBitcode(TheModule, SaveTempsDir, count, ".2.internalized.bc");
 
   if (!SingleModule) {
-    crossImportIntoModule(TheModule, Index, ModuleMap, ImportList);
+    crossImportIntoModule(TheModule, Index, ModuleMap, ImportList,
+                          ClearDSOLocalOnDeclarations);
 
     // Save temps: after cross-module import.
     saveTempBitcode(TheModule, SaveTempsDir, count, ".3.imported.bc");
@@ -673,7 +683,8 @@ void ThinLTOCodeGenerator::promote(Module &TheModule, ModuleSummaryIndex &Index,
       Index, IsExported(ExportLists, GUIDPreservedSymbols),
       IsPrevailing(PrevailingCopy));
 
-  promoteModule(TheModule, Index);
+  // FIXME Set ClearDSOLocalOnDeclarations.
+  promoteModule(TheModule, Index, /*ClearDSOLocalOnDeclarations=*/false);
 }
 
 /**
@@ -705,7 +716,9 @@ void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
                            ExportLists);
   auto &ImportList = ImportLists[TheModule.getModuleIdentifier()];
 
-  crossImportIntoModule(TheModule, Index, ModuleMap, ImportList);
+  // FIXME Set ClearDSOLocalOnDeclarations.
+  crossImportIntoModule(TheModule, Index, ModuleMap, ImportList,
+                        /*ClearDSOLocalOnDeclarations=*/false);
 }
 
 /**
@@ -832,7 +845,8 @@ void ThinLTOCodeGenerator::internalize(Module &TheModule,
       Index, IsExported(ExportLists, GUIDPreservedSymbols),
       IsPrevailing(PrevailingCopy));
 
-  promoteModule(TheModule, Index);
+  // FIXME Set ClearDSOLocalOnDeclarations.
+  promoteModule(TheModule, Index, /*ClearDSOLocalOnDeclarations=*/false);
 
   // Internalization
   thinLTOResolvePrevailingInModule(

diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -1233,7 +1233,8 @@ Expected<bool> FunctionImporter::importFunctions(
     UpgradeDebugInfo(*SrcModule);
 
     // Link in the specified functions.
-    if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
+    if (renameModuleForThinLTO(*SrcModule, Index, ClearDSOLocalOnDeclarations,
+                               &GlobalsToImport))
       return true;
 
     if (PrintImports) {
@@ -1302,7 +1303,8 @@ static bool doImportingForModule(Module &M) {
 
   // Next we need to promote to global scope and rename any local values that
   // are potentially exported to other modules.
-  if (renameModuleForThinLTO(M, *Index, nullptr)) {
+  if (renameModuleForThinLTO(M, *Index, /*clearDSOOnDeclarations=*/false,
+                             /*GlobalsToImport=*/nullptr)) {
     errs() << "Error renaming module\n";
     return false;
   }
@@ -1311,7 +1313,8 @@ static bool doImportingForModule(Module &M) {
   auto ModuleLoader = [&M](StringRef Identifier) {
     return loadFile(std::string(Identifier), M.getContext());
   };
-  FunctionImporter Importer(*Index, ModuleLoader);
+  FunctionImporter Importer(*Index, ModuleLoader,
+                            /*ClearDSOLocalOnDeclarations=*/false);
   Expected<bool> Result = Importer.importFunctions(M, ImportList);
 
   // FIXME: Probably need to propagate Errors through the pass manager.

diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -212,13 +212,6 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
         }
       }
     }
-    // Check the summaries to see if the symbol gets resolved to a known local
-    // definition.
-    if (VI && VI.isDSOLocal()) {
-      GV.setDSOLocal(true);
-      if (GV.hasDLLImportStorageClass())
-        GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);
-    }
   }
 
   // We should always have a ValueInfo (i.e. GV in index) for definitions when
@@ -280,6 +273,20 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
   } else
     GV.setLinkage(getLinkage(&GV, /* DoPromote */ false));
 
+  // When ClearDSOLocalOnDeclarations is true, clear dso_local if GV is
+  // converted to a declaration, to disable direct access. Don't do this if GV
+  // is implicitly dso_local due to a non-default visibility.
+  if (ClearDSOLocalOnDeclarations && GV.isDeclarationForLinker() &&
+      !GV.isImplicitDSOLocal()) {
+    GV.setDSOLocal(false);
+  } else if (VI && VI.isDSOLocal()) {
+    // If all summaries are dso_local, symbol gets resolved to a known local
+    // definition.
+    GV.setDSOLocal(true);
+    if (GV.hasDLLImportStorageClass())
+      GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);
+  }
+
   // Remove functions imported as available externally defs from comdats,
   // as this is a declaration for the linker, and will be dropped eventually.
   // It is illegal for comdats to contain declarations.
@@ -319,7 +326,9 @@ bool FunctionImportGlobalProcessing::run() {
 }
 
 bool llvm::renameModuleForThinLTO(Module &M, const ModuleSummaryIndex &Index,
+                                  bool ClearDSOLocalOnDeclarations,
                                   SetVector<GlobalValue *> *GlobalsToImport) {
-  FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
+  FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport,
+                                                   ClearDSOLocalOnDeclarations);
   return ThinLTOProcessing.run();
 }
diff --git a/llvm/test/LTO/Resolution/X86/local-def-dllimport.ll b/llvm/test/LTO/Resolution/X86/local-def-dllimport.ll
@@ -13,9 +13,7 @@ target triple = "x86_64-unknown-linux-gnu"
 $g = comdat any
 @g = global i8 42, comdat, !type !0
 
-; CHECK: define
-; CHECK-NOT: dllimport
-; CHECK-SAME: @f
+; CHECK: define available_externally dllimport i8* @f()
 define available_externally dllimport i8* @f() {
   ret i8* @g
 }

diff --git a/llvm/test/ThinLTO/X86/Inputs/index-const-prop-gvref.ll b/llvm/test/ThinLTO/X86/Inputs/index-const-prop-gvref.ll
@@ -1,5 +1,5 @@
 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"
 
-@b = global i32* @a, align 8
-@a = global i32 42, align 4
+@b = dso_local global i32* @a, align 8
+@a = dso_local global i32 42, align 4
diff --git a/llvm/test/ThinLTO/X86/funcimport_alwaysinline.ll b/llvm/test/ThinLTO/X86/funcimport_alwaysinline.ll
@@ -11,7 +11,7 @@
 
 ; foo() being always_inline should be imported irrespective of the
 ; instruction limit
-; CHECK1: define available_externally dso_local void @foo()
+; CHECK1: define available_externally void @foo()
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"

diff --git a/llvm/test/ThinLTO/X86/index-const-prop-alias.ll b/llvm/test/ThinLTO/X86/index-const-prop-alias.ll
@@ -16,15 +16,15 @@
 ; RUN: llvm-dis %t5.1.3.import.bc -o - | FileCheck %s --check-prefix=PRESERVED
 
 ; We currently don't support importing aliases
-; IMPORT:       @g.alias = external dso_local global i32
+; IMPORT:       @g.alias = external global i32
 ; IMPORT-NEXT:  @g = internal global i32 42, align 4 #0
 ; IMPORT:  attributes #0 = { "thinlto-internalize" }
 
 ; CODEGEN:      define dso_local i32 @main
 ; CODEGEN-NEXT:    ret i32 42
 
-; PRESERVED:      @g.alias = external dso_local global i32
-; PRESERVED-NEXT: @g = available_externally dso_local global i32 42, align 4
+; PRESERVED:      @g.alias = external global i32
+; PRESERVED-NEXT: @g = available_externally global i32 42, align 4
 
 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"

diff --git a/llvm/test/ThinLTO/X86/index-const-prop-comdat.ll b/llvm/test/ThinLTO/X86/index-const-prop-comdat.ll
@@ -4,7 +4,7 @@
 ; RUN: llvm-dis %t3.2.3.import.bc -o - | FileCheck %s
 
 ; Comdats are not internalized even if they are read only.
-; CHECK: @g = available_externally dso_local global i32 42 
+; CHECK: @g = available_externally 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"

diff --git a/llvm/test/ThinLTO/X86/index-const-prop-dead.ll b/llvm/test/ThinLTO/X86/index-const-prop-dead.ll
@@ -6,7 +6,7 @@
 
 ; Dead globals are converted to declarations by ThinLTO in dropDeadSymbols
 ; If we try to internalize such we'll get a broken module. 
-; CHECK: @g = external dso_local global i32
+; CHECK: @g = external global i32
 
 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"

diff --git a/llvm/test/ThinLTO/X86/index-const-prop-full-lto.ll b/llvm/test/ThinLTO/X86/index-const-prop-full-lto.ll
@@ -8,7 +8,7 @@
 
 ; All references from functions in full LTO module are not constant.
 ; We cannot internalize @g
-; CHECK: @g = available_externally dso_local global i32 42
+; CHECK: @g = available_externally 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"

diff --git a/llvm/test/ThinLTO/X86/index-const-prop-gvref-pie.ll b/llvm/test/ThinLTO/X86/index-const-prop-gvref-pie.ll
@@ -0,0 +1,28 @@
+;; The same as index-const-prop-gvref.ll, except for PIE.
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/index-const-prop-gvref.ll -o %t2.bc
+; RUN: llvm-lto2 run -save-temps %t2.bc -r=%t2.bc,b,pl -r=%t2.bc,a,pl \
+; RUN:   %t1.bc -r=%t1.bc,main,plx -r=%t1.bc,a, -r=%t1.bc,b, -o %t3
+; RUN: llvm-dis %t3.2.3.import.bc -o - | FileCheck %s --check-prefix=DEST
+
+;; For PIE, keep dso_local for declarations to enable direct access.
+; DEST:      @b = external dso_local global i32*
+; DEST-NEXT: @a = available_externally dso_local global i32 42, align 4
+
+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"
+
+@a = external global i32
+@b = external global i32*
+
+define i32 @main() {
+  %p = load i32*, i32** @b, align 8
+  store i32 33, i32* %p, align 4
+  %v = load i32, i32* @a, align 4
+  ret i32 %v
+}
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 7, !"PIE Level", i32 2}
+!1 = !{i32 7, !"PIC Level", i32 2}