Skip to content

Revert "[ELF] Merge verdefIndex into versionId. NFC" #72208 #72484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Nov 16, 2023

Reverts #72208

If a unversioned Defined preempts a versioned DSO definition, the version ID will not be reset.

@llvmbot
Copy link
Member

llvmbot commented Nov 16, 2023

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

Reverts llvm/llvm-project#72208

If a unversioned Defined preempts a versioned DSO definition, the version ID will not be reset.


Full diff: https://github.com/llvm/llvm-project/pull/72484.diff

5 Files Affected:

  • (modified) lld/ELF/InputFiles.cpp (+2-2)
  • (modified) lld/ELF/Relocations.cpp (+1)
  • (modified) lld/ELF/SymbolTable.cpp (+7-5)
  • (modified) lld/ELF/Symbols.h (+5-5)
  • (modified) lld/ELF/SyntheticSections.cpp (+6-4)
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 4b4d7d6db93cd57..8c7f2c8773f2cbc 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1546,7 +1546,7 @@ template <class ELFT> void SharedFile::parse() {
           SharedSymbol{*this, name, sym.getBinding(), sym.st_other,
                        sym.getType(), sym.st_value, sym.st_size, alignment});
       if (s->file == this)
-        s->versionId = ver;
+        s->verdefIndex = ver;
     }
 
     // Also add the symbol with the versioned name to handle undefined symbols
@@ -1563,7 +1563,7 @@ template <class ELFT> void SharedFile::parse() {
         SharedSymbol{*this, saver().save(name), sym.getBinding(), sym.st_other,
                      sym.getType(), sym.st_value, sym.st_size, alignment});
     if (s->file == this)
-      s->versionId = idx;
+      s->verdefIndex = idx;
   }
 }
 
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index d4685ce70ece8cb..6765461805dadb0 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -309,6 +309,7 @@ static void replaceWithDefined(Symbol &sym, SectionBase &sec, uint64_t value,
           size, &sec)
       .overwrite(sym);
 
+  sym.verdefIndex = old.verdefIndex;
   sym.exportDynamic = true;
   sym.isUsedInRegularObj = true;
   // A copy relocated alias may need a GOT entry.
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index b3d97e4de779068..fe7edd5b0eb49aa 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -92,6 +92,7 @@ Symbol *SymbolTable::insert(StringRef name) {
   memset(sym, 0, sizeof(Symbol));
   sym->setName(name);
   sym->partition = 1;
+  sym->verdefIndex = -1;
   sym->versionId = VER_NDX_GLOBAL;
   if (pos != StringRef::npos)
     sym->hasVersionSuffix = true;
@@ -234,9 +235,10 @@ bool SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
         sym->getName().contains('@'))
       continue;
 
