[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

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

ELF/Symbols.h

119

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

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

ELF/InputFiles.cpp

static Symbol *createBitcodeSymbol(const std::vector<bool> &keptComdats,
                                    BitcodeFile &f) {
   StringRef name = saver.save(objSym.getName());
   uint8_t binding = objSym.isWeak() ? STB_WEAK : STB_GLOBAL;
   uint8_t type = objSym.isTLS() ? STT_TLS : STT_NOTYPE;
   uint8_t visibility = mapVisibility(objSym.getVisibility());
   bool canOmitFromDynSym = objSym.canBeOmittedFromSymbolTable();
 
   int c = objSym.getComdatIndex();
-  if (objSym.isUndefined() || (c != -1 && !keptComdats[c])) {
-    Undefined New(&f, name, binding, visibility, type);
-    if (canOmitFromDynSym)
-      New.exportDynamic = false;
-    return symtab->addSymbol(New);
-  }
+  if (objSym.isUndefined() || (c != -1 && !keptComdats[c]))
+    return symtab->addSymbol(Undefined{&f, name, binding, visibility, type});
 
   if (objSym.isCommon())
     return symtab->addSymbol(
         CommonSymbol{&f, name, binding, visibility, STT_OBJECT,
                      objSym.getCommonAlignment(), objSym.getCommonSize()});
 
   Defined New(&f, name, binding, visibility, type, 0, 0, nullptr);
-  if (canOmitFromDynSym)
-    New.exportDynamic = false;
+  New.canOmitFromDynSym = canOmitFromDynSym;
   return symtab->addSymbol(New);
 }
 
 template <class ELFT> void BitcodeFile::parse() {
   std::vector<bool> keptComdats;
   for (StringRef s : obj->getComdatTable())
     keptComdats.push_back(
         symtab->comdatGroups.try_emplace(CachedHashStringRef(s), this).second);

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->canOmitFromDynSym = false;
   sym->canInline = true;
   sym->scriptDefined = false;
   sym->partition = 1;
   return sym;
 }
 
 Symbol *SymbolTable::addSymbol(const Symbol &New) {
   Symbol *sym = symtab->insert(New.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 *b : syms)
   }
 }
 
 // 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)

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 Defined with protected or default visibility to track if it should
+  // appear in .dynsym. This flag is set by interposable DSO symbols (symbols
+  // with default visibility), and by dynamic lists.
   unsigned exportDynamic : 1;
 
+  // Stores the value of bitcode canBeOmittedFromSymbolTable(). Used as an
+  // optimization to omit some .dynsym entries.
+  unsigned canOmitFromDynSym : 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.
public:
   void resolve(const Symbol &other);
 
   // If this is a lazy symbol, fetch an input file and add the symbol
   // in the file to the symbol table. Calling this function on
   // non-lazy object causes a runtime error.
   void fetch() const;
 
 private:
-  static bool isExportDynamic(Kind k, uint8_t visibility) {
-    if (k == SharedKind)
-      return visibility == llvm::ELF::STV_DEFAULT;
-    return config->shared || config->exportDynamic;
-  }
-
   void resolveUndefined(const Undefined &other);
   void resolveCommon(const CommonSymbol &other);
   void resolveDefined(const Defined &other);
   template <class LazyT> void resolveLazy(const LazyT &other);
   void resolveShared(const SharedSymbol &other);
 
   int compare(const Symbol *other) const;
 
   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(k == SharedKind && visibility == llvm::ELF::STV_DEFAULT),
+        canOmitFromDynSym(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.

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;
+  if (isUndefined() || isShared() || exportDynamic)
+    return true;
+
+  // -shared and --export-dynamic default to export defined symbols. As an
+  // optimization, it can be omitted if lto canBeOmittedFromSymbolTable() is
+  // true.
+  return (config->shared || config->exportDynamic) && !canOmitFromDynSym;
 }
 
 // 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())

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;
 
+  // In a DSO, the dynamic list specifies preemptible symbols.
+  if (config->hasDynamicList)
+    return b.exportDynamic;
+
   // -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>();
   }