[ELF] Simplify handling of exportDynamic and isPreemptible

[MaskRay]

Summary

In Writer::includeInDynSym(), exportDynamic is used by a Defined with
protected or default visibility, to record whether it is required to be
exported into .dynsym. It is set when any of the following conditions
hold:

1. There is an interposable symbol from a DSO (Undefined or SharedSymbol with default visibility)
2. If -shared or --export-dynamic is specified, any symbol in an object file/bitcode sets this property, unless suppressed by canBeOmittedFromSymbolTable().
3. --dynamic-list when producing an executable

Bullet 3) does not play well with 1) and 2). When -shared is specified,
exportDynamic of most symbols is true. This makes it incapable to record
--dynamic-list marked symbols. We thus have obscure:

if (!config->shared)
  b->exportDynamic = true;
else if (b->includeInDynsym())
  b->isPreemptible = true;

This patch adds another bit Symbol::inDynamicList to record
3). We can thus simplify handleDynamicList() by unifying the DSO and

executable cases. It also allows us to simplify isPreemptible - now

the field is only used in finalizeSections() and later stages.

[peter.smith]

The code changes look good to me. I've made some suggestions on the name of the flag and some of the comments.

ELF/InputFiles.cpp

1485 ↗

(On Diff #214644)

Post variable naming change, could we use newDefined or newSym to avoid the New (presumably to avoid clash with new).

ELF/Symbols.h

119 ↗

(On Diff #214644)

I've struggled with the name and comment here. I think it is because this is really the union of two separate properties that have a similar effect, with a similar name to a command line option that has a related but different effect. The two separate properties are:

• In an executable, there is a Shared symbol with default visibility that we may preempt or define.
• The symbol is present in a dynamic list.

Separate from this we have --export-dynamic which affects whether all symbols of the appropriate visibility defined in an executable should be put into the dynamic symbol table. The presence of --export-dynamic on the command line does not affect the exportDynamic flag in Symbol even though the name suggests that it should.

The best I can come up with for alternative is something like requiresDynamic or forceDynamic.

requiresDynamic is used by a Defined symbol with protected or default visibility, to record whether a symbol is required to be exported into the .dynsym. This is set when any of the following conditions hold:
- In an executable, when there is an existing Shared symbol with default visibility from an interposable DSO.
- When the symbol is present in the dynamic list file.

ELF/Writer.cpp

1659 ↗

(On Diff #214644)

A small nit, the comment implies that the dynamic list is mandatory.
// In a DSO, if the symbol is in a dynamic list then it specifies whether the symbol is preemptible.

[MaskRay]

Given it more thought. Rather than extracting -shared or --export-dynamic logic outside exportDynamic,
we should instead pull the --dynamic-list information out.

Delete canOmitFromDynSym and add inDynamicList instead.

Added a new test dynamic-list-preempt2.s in rLLD368649 to test some interaction between exportDynamic is isPreemptible.

[MaskRay]

Thanks. These are very valuable suggestions. Changed the way exportDynamic is handled, compared with the first revision. isPreemptible can still be simplified.

[peter.smith]

Thanks for the update, this looks a lot easier to understand. Looks good to me.

[MaskRay]

Add sym->inDynamicList = false; to SymbolTable::insert

Bit fields that may be referenced as ... = old.field in Symbol::replace should be initialized in SymbolTable::insert

lld/trunk/ELF/SymbolTable.cpp

Symbol *SymbolTable::insert(StringRef name) {
   symVector.push_back(sym);
 
   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->scriptDefined = false;
   sym->partition = 1;
   return sym;
 }
 
 Symbol *SymbolTable::addSymbol(const Symbol &newSym) {
   Symbol *sym = symtab->insert(newSym.getName());

 void SymbolTable::handleDynamicList() {
   for (SymbolVersion &ver : config->dynamicList) {
     std::vector<Symbol *> syms;
     if (ver.hasWildcard)
       syms = findAllByVersion(ver);
     else
       syms = findByVersion(ver);
 
-    for (Symbol *b : syms) {
-      if (!config->shared)
-        b->exportDynamic = true;
-      else if (b->includeInDynsym())
-        b->isPreemptible = true;
-    }
+    for (Symbol *sym : syms)
+      sym->inDynamicList = true;
   }
 }
 
 // Set symbol versions to symbols. This function handles patterns
 // containing no wildcard characters.
 void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
                                      StringRef versionName) {
   if (ver.hasWildcard)

lld/trunk/ELF/Symbols.h

public:
   unsigned visibility : 2;
 
   // True if the symbol was used for linking and thus need to be added to the
   // output file's symbol table. This is true for all symbols except for
   // unreferenced DSO symbols, lazy (archive) symbols, and bitcode symbols that
   // are unreferenced except by other bitcode objects.
   unsigned isUsedInRegularObj : 1;
 
-  // If this flag is true and the symbol has protected or default visibility, it
-  // will appear in .dynsym. This flag is set by interposable DSO symbols in
-  // executables, by most symbols in DSOs and executables built with
-  // --export-dynamic, and by dynamic lists.
+  // Used by a Defined symbol with protected or default visibility, to record
+  // whether it is required to be exported into .dynsym. This is set when any of
+  // the following conditions hold:
+  //
+  // - If there is an interposable symbol from a DSO.
+  // - If -shared or --export-dynamic is specified, any symbol in an object
+  //   file/bitcode sets this property, unless suppressed by LTO
+  //   canBeOmittedFromSymbolTable().
   unsigned exportDynamic : 1;
 
+  // True if the symbol is in the --dynamic-list file. A Defined symbol with
+  // protected or default visibility with this property is required to be
+  // exported into .dynsym.
+  unsigned inDynamicList : 1;
+
   // False if LTO shouldn't inline whatever this symbol points to. If a symbol
   // is overwritten after LTO, LTO shouldn't inline the symbol because it
   // doesn't know the final contents of the symbol.
   unsigned canInline : 1;
 
   // Used by Undefined and SharedSymbol to track if there has been at least one
   // undefined reference to the symbol. The binding may change to STB_WEAK if
   // the first undefined reference from a non-shared object is weak.
private:
   inline size_t getSymbolSize() const;
 
 protected:
   Symbol(Kind k, InputFile *file, StringRefZ name, uint8_t binding,
          uint8_t stOther, uint8_t type)
       : file(file), nameData(name.data), nameSize(name.size), binding(binding),
         type(type), stOther(stOther), symbolKind(k), visibility(stOther & 3),
         isUsedInRegularObj(!file || file->kind() == InputFile::ObjKind),
-        exportDynamic(isExportDynamic(k, visibility)), canInline(false),
-        referenced(false), traced(false), needsPltAddr(false), isInIplt(false),
-        gotInIgot(false), isPreemptible(false), used(!config->gcSections),
-        needsTocRestore(false), scriptDefined(false) {}
+        exportDynamic(isExportDynamic(k, visibility)), inDynamicList(false),
+        canInline(false), referenced(false), traced(false), needsPltAddr(false),
+        isInIplt(false), gotInIgot(false), isPreemptible(false),
+        used(!config->gcSections), needsTocRestore(false),
+        scriptDefined(false) {}
 
 public:
   // True the symbol should point to its PLT entry.
   // For SharedSymbol only.
   unsigned needsPltAddr : 1;
 
   // True if this symbol is in the Iplt sub-section of the Plt and the Igot
   // sub-section of the .got.plt or .got.
void Symbol::replace(const Symbol &newSym) {
 
   Symbol old = *this;
   memcpy(this, &newSym, newSym.getSymbolSize());
 
   versionId = old.versionId;
   visibility = old.visibility;
   isUsedInRegularObj = old.isUsedInRegularObj;
   exportDynamic = old.exportDynamic;
+  inDynamicList = old.inDynamicList;
   canInline = old.canInline;
   referenced = old.referenced;
   traced = old.traced;
   isPreemptible = old.isPreemptible;
   scriptDefined = old.scriptDefined;
   partition = old.partition;
 
   // Symbol length is computed lazily. If we already know a symbol length,

lld/trunk/ELF/Symbols.cpp

bool Symbol::includeInDynsym() const {
   if (computeBinding() == STB_LOCAL)
     return false;
 
   // If a PIE binary was not linked against any shared libraries, then we can
   // safely drop weak undef symbols from .dynsym.
   if (isUndefWeak() && config->pie && sharedFiles.empty())
     return false;
 
-  return isUndefined() || isShared() || exportDynamic;
+  return isUndefined() || isShared() || exportDynamic || inDynamicList;
 }
 
 // Print out a log message for --trace-symbol.
 void elf::printTraceSymbol(const Symbol *sym) {
   std::string s;
   if (sym->isUndefined())
     s = ": reference to ";
   else if (sym->isLazy())

lld/trunk/ELF/Writer.cpp

static bool computeIsPreemptible(const Symbol &b) {
   if (b.visibility != STV_DEFAULT)
     return false;
 
   // At this point copy relocations have not been created yet, so any
   // symbol that is not defined locally is preemptible.
   if (!b.isDefined())
     return true;
 
-  // If we have a dynamic list it specifies which local symbols are preemptible.
-  if (config->hasDynamicList)
-    return false;
-
   if (!config->shared)
     return false;
 
+  // If the dynamic list is present, it specifies preemptable symbols in a DSO.
+  if (config->hasDynamicList)
+    return b.inDynamicList;
+
   // -Bsymbolic means that definitions are not preempted.
   if (config->bsymbolic || (config->bsymbolicFunctions && b.isFunc()))
     return false;
   return true;
 }
 
 // Create output section objects and add them to OutputSections.
 template <class ELFT> void Writer<ELFT>::finalizeSections() {
template <class ELFT> void Writer<ELFT>::finalizeSections() {
   }
 
   // This responsible for splitting up .eh_frame section into
   // pieces. The relocation scan uses those pieces, so this has to be
   // earlier.
   for (Partition &part : partitions)
     finalizeSynthetic(part.ehFrame);
 
-  symtab->forEachSymbol([](Symbol *s) {
-    if (!s->isPreemptible)
-      s->isPreemptible = computeIsPreemptible(*s);
-  });
+  symtab->forEachSymbol(
+      [](Symbol *s) { s->isPreemptible = computeIsPreemptible(*s); });
 
   // Scan relocations. This must be done after every symbol is declared so that
   // we can correctly decide if a dynamic relocation is needed.
   if (!config->relocatable) {
     forEachRelSec(scanRelocations<ELFT>);
     reportUndefinedSymbols<ELFT>();
   }