[ELF] Use stOther to track visibility

This simplifies SymbolTableSection<ELFT>::writeTo. Add dsoProtected to be used
in canDefineSymbolInExecutable and get the side benefit that the protected DSO
preemption diagnostic is clearer.

lld/ELF/LTO.cpp

@@ -266,7 +266,7 @@ void BitcodeCompiler::add(BitcodeFile &f) {
         (config->exportDynamic || sym->exportDynamic || sym->inDynamicList);
     const auto *dr = dyn_cast<Defined>(sym);
     r.FinalDefinitionInLinkageUnit =
-        (isExec || sym->visibility != STV_DEFAULT) && dr &&
+        (isExec || sym->visibility() != STV_DEFAULT) && dr &&
         // Skip absolute symbols from ELF objects, otherwise PC-rel relocations
         // will be generated by for them, triggering linker errors.
         // Symbol section is always null for bitcode symbols, hence the check

lld/ELF/Relocations.cpp

@@ -719,7 +719,7 @@ static void reportUndefinedSymbol(const UndefinedDiag &undef,
   Undefined &sym = *undef.sym;
 
   auto visibility = [&]() -> std::string {
-    switch (sym.visibility) {
+    switch (sym.visibility()) {
     case STV_INTERNAL:
       return "internal ";
     case STV_HIDDEN:
@@ -831,7 +831,7 @@ static bool maybeReportUndefined(Undefined &sym, InputSectionBase &sec,
   if (sym.isWeak())
     return false;
 
-  bool canBeExternal = !sym.isLocal() && sym.visibility == STV_DEFAULT;
+  bool canBeExternal = !sym.isLocal() && sym.visibility() == STV_DEFAULT;
   if (config->unresolvedSymbols == UnresolvedPolicy::Ignore && canBeExternal)
     return false;
 
@@ -938,9 +938,8 @@ static bool canDefineSymbolInExecutable(Symbol &sym) {
   // If the symbol has default visibility the symbol defined in the
   // executable will preempt it.
   // Note that we want the visibility of the shared symbol itself, not
-  // the visibility of the symbol in the output file we are producing. That is
-  // why we use Sym.stOther.
-  if ((sym.stOther & 0x3) == STV_DEFAULT)
+  // the visibility of the symbol in the output file we are producing.
+  if (!sym.dsoProtected)
     return true;
 
   // If we are allowed to break address equality of functions, defining

lld/ELF/SymbolTable.cpp

@@ -92,7 +92,7 @@ Symbol *SymbolTable::insert(StringRef name) {
   sym->setName(name);
   sym->symbolKind = Symbol::PlaceholderKind;
   sym->partition = 1;
-  sym->visibility = STV_DEFAULT;
+  sym->setVisibility(STV_DEFAULT);
   sym->isUsedInRegularObj = false;
   sym->exportDynamic = false;
   sym->inDynamicList = false;

lld/ELF/Symbols.cpp

@@ -264,8 +264,8 @@ void Symbol::extract() const {
 }
 
 uint8_t Symbol::computeBinding() const {
-  if ((visibility != STV_DEFAULT && visibility != STV_PROTECTED) ||
-      versionId == VER_NDX_LOCAL)
+  auto v = visibility();
+  if ((v != STV_DEFAULT && v != STV_PROTECTED) || versionId == VER_NDX_LOCAL)
     return STB_LOCAL;
   if (binding == STB_GNU_UNIQUE && !config->gnuUnique)
     return STB_GLOBAL;
@@ -344,7 +344,7 @@ bool elf::computeIsPreemptible(const Symbol &sym) {
 
   // Only symbols with default visibility that appear in dynsym can be
   // preempted. Symbols with protected visibility cannot be preempted.
-  if (!sym.includeInDynsym() || sym.visibility != STV_DEFAULT)
+  if (!sym.includeInDynsym() || sym.visibility() != STV_DEFAULT)
     return false;
 
   // At this point copy relocations have not been created yet, so any
@@ -377,10 +377,10 @@ void Symbol::mergeProperties(const Symbol &other) {
     exportDynamic = true;
 
   // DSO symbols do not affect visibility in the output.
-  if (!other.isShared() && other.visibility != STV_DEFAULT)
-    visibility = visibility == STV_DEFAULT
-                     ? other.visibility
-                     : std::min(visibility, other.visibility);
+  if (!other.isShared() && other.visibility() != STV_DEFAULT) {
+    uint8_t v = visibility(), ov = other.visibility();
+    setVisibility(v == STV_DEFAULT ? ov : std::min(v, ov));
+  }
 }
 
 void Symbol::resolve(const Symbol &other) {
@@ -418,7 +418,7 @@ void Symbol::resolveUndefined(const Undefined &other) {
   //
   // If this is a non-weak defined symbol in a discarded section, override the
   // existing undefined symbol for better error message later.
-  if ((isShared() && other.visibility != STV_DEFAULT) ||
+  if ((isShared() && other.visibility() != STV_DEFAULT) ||
       (isUndefined() && other.binding != STB_WEAK && other.discardedSecIdx)) {
     replace(other);
     return;
@@ -666,7 +666,7 @@ void Symbol::resolveShared(const SharedSymbol &other) {
       cast<CommonSymbol>(this)->size = other.size;
     return;
   }
-  if (visibility == STV_DEFAULT && (isUndefined() || isLazy())) {
+  if (visibility() == STV_DEFAULT && (isUndefined() || isLazy())) {
     // An undefined symbol with non default visibility must be satisfied
     // in the same DSO.
     uint8_t bind = binding;

lld/ELF/Symbols.h

@@ -93,10 +93,6 @@ public:
   // The partition whose dynamic symbol table contains this symbol's definition.
   uint8_t partition = 1;
 
-  // Symbol visibility. This is the computed minimum visibility of all
-  // observed non-DSO symbols.
-  uint8_t visibility : 2;
-
   // True if this symbol is preemptible at load time.
   uint8_t isPreemptible : 1;
 
@@ -146,6 +142,13 @@ public:
 
   inline void replace(const Symbol &other);
 
+  // Symbol visibility. This is the computed minimum visibility of all
+  // observed non-DSO symbols.
+  uint8_t visibility() const { return stOther & 3; }
+  void setVisibility(uint8_t visibility) {
+    stOther = (stOther & ~3) | visibility;
+  }
+
   bool includeInDynsym() const;
   uint8_t computeBinding() const;
   bool isGlobal() const { return binding == llvm::ELF::STB_GLOBAL; }
@@ -244,16 +247,15 @@ protected:
   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),
-        visibility(stOther & 3), isPreemptible(false),
+        binding(binding), stOther(stOther), symbolKind(k), isPreemptible(false),
         isUsedInRegularObj(false), used(false), exportDynamic(false),
         inDynamicList(false), referenced(false), referencedAfterWrap(false),
         traced(false), hasVersionSuffix(false), isInIplt(false),
         gotInIgot(false), folded(false), needsTocRestore(false),
-        scriptDefined(false), needsCopy(false), needsGot(false),
-        needsPlt(false), needsTlsDesc(false), needsTlsGd(false),
-        needsTlsGdToIe(false), needsGotDtprel(false), needsTlsIe(false),
-        hasDirectReloc(false) {}
+        scriptDefined(false), dsoProtected(false), needsCopy(false),
+        needsGot(false), needsPlt(false), needsTlsDesc(false),
+        needsTlsGd(false), needsTlsGdToIe(false), needsGotDtprel(false),
+        needsTlsIe(false), hasDirectReloc(false) {}
 
 public:
   // True if this symbol is in the Iplt sub-section of the Plt and the Igot
@@ -277,6 +279,9 @@ public:
   // of the symbol.
   uint8_t scriptDefined : 1;
 
+  // True if defined in a DSO as protected visibility.
+  uint8_t dsoProtected : 1;
+
   // True if this symbol needs a canonical PLT entry, or (during
   // postScanRelocations) a copy relocation.
   uint8_t needsCopy : 1;
@@ -398,6 +403,7 @@ public:
       : Symbol(SharedKind, &file, name, binding, stOther, type), value(value),
         size(size), alignment(alignment) {
     exportDynamic = true;
+    dsoProtected = visibility() == llvm::ELF::STV_PROTECTED;
     // GNU ifunc is a mechanism to allow user-supplied functions to
     // resolve PLT slot values at load-time. This is contrary to the
     // regular symbol resolution scheme in which symbols are resolved just
@@ -527,7 +533,7 @@ void Symbol::replace(const Symbol &other) {
   nameData = old.nameData;
   nameSize = old.nameSize;
   partition = old.partition;
-  visibility = old.visibility;
+  setVisibility(old.visibility());
   isPreemptible = old.isPreemptible;
   isUsedInRegularObj = old.isUsedInRegularObj;
   exportDynamic = old.exportDynamic;

lld/ELF/SyntheticSections.cpp

@@ -2198,16 +2198,7 @@ template <class ELFT> void SymbolTableSection<ELFT>::writeTo(uint8_t *buf) {
     // Set st_name, st_info and st_other.
     eSym->st_name = ent.strTabOffset;
     eSym->setBindingAndType(sym->binding, sym->type);
-    eSym->st_other = sym->visibility;
-
-    // The 3 most significant bits of st_other are used by OpenPOWER ABI.
-    // See getPPC64GlobalEntryToLocalEntryOffset() for more details.
-    if (config->emachine == EM_PPC64)
-      eSym->st_other |= sym->stOther & 0xe0;
-    // The most significant bit of st_other is used by AArch64 ABI for the
-    // variant PCS.
-    else if (config->emachine == EM_AARCH64)
-      eSym->st_other |= sym->stOther & STO_AARCH64_VARIANT_PCS;
+    eSym->st_other = sym->stOther;
 
     if (BssSection *commonSec = getCommonSec(sym)) {
       // When -r is specified, a COMMON symbol is not allocated. Its st_shndx

lld/test/ELF/x86-64-dyn-rel-error5.s

@@ -1,7 +1,7 @@
 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
-# RUN: not ld.lld -pie %t.o -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK,PIE %s
-# RUN: not ld.lld -shared %t.o -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK,SHARED %s
+# RUN: not ld.lld -pie %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: not ld.lld -shared %t.o -o /dev/null 2>&1 | FileCheck %s
 
 ## Check we don't create dynamic relocations in a writable section,
 ## if the number of bits is smaller than the wordsize.
@@ -17,8 +17,7 @@ hidden:
 # CHECK: error: relocation R_X86_64_16 cannot be used against local symbol; recompile with -fPIC
 # CHECK: error: relocation R_X86_64_32 cannot be used against local symbol; recompile with -fPIC
 
-# PIE: error: cannot preempt symbol: hidden
-# SHARED: error: relocation R_X86_64_32 cannot be used against symbol 'hidden'; recompile with -fPIC
+# CHECK: error: relocation R_X86_64_32 cannot be used against symbol 'hidden'; recompile with -fPIC
 
 .data
 .byte local     # R_X86_64_8