[llvm] 3d708bf - Recommit "[ThinLTO] Add correctness check for RO/WO variable import"
via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 15 05:32:37 PST 2019
Author: evgeny
Date: 2019-11-15T16:13:19+03:00
New Revision: 3d708bf5c2672cae01e5ecb0ed1877e3d56ee451
URL: https://github.com/llvm/llvm-project/commit/3d708bf5c2672cae01e5ecb0ed1877e3d56ee451
DIFF: https://github.com/llvm/llvm-project/commit/3d708bf5c2672cae01e5ecb0ed1877e3d56ee451.diff
LOG: Recommit "[ThinLTO] Add correctness check for RO/WO variable import"
ValueInfo has user-defined 'operator bool' which allows incorrect implicit conversion
to GlobalValue::GUID (which is unsigned long). This causes bugs which are hard to
track and should be removed in future.
Added:
Modified:
llvm/include/llvm/LTO/LTO.h
llvm/include/llvm/Transforms/IPO/FunctionImport.h
llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
llvm/lib/LTO/LTO.cpp
llvm/lib/LTO/ThinLTOCodeGenerator.cpp
llvm/lib/Transforms/IPO/FunctionImport.cpp
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/LTO/LTO.h b/llvm/include/llvm/LTO/LTO.h
index 0a1e3e1d0e42..42c9d5b78c0b 100644
--- a/llvm/include/llvm/LTO/LTO.h
+++ b/llvm/include/llvm/LTO/LTO.h
@@ -59,7 +59,7 @@ void thinLTOResolvePrevailingInIndex(
/// must apply the changes to the Module via thinLTOInternalizeModule.
void thinLTOInternalizeAndPromoteInIndex(
ModuleSummaryIndex &Index,
- function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
+ function_ref<bool(StringRef, ValueInfo)> isExported,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing);
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index bbf270c400af..b4dde7b199ff 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -98,7 +98,7 @@ class FunctionImporter {
using ImportMapTy = StringMap<FunctionsToImportTy>;
/// The set contains an entry for every global value the module exports.
- using ExportSetTy = std::unordered_set<GlobalValue::GUID>;
+ using ExportSetTy = DenseSet<ValueInfo>;
/// A function of this type is used to load modules referenced by the index.
using ModuleLoaderTy =
diff --git a/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h b/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
index 22435e4ed1e5..8af2af7f352f 100644
--- a/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
+++ b/llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
@@ -251,7 +251,7 @@ void runWholeProgramDevirtOnIndex(
/// devirt target names for any locals that were exported.
void updateIndexWPDForExports(
ModuleSummaryIndex &Summary,
- function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
+ function_ref<bool(StringRef, ValueInfo)> isExported,
std::map<ValueInfo, std::vector<VTableSlotSummary>> &LocalWPDTargetsMap);
} // end namespace llvm
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 112858c39af1..85a14b779a78 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -147,9 +147,11 @@ void llvm::computeLTOCacheKey(
// Include the hash for the current module
auto ModHash = Index.getModuleHash(ModuleID);
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
- for (auto F : ExportList)
+ for (const auto &VI : ExportList) {
+ auto GUID = VI.getGUID();
// The export list can impact the internalization, be conservative here
- Hasher.update(ArrayRef<uint8_t>((uint8_t *)&F, sizeof(F)));
+ Hasher.update(ArrayRef<uint8_t>((uint8_t *)&GUID, sizeof(GUID)));
+ }
// Include the hash for every module we import functions from. The set of
// imported symbols for each module may affect code generation and is
@@ -384,12 +386,12 @@ static bool isWeakObjectWithRWAccess(GlobalValueSummary *GVS) {
}
static void thinLTOInternalizeAndPromoteGUID(
- GlobalValueSummaryList &GVSummaryList, GlobalValue::GUID GUID,
- function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
+ GlobalValueSummaryList &GVSummaryList, ValueInfo VI,
+ function_ref<bool(StringRef, ValueInfo)> isExported,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing) {
for (auto &S : GVSummaryList) {
- if (isExported(S->modulePath(), GUID)) {
+ if (isExported(S->modulePath(), VI)) {
if (GlobalValue::isLocalLinkage(S->linkage()))
S->setLinkage(GlobalValue::ExternalLinkage);
} else if (EnableLTOInternalization &&
@@ -397,7 +399,7 @@ static void thinLTOInternalizeAndPromoteGUID(
// doesn't resolve them.
!GlobalValue::isLocalLinkage(S->linkage()) &&
(!GlobalValue::isInterposableLinkage(S->linkage()) ||
- isPrevailing(GUID, S.get())) &&
+ isPrevailing(VI.getGUID(), S.get())) &&
S->linkage() != GlobalValue::AppendingLinkage &&
// We can't internalize available_externally globals because this
// can break function pointer equality.
@@ -416,12 +418,12 @@ static void thinLTOInternalizeAndPromoteGUID(
// as external and non-exported values as internal.
void llvm::thinLTOInternalizeAndPromoteInIndex(
ModuleSummaryIndex &Index,
- function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
+ function_ref<bool(StringRef, ValueInfo)> isExported,
function_ref<bool(GlobalValue::GUID, const GlobalValueSummary *)>
isPrevailing) {
for (auto &I : Index)
- thinLTOInternalizeAndPromoteGUID(I.second.SummaryList, I.first, isExported,
- isPrevailing);
+ thinLTOInternalizeAndPromoteGUID(
+ I.second.SummaryList, Index.getValueInfo(I), isExported, isPrevailing);
}
// Requires a destructor for std::vector<InputModule>.
@@ -1331,11 +1333,10 @@ Error LTO::runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache,
ExportedGUIDs.insert(
GlobalValue::getGUID(GlobalValue::dropLLVMManglingEscape(Def)));
- auto isExported = [&](StringRef ModuleIdentifier, GlobalValue::GUID GUID) {
+ auto isExported = [&](StringRef ModuleIdentifier, ValueInfo VI) {
const auto &ExportList = ExportLists.find(ModuleIdentifier);
- return (ExportList != ExportLists.end() &&
- ExportList->second.count(GUID)) ||
- ExportedGUIDs.count(GUID);
+ return (ExportList != ExportLists.end() && ExportList->second.count(VI)) ||
+ ExportedGUIDs.count(VI.getGUID());
};
// Update local devirtualized targets that were exported by cross-module
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index f6ba7843091b..a9e27832917c 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -590,11 +590,10 @@ struct IsExported {
const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols)
: ExportLists(ExportLists), GUIDPreservedSymbols(GUIDPreservedSymbols) {}
- bool operator()(StringRef ModuleIdentifier, GlobalValue::GUID GUID) const {
+ bool operator()(StringRef ModuleIdentifier, ValueInfo VI) const {
const auto &ExportList = ExportLists.find(ModuleIdentifier);
- return (ExportList != ExportLists.end() &&
- ExportList->second.count(GUID)) ||
- GUIDPreservedSymbols.count(GUID);
+ return (ExportList != ExportLists.end() && ExportList->second.count(VI)) ||
+ GUIDPreservedSymbols.count(VI.getGUID());
}
};
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index b22cf47a797c..6a45bda7dba7 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -308,7 +308,7 @@ static void computeImportForReferencedGlobals(
auto MarkExported = [&](const ValueInfo &VI, const GlobalValueSummary *S) {
if (ExportLists)
- (*ExportLists)[S->modulePath()].insert(VI.getGUID());
+ (*ExportLists)[S->modulePath()].insert(VI);
};
for (auto &RefSummary : VI.getSummaryList())
@@ -497,7 +497,7 @@ static void computeImportForFunction(
// Make exports in the source module.
if (ExportLists) {
auto &ExportList = (*ExportLists)[ExportModulePath];
- ExportList.insert(VI.getGUID());
+ ExportList.insert(VI);
if (!PreviouslyImported) {
// This is the first time this function was exported from its source
// module, so mark all functions and globals it references as exported
@@ -505,14 +505,11 @@ static void computeImportForFunction(
// For efficiency, we unconditionally add all the referenced GUIDs
// to the ExportList for this module, and will prune out any not
// defined in the module later in a single pass.
- for (auto &Edge : ResolvedCalleeSummary->calls()) {
- auto CalleeGUID = Edge.first.getGUID();
- ExportList.insert(CalleeGUID);
- }
- for (auto &Ref : ResolvedCalleeSummary->refs()) {
- auto GUID = Ref.getGUID();
- ExportList.insert(GUID);
- }
+ for (auto &Edge : ResolvedCalleeSummary->calls())
+ ExportList.insert(Edge.first);
+
+ for (auto &Ref : ResolvedCalleeSummary->refs())
+ ExportList.insert(Ref);
}
}
}
@@ -630,6 +627,37 @@ static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index,
}
#endif
+static bool
+checkVariableImport(const ModuleSummaryIndex &Index,
+ StringMap<FunctionImporter::ImportMapTy> &ImportLists,
+ StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
+
+ DenseSet<GlobalValue::GUID> FlattenedImports;
+
+ for (auto &ImportPerModule : ImportLists)
+ for (auto &ExportPerModule : ImportPerModule.second)
+ FlattenedImports.insert(ExportPerModule.second.begin(),
+ ExportPerModule.second.end());
+
+ // Checks that all GUIDs of read/writeonly vars we see in export lists
+ // are also in the import lists. Otherwise we my face linker undefs,
+ // because readonly and writeonly vars are internalized in their
+ // source modules.
+ auto IsReadOrWriteOnlyVar = [&](StringRef ModulePath, const ValueInfo &VI) {
+ auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
+ Index.findSummaryInModule(VI, ModulePath));
+ return GVS && (Index.isReadOnly(GVS) || Index.isWriteOnly(GVS));
+ };
+
+ for (auto &ExportPerModule : ExportLists)
+ for (auto &VI : ExportPerModule.second)
+ if (!FlattenedImports.count(VI.getGUID()) &&
+ IsReadOrWriteOnlyVar(ExportPerModule.first(), VI))
+ return false;
+
+ return true;
+}
+
/// Compute all the import and export for every module using the Index.
void llvm::ComputeCrossModuleImport(
const ModuleSummaryIndex &Index,
@@ -655,13 +683,14 @@ void llvm::ComputeCrossModuleImport(
const auto &DefinedGVSummaries =
ModuleToDefinedGVSummaries.lookup(ELI.first());
for (auto EI = ELI.second.begin(); EI != ELI.second.end();) {
- if (!DefinedGVSummaries.count(*EI))
- EI = ELI.second.erase(EI);
+ if (!DefinedGVSummaries.count(EI->getGUID()))
+ ELI.second.erase(EI++);
else
++EI;
}
}
+ assert(checkVariableImport(Index, ImportLists, ExportLists));
#ifndef NDEBUG
LLVM_DEBUG(dbgs() << "Import/Export lists for " << ImportLists.size()
<< " modules:\n");
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index 6bc86594c779..f4561b303ebe 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -711,7 +711,7 @@ void runWholeProgramDevirtOnIndex(
void updateIndexWPDForExports(
ModuleSummaryIndex &Summary,
- function_ref<bool(StringRef, GlobalValue::GUID)> isExported,
+ function_ref<bool(StringRef, ValueInfo)> isExported,
std::map<ValueInfo, std::vector<VTableSlotSummary>> &LocalWPDTargetsMap) {
for (auto &T : LocalWPDTargetsMap) {
auto &VI = T.first;
@@ -719,7 +719,7 @@ void updateIndexWPDForExports(
assert(VI.getSummaryList().size() == 1 &&
"Devirt of local target has more than one copy");
auto &S = VI.getSummaryList()[0];
- if (!isExported(S->modulePath(), VI.getGUID()))
+ if (!isExported(S->modulePath(), VI))
continue;
// It's been exported by a cross module import.
diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
index 401be6820439..d795a0a20705 100644
--- a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -250,12 +250,12 @@ void FunctionImportGlobalProcessing::processGlobalForThinLTO(GlobalValue &GV) {
// contains summaries from the source modules if they are being imported.
// We might have a non-null VI and get here even in that case if the name
// matches one in this module (e.g. weak or appending linkage).
- auto* GVS = dyn_cast_or_null<GlobalVarSummary>(
- ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
+ auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
+ ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
if (GVS &&
(ImportIndex.isReadOnly(GVS) || ImportIndex.isWriteOnly(GVS))) {
V->addAttribute("thinlto-internalize");
- // Objects referenced by writeonly GV initializer should not be
+ // Objects referenced by writeonly GV initializer should not be
// promoted, because there is no any kind of read access to them
// on behalf of this writeonly GV. To avoid promotion we convert
// GV initializer to 'zeroinitializer'. This effectively drops
More information about the llvm-commits
mailing list