[llvm] 09b231c - Re-apply "[NFCI][LTO][lld] Optimize away symbol copies within LTO global resolution in ELF" (#107792)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 9 11:17:01 PDT 2024
Author: Mingming Liu
Date: 2024-09-09T11:16:58-07:00
New Revision: 09b231cb38755e1bd122dbab9c57c4847bf64204
URL: https://github.com/llvm/llvm-project/commit/09b231cb38755e1bd122dbab9c57c4847bf64204
DIFF: https://github.com/llvm/llvm-project/commit/09b231cb38755e1bd122dbab9c57c4847bf64204.diff
LOG: Re-apply "[NFCI][LTO][lld] Optimize away symbol copies within LTO global resolution in ELF" (#107792)
Fix the use-after-free bug and re-apply
https://github.com/llvm/llvm-project/pull/106193
* Without the fix, the string referenced by `objSym.Name` could be
destroyed even if string saver keeps a copy of the referenced string.
This caused use-after-free.
* The fix ([latest
commit](https://github.com/llvm/llvm-project/pull/107792/commits/9776ed44cfb26172480145aed8f59ba78a6fa2ea))
updates `objSym.Name` to reference (via `StringRef`) the string saver's
copy.
Test:
1. For `lld/test/ELF/lto/asmundef.ll`, its test failure is reproducible
with `-DLLVM_USE_SANITIZER=Address` and gone with the fix.
3. Run all tests by following
https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild#try-local-changes.
* Without the fix, `ELF/lto/asmundef.ll` aborted the multi-stage test at
`@@@BUILD_STEP stage2/asan_ubsan check@@@`, defined
[here](https://github.com/llvm/llvm-zorg/blob/main/zorg/buildbot/builders/sanitizers/buildbot_fast.sh#L30)
* With the fix, the [multi-stage
test](https://github.com/llvm/llvm-zorg/blob/main/zorg/buildbot/builders/sanitizers/buildbot_fast.sh)
pass stage2 {asan, ubsan, masan}. This is also the test used by
https://lab.llvm.org/buildbot/#/builders/169
**Original commit message**
`StringMap<T>` creates a [copy of the
string](https://github.com/llvm/llvm-project/blob/d4c519e7b2ac21350ec08b23eda44bf4a2d3c974/llvm/include/llvm/ADT/StringMapEntry.h#L55-L58)
for entry insertions and intentionally keep copies [since the
implementation optimizes string memory
usage](https://github.com/llvm/llvm-project/blob/d4c519e7b2ac21350ec08b23eda44bf4a2d3c974/llvm/include/llvm/ADT/StringMap.h#L124).
On the other hand, linker keeps copies of symbol names [1] in
`lld::elf::parseFiles` [2] before invoking `compileBitcodeFiles` [3].
This change proposes to optimize away string copies inside
[LTO::GlobalResolutions](https://github.com/llvm/llvm-project/blob/24e791b4164986a1ca7776e3ae0292ef20d20c47/llvm/include/llvm/LTO/LTO.h#L409),
which will make LTO indexing more memory efficient for ELF. There are
similar opportunities for other (COFF, wasm, MachO) formats.
The optimization takes place for lld (ELF) only. For the rest of use
cases (gold plugin, `llvm-lto2`, etc), LTO owns a string saver to keep
copies and use global resolution key for de-duplication.
Together with @kazutakahirata's work to make `ComputeCrossModuleImport`
more memory efficient, we see a ~20% peak memory usage reduction in a
binary where peak memory usage needs to go down. Thanks to the
optimization in
https://github.com/llvm/llvm-project/commit/329ba523ccbbe68a12434926c92fd9a86494d958,
the max (as opposed to the sum) of `ComputeCrossModuleImport` or
`GlobalResolution` shows up in peak memory usage.
* Regarding correctness, the set of
[resolved](https://github.com/llvm/llvm-project/blob/80c47ad3aec9d7f22e1b1bdc88960a91b66f89f1/llvm/lib/LTO/LTO.cpp#L739)
[per-module
symbols](https://github.com/llvm/llvm-project/blob/80c47ad3aec9d7f22e1b1bdc88960a91b66f89f1/llvm/include/llvm/LTO/LTO.h#L188-L191)
is a subset of
[llvm::lto::InputFile::Symbols](https://github.com/llvm/llvm-project/blob/80c47ad3aec9d7f22e1b1bdc88960a91b66f89f1/llvm/include/llvm/LTO/LTO.h#L120).
And bitcode symbol parsing saves symbol name when iterating
`obj->symbols` in `BitcodeFile::parse` already. This change updates
`BitcodeFile::parseLazy` to keep copies of per-module undefined symbols.
* Presumably the undefined symbols in a LTO unit (copied in this patch
in linker unique saver) is a small set compared with the set of symbols
in global-resolution (copied before this patch), making this a
worthwhile trade-off. Benchmarking this change alone shows measurable
memory savings across various benchmarks.
[1] ELF
https://github.com/llvm/llvm-project/blob/1cea5c2138bef3d8fec75508df6dbb858e6e3560/lld/ELF/InputFiles.cpp#L1748
[2]
https://github.com/llvm/llvm-project/blob/ef7b18a53c0d186dcda1e322be6035407fdedb55/lld/ELF/Driver.cpp#L2863
[3]
https://github.com/llvm/llvm-project/blob/ef7b18a53c0d186dcda1e322be6035407fdedb55/lld/ELF/Driver.cpp#L2995
Added:
Modified:
lld/ELF/InputFiles.cpp
lld/ELF/LTO.cpp
llvm/include/llvm/LTO/Config.h
llvm/include/llvm/LTO/LTO.h
llvm/include/llvm/Object/IRSymtab.h
llvm/lib/LTO/LTO.cpp
Removed:
################################################################################
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 1570adf1370930..db520178f3f534 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1744,10 +1744,15 @@ createBitcodeSymbol(Symbol *&sym, const std::vector<bool> &keptComdats,
uint8_t type = objSym.isTLS() ? STT_TLS : STT_NOTYPE;
uint8_t visibility = mapVisibility(objSym.getVisibility());
- // Symbols can be duplicated in bitcode files because of '#include' and
- // linkonce_odr. Use unique_saver to save symbol names for de-duplication.
- if (!sym)
- sym = symtab.insert(unique_saver().save(objSym.getName()));
+ if (!sym) {
+ // Symbols can be duplicated in bitcode files because of '#include' and
+ // linkonce_odr. Use unique_saver to save symbol names for de-duplication.
+ // Update objSym.Name to reference (via StringRef) the string saver's copy;
+ // this way LTO can reference the same string saver's copy rather than
+ // keeping copies of its own.
+ objSym.Name = unique_saver().save(objSym.getName());
+ sym = symtab.insert(objSym.getName());
+ }
int c = objSym.getComdatIndex();
if (objSym.isUndefined() || (c != -1 && !keptComdats[c])) {
@@ -1797,14 +1802,19 @@ void BitcodeFile::parse() {
void BitcodeFile::parseLazy() {
numSymbols = obj->symbols().size();
symbols = std::make_unique<Symbol *[]>(numSymbols);
- for (auto [i, irSym] : llvm::enumerate(obj->symbols()))
+ for (auto [i, irSym] : llvm::enumerate(obj->symbols())) {
+ // Symbols can be duplicated in bitcode files because of '#include' and
+ // linkonce_odr. Use unique_saver to save symbol names for de-duplication.
+ // Update objSym.Name to reference (via StringRef) the string saver's copy;
+ // this way LTO can reference the same string saver's copy rather than
+ // keeping copies of its own.
+ irSym.Name = unique_saver().save(irSym.getName());
if (!irSym.isUndefined()) {
- // Symbols can be duplicated in bitcode files because of '#include' and
- // linkonce_odr. Use unique_saver to save symbol names for de-duplication.
- auto *sym = symtab.insert(unique_saver().save(irSym.getName()));
+ auto *sym = symtab.insert(irSym.getName());
sym->resolve(LazySymbol{*this});
symbols[i] = sym;
}
+ }
}
void BitcodeFile::postParse() {
diff --git a/lld/ELF/LTO.cpp b/lld/ELF/LTO.cpp
index 935d0a9eab9ee0..f339f1c2c0ec21 100644
--- a/lld/ELF/LTO.cpp
+++ b/lld/ELF/LTO.cpp
@@ -135,6 +135,7 @@ static lto::Config createConfig() {
config->ltoValidateAllVtablesHaveTypeInfos;
c.AllVtablesHaveTypeInfos = ctx.ltoAllVtablesHaveTypeInfos;
c.AlwaysEmitRegularLTOObj = !config->ltoObjPath.empty();
+ c.KeepSymbolNameCopies = false;
for (const llvm::StringRef &name : config->thinLTOModulesToCompile)
c.ThinLTOModulesToCompile.emplace_back(name);
diff --git a/llvm/include/llvm/LTO/Config.h b/llvm/include/llvm/LTO/Config.h
index 482b6e55a19d35..a49cce9f30e20c 100644
--- a/llvm/include/llvm/LTO/Config.h
+++ b/llvm/include/llvm/LTO/Config.h
@@ -88,6 +88,11 @@ struct Config {
/// want to know a priori all possible output files.
bool AlwaysEmitRegularLTOObj = false;
+ /// If true, the LTO instance creates copies of the symbol names for LTO::run.
+ /// The lld linker uses string saver to keep symbol names alive and doesn't
+ /// need to create copies, so it can set this field to false.
+ bool KeepSymbolNameCopies = true;
+
/// Allows non-imported definitions to get the potentially more constraining
/// visibility from the prevailing definition. FromPrevailing is the default
/// because it works for many binary formats. ELF can use the more optimized
diff --git a/llvm/include/llvm/LTO/LTO.h b/llvm/include/llvm/LTO/LTO.h
index 949e80a43f0e88..119f872b26c4ff 100644
--- a/llvm/include/llvm/LTO/LTO.h
+++ b/llvm/include/llvm/LTO/LTO.h
@@ -15,6 +15,9 @@
#ifndef LLVM_LTO_LTO_H
#define LLVM_LTO_LTO_H
+#include <memory>
+
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Bitcode/BitcodeReader.h"
@@ -23,6 +26,7 @@
#include "llvm/Object/IRSymtab.h"
#include "llvm/Support/Caching.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/StringSaver.h"
#include "llvm/Support/thread.h"
#include "llvm/Transforms/IPO/FunctionAttrs.h"
#include "llvm/Transforms/IPO/FunctionImport.h"
@@ -132,9 +136,9 @@ class InputFile {
/// Create an InputFile.
static Expected<std::unique_ptr<InputFile>> create(MemoryBufferRef Object);
- /// The purpose of this class is to only expose the symbol information that an
- /// LTO client should need in order to do symbol resolution.
- class Symbol : irsymtab::Symbol {
+ /// The purpose of this struct is to only expose the symbol information that
+ /// an LTO client should need in order to do symbol resolution.
+ struct Symbol : irsymtab::Symbol {
friend LTO;
public:
@@ -403,10 +407,19 @@ class LTO {
};
};
+ // GlobalResolutionSymbolSaver allocator.
+ std::unique_ptr<llvm::BumpPtrAllocator> Alloc;
+
+ // Symbol saver for global resolution map.
+ std::unique_ptr<llvm::StringSaver> GlobalResolutionSymbolSaver;
+
// Global mapping from mangled symbol names to resolutions.
- // Make this an optional to guard against accessing after it has been reset
+ // Make this an unique_ptr to guard against accessing after it has been reset
// (to reduce memory after we're done with it).
- std::optional<StringMap<GlobalResolution>> GlobalResolutions;
+ std::unique_ptr<llvm::DenseMap<StringRef, GlobalResolution>>
+ GlobalResolutions;
+
+ void releaseGlobalResolutionsMemory();
void addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
ArrayRef<SymbolResolution> Res, unsigned Partition,
diff --git a/llvm/include/llvm/Object/IRSymtab.h b/llvm/include/llvm/Object/IRSymtab.h
index 72a51ffa1022de..4e0013ea767e3e 100644
--- a/llvm/include/llvm/Object/IRSymtab.h
+++ b/llvm/include/llvm/Object/IRSymtab.h
@@ -169,7 +169,8 @@ Error build(ArrayRef<Module *> Mods, SmallVector<char, 0> &Symtab,
/// possibly a storage::Uncommon.
struct Symbol {
// Copied from storage::Symbol.
- StringRef Name, IRName;
+ mutable StringRef Name;
+ StringRef IRName;
int ComdatIndex;
uint32_t Flags;
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 68072563cb33d6..5d9a5cbd18f156 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -77,6 +77,10 @@ cl::opt<bool> EnableLTOInternalization(
"enable-lto-internalization", cl::init(true), cl::Hidden,
cl::desc("Enable global value internalization in LTO"));
+static cl::opt<bool>
+ LTOKeepSymbolCopies("lto-keep-symbol-copies", cl::init(false), cl::Hidden,
+ cl::desc("Keep copies of symbols in LTO indexing"));
+
/// Indicate we are linking with an allocator that supports hot/cold operator
/// new interfaces.
extern cl::opt<bool> SupportsHotColdNew;
@@ -587,8 +591,14 @@ LTO::LTO(Config Conf, ThinBackend Backend,
: Conf(std::move(Conf)),
RegularLTO(ParallelCodeGenParallelismLevel, this->Conf),
ThinLTO(std::move(Backend)),
- GlobalResolutions(std::make_optional<StringMap<GlobalResolution>>()),
- LTOMode(LTOMode) {}
+ GlobalResolutions(
+ std::make_unique<DenseMap<StringRef, GlobalResolution>>()),
+ LTOMode(LTOMode) {
+ if (Conf.KeepSymbolNameCopies || LTOKeepSymbolCopies) {
+ Alloc = std::make_unique<BumpPtrAllocator>();
+ GlobalResolutionSymbolSaver = std::make_unique<llvm::StringSaver>(*Alloc);
+ }
+}
// Requires a destructor for MapVector<BitcodeModule>.
LTO::~LTO() = default;
@@ -606,7 +616,12 @@ void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
assert(ResI != ResE);
SymbolResolution Res = *ResI++;
- auto &GlobalRes = (*GlobalResolutions)[Sym.getName()];
+ StringRef SymbolName = Sym.getName();
+ // Keep copies of symbols if the client of LTO says so.
+ if (GlobalResolutionSymbolSaver && !GlobalResolutions->contains(SymbolName))
+ SymbolName = GlobalResolutionSymbolSaver->save(SymbolName);
+
+ auto &GlobalRes = (*GlobalResolutions)[SymbolName];
GlobalRes.UnnamedAddr &= Sym.isUnnamedAddr();
if (Res.Prevailing) {
assert(!GlobalRes.Prevailing &&
@@ -660,6 +675,14 @@ void LTO::addModuleToGlobalRes(ArrayRef<InputFile::Symbol> Syms,
}
}
+void LTO::releaseGlobalResolutionsMemory() {
+ // Release GlobalResolutions dense-map itself.
+ GlobalResolutions.reset();
+ // Release the string saver memory.
+ GlobalResolutionSymbolSaver.reset();
+ Alloc.reset();
+}
+
static void writeToResolutionFile(raw_ostream &OS, InputFile *Input,
ArrayRef<SymbolResolution> Res) {
StringRef Path = Input->getName();
@@ -1771,7 +1794,7 @@ Error LTO::runThinLTO(AddStreamFn AddStream, FileCache Cache,
// are no further accesses. We specifically want to do this before computing
// cross module importing, which adds to peak memory via the computed import
// and export lists.
- GlobalResolutions.reset();
+ releaseGlobalResolutionsMemory();
if (Conf.OptLevel > 0)
ComputeCrossModuleImport(ThinLTO.CombinedIndex, ModuleToDefinedGVSummaries,
More information about the llvm-commits
mailing list