-    // If the version has not been assigned, assign versionId to the symbol.
-    if (!sym->versionScriptAssigned) {
-      sym->versionScriptAssigned = true;
+    // If the version has not been assigned, verdefIndex is -1. Use an arbitrary
+    // number (0) to indicate the version has been assigned.
+    if (sym->verdefIndex == uint16_t(-1)) {
+      sym->verdefIndex = 0;
       sym->versionId = versionId;
     }
     if (sym->versionId == versionId)
@@ -254,8 +256,8 @@ void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId,
   // so we set a version to a symbol only if no version has been assigned
   // to the symbol. This behavior is compatible with GNU.
   for (Symbol *sym : findAllByVersion(ver, includeNonDefault))
-    if (!sym->versionScriptAssigned) {
-      sym->versionScriptAssigned = true;
+    if (sym->verdefIndex == uint16_t(-1)) {
+      sym->verdefIndex = 0;
       sym->versionId = versionId;
     }
 }
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 0c79efccb548680..4addb79d1257914 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -313,12 +313,11 @@ class Symbol {
   uint32_t auxIdx;
   uint32_t dynsymIndex;
 
-  // For a Defined symbol, this represents the Verdef index (VER_NDX_LOCAL,
-  // VER_NDX_GLOBAL, or a named version). For a SharedSymbol, this represents
-  // the Verdef index within the input DSO, which will be converted to a Verneed
-  // index in the output.
+  // This field is a index to the symbol's version definition.
+  uint16_t verdefIndex;
+
+  // Version definition index.
   uint16_t versionId;
-  uint8_t versionScriptAssigned : 1;
 
   void setFlags(uint16_t bits) {
     flags.fetch_or(bits, std::memory_order_relaxed);
@@ -358,6 +357,7 @@ class Defined : public Symbol {
   }
   void overwrite(Symbol &sym) const {
     Symbol::overwrite(sym, DefinedKind);
+    sym.verdefIndex = -1;
     auto &s = static_cast<Defined &>(sym);
     s.value = value;
     s.size = size;
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 2b32eb3a0fe3558..0f7ebf9d7ba840b 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -3140,8 +3140,10 @@ bool VersionTableSection::isNeeded() const {
 
 void elf::addVerneed(Symbol *ss) {
   auto &file = cast<SharedFile>(*ss->file);
-  if (ss->versionId == VER_NDX_GLOBAL)
+  if (ss->verdefIndex == VER_NDX_GLOBAL) {
+    ss->versionId = VER_NDX_GLOBAL;
     return;
+  }
 
   if (file.vernauxs.empty())
     file.vernauxs.resize(file.verdefs.size());
@@ -3150,10 +3152,10 @@ void elf::addVerneed(Symbol *ss) {
   // already allocated one. The verdef identifiers cover the range
   // [1..getVerDefNum()]; this causes the vernaux identifiers to start from
   // getVerDefNum()+1.
-  if (file.vernauxs[ss->versionId] == 0)
-    file.vernauxs[ss->versionId] = ++SharedFile::vernauxNum + getVerDefNum();
+  if (file.vernauxs[ss->verdefIndex] == 0)
+    file.vernauxs[ss->verdefIndex] = ++SharedFile::vernauxNum + getVerDefNum();
 
-  ss->versionId = file.vernauxs[ss->versionId];
+  ss->versionId = file.vernauxs[ss->verdefIndex];
 }
 
 template <class ELFT>

@llvmbot
Copy link
Member

llvmbot commented Nov 16, 2023

@llvm/pr-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

Changes

Reverts llvm/llvm-project#72208

If a unversioned Defined preempts a versioned DSO definition, the version ID will not be reset.


Full diff: https://github.com/llvm/llvm-project/pull/72484.diff

5 Files Affected:

  • (modified) lld/ELF/InputFiles.cpp (+2-2)
  • (modified) lld/ELF/Relocations.cpp (+1)
  • (modified) lld/ELF/SymbolTable.cpp (+7-5)
  • (modified) lld/ELF/Symbols.h (+5-5)
  • (modified) lld/ELF/SyntheticSections.cpp (+6-4)
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 4b4d7d6db93cd57..8c7f2c8773f2cbc 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1546,7 +1546,7 @@ template <class ELFT> void SharedFile::parse() {
           SharedSymbol{*this, name, sym.getBinding(), sym.st_other,
                        sym.getType(), sym.st_value, sym.st_size, alignment});
       if (s->file == this)
-        s->versionId = ver;
+        s->verdefIndex = ver;
     }
 
     // Also add the symbol with the versioned name to handle undefined symbols
@@ -1563,7 +1563,7 @@ template <class ELFT> void SharedFile::parse() {
         SharedSymbol{*this, saver().save(name), sym.getBinding(), sym.st_other,
                      sym.getType(), sym.st_value, sym.st_size, alignment});
     if (s->file == this)
-      s->versionId = idx;
+      s->verdefIndex = idx;
   }
 }
 
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index d4685ce70ece8cb..6765461805dadb0 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -309,6 +309,7 @@ static void replaceWithDefined(Symbol &sym, SectionBase &sec, uint64_t value,
           size, &sec)
       .overwrite(sym);
 
+  sym.verdefIndex = old.verdefIndex;
   sym.exportDynamic = true;
   sym.isUsedInRegularObj = true;
   // A copy relocated alias may need a GOT entry.
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index b3d97e4de779068..fe7edd5b0eb49aa 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -92,6 +92,7 @@ Symbol *SymbolTable::insert(StringRef name) {
   memset(sym, 0, sizeof(Symbol));
   sym->setName(name);
   sym->partition = 1;
+  sym->verdefIndex = -1;
   sym->versionId = VER_NDX_GLOBAL;
   if (pos != StringRef::npos)
     sym->hasVersionSuffix = true;
@@ -234,9 +235,10 @@ bool SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
         sym->getName().contains('@'))
       continue;
 
-    // If the version has not been assigned, assign versionId to the symbol.
-    if (!sym->versionScriptAssigned) {
-      sym->versionScriptAssigned = true;
+    // If the version has not been assigned, verdefIndex is -1. Use an arbitrary
+    // number (0) to indicate the version has been assigned.
+    if (sym->verdefIndex == uint16_t(-1)) {
+      sym->verdefIndex = 0;
       sym->versionId = versionId;
     }
     if (sym->versionId == versionId)
@@ -254,8 +256,8 @@ void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId,
   // so we set a version to a symbol only if no version has been assigned
   // to the symbol. This behavior is compatible with GNU.
   for (Symbol *sym : findAllByVersion(ver, includeNonDefault))
-    if (!sym->versionScriptAssigned) {
-      sym->versionScriptAssigned = true;
+    if (sym->verdefIndex == uint16_t(-1)) {
+      sym->verdefIndex = 0;
       sym->versionId = versionId;
     }
 }
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 0c79efccb548680..4addb79d1257914 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -313,12 +313,11 @@ class Symbol {
   uint32_t auxIdx;
   uint32_t dynsymIndex;
 
-  // For a Defined symbol, this represents the Verdef index (VER_NDX_LOCAL,
-  // VER_NDX_GLOBAL, or a named version). For a SharedSymbol, this represents
-  // the Verdef index within the input DSO, which will be converted to a Verneed
-  // index in the output.
+  // This field is a index to the symbol's version definition.
+  uint16_t verdefIndex;
+
+  // Version definition index.
   uint16_t versionId;
-  uint8_t versionScriptAssigned : 1;
 
   void setFlags(uint16_t bits) {
     flags.fetch_or(bits, std::memory_order_relaxed);
@@ -358,6 +357,7 @@ class Defined : public Symbol {
   }
   void overwrite(Symbol &sym) const {
     Symbol::overwrite(sym, DefinedKind);
+    sym.verdefIndex = -1;
     auto &s = static_cast<Defined &>(sym);
     s.value = value;
     s.size = size;
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 2b32eb3a0fe3558..0f7ebf9d7ba840b 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -3140,8 +3140,10 @@ bool VersionTableSection::isNeeded() const {
 
 void elf::addVerneed(Symbol *ss) {
   auto &file = cast<SharedFile>(*ss->file);
-  if (ss->versionId == VER_NDX_GLOBAL)
+  if (ss->verdefIndex == VER_NDX_GLOBAL) {
+    ss->versionId = VER_NDX_GLOBAL;
     return;
+  }
 
   if (file.vernauxs.empty())
     file.vernauxs.resize(file.verdefs.size());
@@ -3150,10 +3152,10 @@ void elf::addVerneed(Symbol *ss) {
   // already allocated one. The verdef identifiers cover the range
   // [1..getVerDefNum()]; this causes the vernaux identifiers to start from
   // getVerDefNum()+1.
-  if (file.vernauxs[ss->versionId] == 0)
-    file.vernauxs[ss->versionId] = ++SharedFile::vernauxNum + getVerDefNum();
+  if (file.vernauxs[ss->verdefIndex] == 0)
+    file.vernauxs[ss->verdefIndex] = ++SharedFile::vernauxNum + getVerDefNum();
 
-  ss->versionId = file.vernauxs[ss->versionId];
+  ss->versionId = file.vernauxs[ss->verdefIndex];
 }
 
 template <class ELFT>

@MaskRay MaskRay merged commit e845754 into main Nov 16, 2023
@MaskRay MaskRay deleted the revert-72208-lld-verdefIndex branch November 16, 2023 07:14
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
…#72484)

Reverts llvm#72208

If a unversioned Defined preempts a versioned DSO definition, the
version ID will not be reset.
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…#72484)

Reverts llvm#72208

If a unversioned Defined preempts a versioned DSO definition, the
version ID will not be reset.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants