[ELF] Initialize 2 fields of Symbol in SymbolTable::insert

[MaskRay]

Summary

A new symbol is added to elf::symtab in 3 steps:

1. SymbolTable::insert creates a placeholder.
2. Symbol::mergeProperties
3. Symbol::replace

Fields referenced by steps 2) and 3) should be initialized in
SymbolTable::insert. traced and referenced were missed previously.
This did not cause problems because compilers generated code that
initialized them (bit fields) to 0.

[grimar]

LGTM. Please wait for other reviewer(s) opinion about comments (I'd remove them).

ELF/SymbolTable.cpp

75 ↗

(On Diff #214787)

I am not sure that comments of this patch are valuable. You just initializing fields
because they can be used anywhere in LLD. Not sure it is worth to mention where
they are used/copied.

[MaskRay]

Improve comments

[peter.smith]

LGTM too, I agree with George that making the comment specific isn't helpful but a general one isn't doing any harm.

An alternative is to make an initialization function in Symbol and have the default constructor call that for the bitfields. This could be called from SymbolTable::insert. This would have the benefit that we'd only have to update one place for a new field. I notice that canInline is given a different default value from the default constructor, but if this is important it could be a parameter to the initialization function.

A further alternative is to wrap the bitfields in a struct that can be initialized and copied as a whole, but would require every use of one of the fields to be updated.

[MaskRay]

In D66130#1626705, @peter.smith wrote:

LGTM too, I agree with George that making the comment specific isn't helpful but a general one isn't doing any harm.

An alternative is to make an initialization function in Symbol and have the default constructor call that for the bitfields. This could be called from SymbolTable::insert. This would have the benefit that we'd only have to update one place for a new field. I notice that canInline is given a different default value from the default constructor, but if this is important it could be a parameter to the initialization function.

Yes, canInline is weird. The member initializer arbitrarily sets it to false, then SymbolTable::insert changes it to true, in --wrap handling it gets assigned to false (true->false semilattice).

A further alternative is to wrap the bitfields in a struct that can be initialized and copied as a whole, but would require every use of one of the fields to be updated.

The problem is that I can't give the member function a reasonable name :( Conceptually any field referenced when the symbol is still a placeholder should be initialized (these fields are all bit fields currently). Note, there are many other fields that are not referenced - they are not initialized in SymbolTable::insert. Some bit fields are initialized differently in the member initializer list and SymbolTable::insert

• visibility
• isUsedInRegularObj
• canInline
• exportDynamic

So a member function may not save us a lot of trouble.

[MaskRay]

Delete a sentence from a comment

[ruiu]

It might make sense to zero-initialize a new instance with memset(sym, 0, sizeof(Symbol)) before setting any (non-zero/false) values to sym members, as it is less error-prone. What do you think?

[MaskRay]

In D66130#1635001, @ruiu wrote:

It might make sense to zero-initialize a new instance with memset(sym, 0, sizeof(Symbol)) before setting any (non-zero/false) values to sym members, as it is less error-prone. What do you think?

The list documents the fields that may be used while the symbol is still a placeholder. I worry that such auto zero initialization can hide problems that are harder to diagnose.

lld/trunk/ELF/SymbolTable.cpp

Symbol *SymbolTable::insert(StringRef name) {
   bool isNew = p.second;
 
   if (!isNew)
     return symVector[symIndex];
 
   Symbol *sym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
   symVector.push_back(sym);
 
+  // *sym was not initialized by a constructor. Fields that may get referenced
+  // when it is a placeholder must be initialized here.
   sym->setName(name);
   sym->symbolKind = Symbol::PlaceholderKind;
   sym->versionId = VER_NDX_GLOBAL;
   sym->visibility = STV_DEFAULT;
   sym->isUsedInRegularObj = false;
   sym->exportDynamic = false;
   sym->inDynamicList = false;
   sym->canInline = true;
+  sym->referenced = false;
+  sym->traced = false;
   sym->scriptDefined = false;
   sym->partition = 1;
   return sym;
 }
 
 Symbol *SymbolTable::addSymbol(const Symbol &newSym) {
   Symbol *sym = symtab->insert(newSym.getName());
   sym->resolve(newSym);

lld/trunk/ELF/Symbols.h

void Symbol::replace(const Symbol &newSym) {
   if (symbolKind != PlaceholderKind && !isLazy() && !newSym.isLazy() &&
       (type == STT_TLS) != (newSym.type == STT_TLS))
     error("TLS attribute mismatch: " + toString(*this) + "\n>>> defined in " +
           toString(newSym.file) + "\n>>> defined in " + toString(file));
 
   Symbol old = *this;
   memcpy(this, &newSym, newSym.getSymbolSize());
 
+  // old may be a placeholder. The referenced fields must be initialized in
+  // SymbolTable::insert.
   versionId = old.versionId;
   visibility = old.visibility;
   isUsedInRegularObj = old.isUsedInRegularObj;
   exportDynamic = old.exportDynamic;
   inDynamicList = old.inDynamicList;
   canInline = old.canInline;
   referenced = old.referenced;
   traced = old.traced;