release/22.x: [ELF] Initialize Symbol fields in the constructor instead of via memset (#198129)#198665
Merged
Merged
Conversation
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: llvmbot ChangesBackport 905a88b Requested by: @thesamesam Full diff: https://github.com/llvm/llvm-project/pull/198665.diff 2 Files Affected:
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 176f6937a8d66..7ba2be5d1608f 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1261,7 +1261,6 @@ void ObjFile<ELFT>::initSectionsAndLocalSyms(bool ignoreComdats) {
if (!firstGlobal)
return;
SymbolUnion *locals = makeThreadLocalN<SymbolUnion>(firstGlobal);
- memset(locals, 0, sizeof(SymbolUnion) * firstGlobal);
ArrayRef<Elf_Sym> eSyms = this->getELFSyms<ELFT>();
for (size_t i = 0, end = firstGlobal; i != end; ++i) {
@@ -1297,7 +1296,6 @@ void ObjFile<ELFT>::initSectionsAndLocalSyms(bool ignoreComdats) {
else
new (symbols[i]) Defined(ctx, this, name, STB_LOCAL, eSym.st_other, type,
eSym.st_value, eSym.st_size, sec);
- symbols[i]->partition = 1;
symbols[i]->isUsedInRegularObj = true;
}
}
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 034c8734addb8..23994ccea1ef0 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -102,7 +102,7 @@ class Symbol {
uint8_t symbolKind;
// The partition whose dynamic symbol table contains this symbol's definition.
- uint8_t partition;
+ uint8_t partition = 1;
// True if this symbol is preemptible at load time.
//
@@ -242,8 +242,13 @@ class Symbol {
Symbol(Kind k, InputFile *file, StringRef name, uint8_t binding,
uint8_t stOther, uint8_t type)
: file(file), nameData(name.data()), nameSize(name.size()), type(type),
- binding(binding), stOther(stOther), symbolKind(k), ltoCanOmit(false),
- archSpecificBit(false) {}
+ binding(binding), stOther(stOther), symbolKind(k), isPreemptible(false),
+ isUsedInRegularObj(false), isExported(false), ltoCanOmit(false),
+ traced(false), hasVersionSuffix(false), isInIplt(false),
+ gotInIgot(false), folded(false), archSpecificBit(false),
+ scriptDefined(false), dsoDefined(false), dsoProtected(false),
+ versionScriptAssigned(false), thunkAccessed(false),
+ inDynamicList(false), referenced(false), referencedAfterWrap(false) {}
void overwrite(Symbol &sym, Kind k) const {
if (sym.traced)
@@ -302,12 +307,12 @@ class Symbol {
// Temporary flags used to communicate which symbol entries need PLT and GOT
// entries during postScanRelocations();
- std::atomic<uint16_t> flags;
+ std::atomic<uint16_t> flags = 0;
// A ctx.symAux index used to access GOT/PLT entry indexes. This is allocated
// in postScanRelocations().
- uint32_t auxIdx;
- uint32_t dynsymIndex;
+ uint32_t auxIdx = 0;
+ uint32_t dynsymIndex = 0;
// If `file` is SharedFile (for SharedSymbol or copy-relocated Defined), this
// represents the Verdef index within the input DSO, which will be converted
@@ -315,7 +320,7 @@ class Symbol {
// index (VER_NDX_LOCAL, VER_NDX_GLOBAL, or a named version).
// VER_NDX_LOCAL indicates a defined symbol that has been localized by a
// version script's local: directive or --exclude-libs.
- uint16_t versionId;
+ uint16_t versionId = 0;
LLVM_PREFERRED_TYPE(bool)
uint8_t versionScriptAssigned : 1;
@@ -526,7 +531,6 @@ union SymbolUnion {
template <typename... T> Defined *makeDefined(T &&...args) {
auto *sym = getSpecificAllocSingleton<SymbolUnion>().Allocate();
- memset(sym, 0, sizeof(Symbol));
auto &s = *new (reinterpret_cast<Defined *>(sym)) Defined(std::forward<T>(args)...);
return &s;
}
|
Member
Member
|
LGTM |
nickdesaulniers
approved these changes
May 19, 2026
…et (llvm#198129) `initSectionsAndLocalSyms` and `makeDefined` memset the storage to zero and then placement-new a Symbol-derived object into it. Placement new begins a new object's lifetime. The standard does not seem to guarantee the memset bytes carry into members the constructor leaves uninitialized. lld built by GCC 16 can make Valgrind report reads of Symbol::flags (via getSymSectionIndex during finalizeSections) as uses of uninitialized values (ClangBuiltLinux/linux#2162). This patch reinstates the per-field initialization that commit 7787427 ("[ELF] Avoid redundant assignment to Symbol fields. NFC") had replaced with a bulk memset. (cherry picked from commit 905a88b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport 905a88b
Requested by: @thesamesam