Skip to content

Commit d2ef8c1

Browse files
MaskRayMaskRay
committed
[ThinLTO] Drop dso_local if a GlobalVariable satisfies isDeclarationForLinker()
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
1 parent 2f8fb4d commit d2ef8c1

19 files changed

+156
-50
lines changed

‎llvm/include/llvm/Transforms/IPO/FunctionImport.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,10 @@ class FunctionImporter {
105105
std::function<Expected<std::unique_ptr<Module>>(StringRef Identifier)>;
106106

107107
/// Create a Function Importer.
108-
FunctionImporter(const ModuleSummaryIndex &Index, ModuleLoaderTy ModuleLoader)
109-
: Index(Index), ModuleLoader(std::move(ModuleLoader)) {}
108+
FunctionImporter(const ModuleSummaryIndex &Index, ModuleLoaderTy ModuleLoader,
109+
bool ClearDSOLocalOnDeclarations)
110+
: Index(Index), ModuleLoader(std::move(ModuleLoader)),
111+
ClearDSOLocalOnDeclarations(ClearDSOLocalOnDeclarations) {}
110112

111113
/// Import functions in Module \p M based on the supplied import list.
112114
Expected<bool> importFunctions(Module &M, const ImportMapTy &ImportList);
@@ -117,6 +119,10 @@ class FunctionImporter {
117119

118120
/// Factory function to load a Module for a given identifier
119121
ModuleLoaderTy ModuleLoader;
122+
123+
/// See the comment of ClearDSOLocalOnDeclarations in
124+
/// Utils/FunctionImportUtils.h.
125+
bool ClearDSOLocalOnDeclarations;
120126
};
121127

122128
/// The function importing pass

‎llvm/include/llvm/Transforms/Utils/FunctionImportUtils.h

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,19 @@ class FunctionImportGlobalProcessing {
3939
/// as part of a different backend compilation process.
4040
bool HasExportedFunctions = false;
4141

42+
/// Set to true (only applicatable to ELF -fpic) if dso_local should be
43+
/// dropped for a declaration.
44+
///
45+
/// On ELF, the assembler is conservative and assumes a global default
46+
/// visibility symbol can be interposable. No direct access relocation is
47+
/// allowed, if the definition is not in the translation unit, even if the
48+
/// definition is available in the linkage unit. Thus we need to clear
49+
/// dso_local to disable direct access.
50+
///
51+
/// This flag should not be set for -fno-pic or -fpie, which would
52+
/// unnecessarily disable direct access.
53+
bool ClearDSOLocalOnDeclarations;
54+
4255
/// Set of llvm.*used values, in order to validate that we don't try
4356
/// to promote any non-renamable values.
4457
SmallPtrSet<GlobalValue *, 8> Used;
@@ -85,10 +98,11 @@ class FunctionImportGlobalProcessing {
8598
GlobalValue::LinkageTypes getLinkage(const GlobalValue *SGV, bool DoPromote);
8699

87100
public:
88-
FunctionImportGlobalProcessing(
89-
Module &M, const ModuleSummaryIndex &Index,
90-
SetVector<GlobalValue *> *GlobalsToImport = nullptr)
91-
: M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport) {
101+
FunctionImportGlobalProcessing(Module &M, const ModuleSummaryIndex &Index,
102+
SetVector<GlobalValue *> *GlobalsToImport,
103+
bool ClearDSOLocalOnDeclarations)
104+
: M(M), ImportIndex(Index), GlobalsToImport(GlobalsToImport),
105+
ClearDSOLocalOnDeclarations(ClearDSOLocalOnDeclarations) {
92106
// If we have a ModuleSummaryIndex but no function to import,
93107
// then this is the primary module being compiled in a ThinLTO
94108
// backend compilation, and we need to see if it has functions that
@@ -111,6 +125,7 @@ class FunctionImportGlobalProcessing {
111125
/// exported local functions renamed and promoted for ThinLTO.
112126
bool renameModuleForThinLTO(
113127
Module &M, const ModuleSummaryIndex &Index,
128+
bool ClearDSOLocalOnDeclarations,
114129
SetVector<GlobalValue *> *GlobalsToImport = nullptr);
115130

116131
/// Compute synthetic function entry counts.

‎llvm/lib/LTO/LTOBackend.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,13 @@ Error lto::thinBackend(const Config &Conf, unsigned Task, AddStreamFn AddStream,
521521
if (Conf.PreOptModuleHook && !Conf.PreOptModuleHook(Task, Mod))
522522
return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
523523

524-
renameModuleForThinLTO(Mod, CombinedIndex);
524+
// When linking an ELF shared object, dso_local should be dropped. We
525+
// conservatively do this for -fpic.
526+
bool ClearDSOLocalOnDeclarations =
527+
TM->getTargetTriple().isOSBinFormatELF() &&
528+
TM->getRelocationModel() != Reloc::Static &&
529+
Mod.getPIELevel() == PIELevel::Default;
530+
renameModuleForThinLTO(Mod, CombinedIndex, ClearDSOLocalOnDeclarations);
525531

526532
dropDeadSymbols(Mod, DefinedGlobals, CombinedIndex);
527533

@@ -547,7 +553,8 @@ Error lto::thinBackend(const Config &Conf, unsigned Task, AddStreamFn AddStream,
547553
/*IsImporting*/ true);
548554
};
549555

550-
FunctionImporter Importer(CombinedIndex, ModuleLoader);
556+
FunctionImporter Importer(CombinedIndex, ModuleLoader,
557+
ClearDSOLocalOnDeclarations);
551558
if (Error Err = Importer.importFunctions(Mod, ImportList).takeError())
552559
return Err;
553560

‎llvm/lib/LTO/ThinLTOCodeGenerator.cpp

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,9 @@ generateModuleMap(std::vector<std::unique_ptr<lto::InputFile>> &Modules) {
152152
return ModuleMap;
153153
}
154154

155-
static void promoteModule(Module &TheModule, const ModuleSummaryIndex &Index) {
156-
if (renameModuleForThinLTO(TheModule, Index))
155+
static void promoteModule(Module &TheModule, const ModuleSummaryIndex &Index,
156+
bool ClearDSOLocalOnDeclarations) {
157+
if (renameModuleForThinLTO(TheModule, Index, ClearDSOLocalOnDeclarations))
157158
report_fatal_error("renameModuleForThinLTO failed");
158159
}
159160

@@ -205,15 +206,16 @@ static std::unique_ptr<Module> loadModuleFromInput(lto::InputFile *Input,
205206

206207
static void
207208
crossImportIntoModule(Module &TheModule, const ModuleSummaryIndex &Index,
208-
StringMap<lto::InputFile*> &ModuleMap,
209-
const FunctionImporter::ImportMapTy &ImportList) {
209+
StringMap<lto::InputFile *> &ModuleMap,
210+
const FunctionImporter::ImportMapTy &ImportList,
211+
bool ClearDSOLocalOnDeclarations) {
210212
auto Loader = [&](StringRef Identifier) {
211213
auto &Input = ModuleMap[Identifier];
212214
return loadModuleFromInput(Input, TheModule.getContext(),
213215
/*Lazy=*/true, /*IsImporting*/ true);
214216
};
215217

216-
FunctionImporter Importer(Index, Loader);
218+
FunctionImporter Importer(Index, Loader, ClearDSOLocalOnDeclarations);
217219
Expected<bool> Result = Importer.importFunctions(TheModule, ImportList);
218220
if (!Result) {
219221
handleAllErrors(Result.takeError(), [&](ErrorInfoBase &EIB) {
@@ -411,8 +413,15 @@ ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
411413
// "Benchmark"-like optimization: single-source case
412414
bool SingleModule = (ModuleMap.size() == 1);
413415

416+
// When linking an ELF shared object, dso_local should be dropped. We
417+
// conservatively do this for -fpic.
418+
bool ClearDSOLocalOnDeclarations =
419+
TM.getTargetTriple().isOSBinFormatELF() &&
420+
TM.getRelocationModel() != Reloc::Static &&
421+
TheModule.getPIELevel() == PIELevel::Default;
422+
414423
if (!SingleModule) {
415-
promoteModule(TheModule, Index);
424+
promoteModule(TheModule, Index, ClearDSOLocalOnDeclarations);
416425

417426
// Apply summary-based prevailing-symbol resolution decisions.
418427
thinLTOResolvePrevailingInModule(TheModule, DefinedGlobals);
@@ -432,7 +441,8 @@ ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
432441
saveTempBitcode(TheModule, SaveTempsDir, count, ".2.internalized.bc");
433442

434443
if (!SingleModule) {
435-
crossImportIntoModule(TheModule, Index, ModuleMap, ImportList);
444+
crossImportIntoModule(TheModule, Index, ModuleMap, ImportList,
445+
ClearDSOLocalOnDeclarations);
436446

437447
// Save temps: after cross-module import.
438448
saveTempBitcode(TheModule, SaveTempsDir, count, ".3.imported.bc");
@@ -673,7 +683,8 @@ void ThinLTOCodeGenerator::promote(Module &TheModule, ModuleSummaryIndex &Index,
673683
Index, IsExported(ExportLists, GUIDPreservedSymbols),
674684
IsPrevailing(PrevailingCopy));
675685

676-
promoteModule(TheModule, Index);
686+
// FIXME Set ClearDSOLocalOnDeclarations.
687+
promoteModule(TheModule, Index, /*ClearDSOLocalOnDeclarations=*/false);
677688
}
678689

679690
/**
@@ -705,7 +716,9 @@ void ThinLTOCodeGenerator::crossModuleImport(Module &TheModule,
705716
ExportLists);
706717
auto &ImportList = ImportLists[TheModule.getModuleIdentifier()];
707718

708-
crossImportIntoModule(TheModule, Index, ModuleMap, ImportList);
719+
// FIXME Set ClearDSOLocalOnDeclarations.
720+
crossImportIntoModule(TheModule, Index, ModuleMap, ImportList,
721+
/*ClearDSOLocalOnDeclarations=*/false);
709722
}
710723

711724
/**
@@ -832,7 +845,8 @@ void ThinLTOCodeGenerator::internalize(Module &TheModule,
832845
Index, IsExported(ExportLists, GUIDPreservedSymbols),
833846
IsPrevailing(PrevailingCopy));
834847

835-
promoteModule(TheModule, Index);
848+
// FIXME Set ClearDSOLocalOnDeclarations.
849+
promoteModule(TheModule, Index, /*ClearDSOLocalOnDeclarations=*/false);
836850

837851
// Internalization
838852
thinLTOResolvePrevailingInModule(

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1233,7 +1233,8 @@ Expected<bool> FunctionImporter::importFunctions(
12331233
UpgradeDebugInfo(*SrcModule);
12341234

12351235
// Link in the specified functions.
1236-
if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
1236+
if (renameModuleForThinLTO(*SrcModule, Index, ClearDSOLocalOnDeclarations,
1237+
&GlobalsToImport))
12371238
return true;
12381239

12391240
if (PrintImports) {
@@ -1302,7 +1303,8 @@ static bool doImportingForModule(Module &M) {
13021303

13031304
// Next we need to promote to global scope and rename any local values that
13041305
// are potentially exported to other modules.
1305-
if (renameModuleForThinLTO(M, *Index, nullptr)) {
1306+
if (renameModuleForThinLTO(M, *Index, /*clearDSOOnDeclarations=*/false,
1307+
/*GlobalsToImport=*/nullptr)) {
13061308
errs() << "Error renaming module\n";
13071309
return false;
13081310
}
@@ -1311,7 +1313,8 @@ static bool doImportingForModule(Module &M) {
13111313
auto ModuleLoader = [&M](StringRef Identifier) {
13121314
return loadFile(std::string(Identifier), M.getContext());
13131315
};
1314-
FunctionImporter Importer(*Index, ModuleLoader);
1316+
FunctionImporter Importer(*Index, ModuleLoader,
1317+
/*ClearDSOLocalOnDeclarations=*/false);
13151318
Expected<bool> Result = Importer.importFunctions(M, ImportList);
13161319

13171320
// FIXME: Probably need to propagate Errors through the pass manager.

‎llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -212,13 +212,6 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
212212
}
213213
}
214214
}
215-
// Check the summaries to see if the symbol gets resolved to a known local
216-
// definition.
217-
if (VI && VI.isDSOLocal()) {
218-
GV.setDSOLocal(true);
219-
if (GV.hasDLLImportStorageClass())
220-
GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);
221-
}
222215
}
223216

224217
// We should always have a ValueInfo (i.e. GV in index) for definitions when
@@ -280,6 +273,20 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
280273
} else
281274
GV.setLinkage(getLinkage(&GV, /* DoPromote */ false));
282275

276+
// When ClearDSOLocalOnDeclarations is true, clear dso_local if GV is
277+
// converted to a declaration, to disable direct access. Don't do this if GV
278+
// is implicitly dso_local due to a non-default visibility.
279+
if (ClearDSOLocalOnDeclarations && GV.isDeclarationForLinker() &&
280+
!GV.isImplicitDSOLocal()) {
281+
GV.setDSOLocal(false);
282+
} else if (VI && VI.isDSOLocal()) {
283+
// If all summaries are dso_local, symbol gets resolved to a known local
284+
// definition.
285+
GV.setDSOLocal(true);
286+
if (GV.hasDLLImportStorageClass())
287+
GV.setDLLStorageClass(GlobalValue::DefaultStorageClass);
288+
}
289+
283290
// Remove functions imported as available externally defs from comdats,
284291
// as this is a declaration for the linker, and will be dropped eventually.
285292
// It is illegal for comdats to contain declarations.
@@ -319,7 +326,9 @@ bool FunctionImportGlobalProcessing::run() {
319326
}
320327

321328
bool llvm::renameModuleForThinLTO(Module &M, const ModuleSummaryIndex &Index,
329+
bool ClearDSOLocalOnDeclarations,
322330
SetVector<GlobalValue *> *GlobalsToImport) {
323-
FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport);
331+
FunctionImportGlobalProcessing ThinLTOProcessing(M, Index, GlobalsToImport,
332+
ClearDSOLocalOnDeclarations);
324333
return ThinLTOProcessing.run();
325334
}

‎llvm/test/LTO/Resolution/X86/local-def-dllimport.ll

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ target triple = "x86_64-unknown-linux-gnu"
1313
$g = comdat any
1414
@g = global i8 42, comdat, !type !0
1515

16-
; CHECK: define
17-
; CHECK-NOT: dllimport
18-
; CHECK-SAME: @f
16+
; CHECK: define available_externally dllimport i8* @f()
1917
define available_externally dllimport i8* @f() {
2018
ret i8* @g
2119
}

‎llvm/test/ThinLTO/X86/Inputs/index-const-prop-gvref.ll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
22
target triple = "x86_64-unknown-linux-gnu"
33

4-
@b = global i32* @a, align 8
5-
@a = global i32 42, align 4
4+
@b = dso_local global i32* @a, align 8
5+
@a = dso_local global i32 42, align 4

‎llvm/test/ThinLTO/X86/funcimport_alwaysinline.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111

1212
; foo() being always_inline should be imported irrespective of the
1313
; instruction limit
14-
; CHECK1: define available_externally dso_local void @foo()
14+
; CHECK1: define available_externally void @foo()
1515

1616
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
1717
target triple = "x86_64-unknown-linux-gnu"

‎llvm/test/ThinLTO/X86/index-const-prop-alias.ll

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,15 @@
1616
; RUN: llvm-dis %t5.1.3.import.bc -o - | FileCheck %s --check-prefix=PRESERVED
1717

1818
; We currently don't support importing aliases
19-
; IMPORT: @g.alias = external dso_local global i32
19+
; IMPORT: @g.alias = external global i32
2020
; IMPORT-NEXT: @g = internal global i32 42, align 4 #0
2121
; IMPORT: attributes #0 = { "thinlto-internalize" }
2222

2323
; CODEGEN: define dso_local i32 @main
2424
; CODEGEN-NEXT: ret i32 42
2525

26-
; PRESERVED: @g.alias = external dso_local global i32
27-
; PRESERVED-NEXT: @g = available_externally dso_local global i32 42, align 4
26+
; PRESERVED: @g.alias = external global i32
27+
; PRESERVED-NEXT: @g = available_externally global i32 42, align 4
2828

2929
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
3030
target triple = "x86_64-unknown-linux-gnu"

0 commit comments

Comments
 (0)