Do not lower the priority of DSO symbols than in-archive symbols

This commit reverts 0612ea4 and
instead solves the original issue with a different approach.

Now, if a hidden symbol was resolved to a DSO symbol, we'll redo
symbol resolution so that the symbol won't be resolved to the same one
next time.

This should fix our build-all buildbot.

src/input-files.cc

@@ -860,27 +860,22 @@ void ObjectFile<E>::parse(Context<E> &ctx) {
 //
 //  1. Strong defined symbol
 //  2. Weak defined symbol
-//  3. Strong defined symbol in an archive
-//  4. Weak Defined symbol in an archive
+//  3. Strong defined symbol in a DSO/archive
+//  4. Weak Defined symbol in a DSO/archive
 //  5. Common symbol
 //  6. Common symbol in an archive
-//  7. Strong defined symbol in a DSO
-//  8. Weak Defined symbol in a DSO
-//  9. Unclaimed (nonexistent) symbol
+//  7. Unclaimed (nonexistent) symbol
 //
 // Ties are broken by file priority.
 template <typename E>
 static u64 get_rank(InputFile<E> *file, const ElfSym<E> &esym, bool is_in_archive) {
   auto get_sym_rank = [&] {
-    if (file->is_dso)
-      return (esym.st_bind == STB_WEAK) ? 8 : 7;
-
     if (esym.is_common()) {
       assert(!file->is_dso);
       return is_in_archive ? 6 : 5;
     }
 
-    if (is_in_archive)
+    if (file->is_dso || is_in_archive)
       return (esym.st_bind == STB_WEAK) ? 4 : 3;
 
     if (esym.st_bind == STB_WEAK)
@@ -894,7 +889,7 @@ static u64 get_rank(InputFile<E> *file, const ElfSym<E> &esym, bool is_in_archiv
 template <typename E>
 static u64 get_rank(const Symbol<E> &sym) {
   if (!sym.file)
-    return 9 << 24;
+    return 7 << 24;
   return get_rank(sym.file, sym.esym(), !sym.file->is_alive);
 }
 
@@ -1360,7 +1355,8 @@ void SharedFile<E>::resolve_symbols(Context<E> &ctx) {
   for (i64 i = 0; i < this->symbols.size(); i++) {
     Symbol<E> &sym = *this->symbols[i];
     const ElfSym<E> &esym = this->elf_syms[i];
-    if (esym.is_undef())
+
+    if (esym.is_undef() || sym.skip_dso)
       continue;
 
     std::scoped_lock lock(sym.mu);

src/input-sections.cc

@@ -324,6 +324,7 @@ bool InputSection<E>::record_undef_error(Context<E> &ctx, const ElfRel<E> &rel)
   // Every ELF file has an absolute local symbol as its first symbol.
   // Referring to that symbol is always valid.
   bool is_undef = esym.is_undef() && !esym.is_weak() && sym.sym_idx;
+
   if (is_undef && sym.esym().is_undef()) {
     if (ctx.arg.unresolved_symbols == UNRESOLVED_ERROR && !sym.is_imported) {
       record();
@@ -335,14 +336,6 @@ bool InputSection<E>::record_undef_error(Context<E> &ctx, const ElfRel<E> &rel)
     }
   }
 
-  // If a protected/hidden undefined symbol is resolved to other .so,
-  // it's handled as if no symbols were found.
-  if (sym.file->is_dso &&
-      (sym.visibility == STV_PROTECTED || sym.visibility == STV_HIDDEN)) {
-    record();
-    return true;
-  }
-
   return false;
 }
 

src/mold.h

@@ -2432,6 +2432,8 @@ class Symbol {
   bool has_copyrel : 1 = false;
   bool is_copyrel_readonly : 1 = false;
 
+  bool skip_dso : 1 = false;
+
   // For --gc-sections
   bool gc_root : 1 = false;
 

src/passes.cc

@@ -263,52 +263,77 @@ void resolve_symbols(Context<E> &ctx) {
   append(files, ctx.objs);
   append(files, ctx.dsos);
 
-  // Call resolve_symbols() to find the most appropriate file for each
-  // symbol. And then mark reachable objects to decide which files to
-  // include into an output.
-  tbb::parallel_for_each(files, [&](InputFile<E> *file) {
-    file->resolve_symbols(ctx);
-  });
+  for (;;) {
+    // Call resolve_symbols() to find the most appropriate file for each
+    // symbol. And then mark reachable objects to decide which files to
+    // include into an output.
+    tbb::parallel_for_each(files, [&](InputFile<E> *file) {
+      file->resolve_symbols(ctx);
+    });
 
-  mark_live_objects(ctx);
+    mark_live_objects(ctx);
 
-  // Now that we know the exact set of input files that are to be
-  // included in the output file, we want to redo symbol resolution.
-  // This is because symbols defined by object files in archive files
-  // may have risen as a result of mark_live_objects().
-  //
-  // To redo symbol resolution, we want to clear the state first.
-  clear_symbols(ctx);
+    // Now that we know the exact set of input files that are to be
+    // included in the output file, we want to redo symbol resolution.
+    // This is because symbols defined by object files in archive files
+    // may have risen as a result of mark_live_objects().
+    //
+    // To redo symbol resolution, we want to clear the state first.
+    clear_symbols(ctx);
 
-  // COMDAT elimination needs to happen exactly here.
-  //
-  // It needs to be after archive extraction, otherwise we might
-  // assign COMDAT leader to an archive member that is not supposed to
-  // be extracted.
-  //
-  // It needs to happen before the final symbol resolution, otherwise
-  // we could eliminate a symbol that is already resolved to and cause
-  // dangling references.
-  tbb::parallel_for_each(ctx.objs, [](ObjectFile<E> *file) {
-    if (file->is_alive)
-      for (ComdatGroupRef<E> &ref : file->comdat_groups)
-        update_minimum(ref.group->owner, file->priority);
-  });
+    // COMDAT elimination needs to happen exactly here.
+    //
+    // It needs to be after archive extraction, otherwise we might
+    // assign COMDAT leader to an archive member that is not supposed to
+    // be extracted.
+    //
+    // It needs to happen before the final symbol resolution, otherwise
+    // we could eliminate a symbol that is already resolved to and cause
+    // dangling references.
+    tbb::parallel_for_each(ctx.objs, [](ObjectFile<E> *file) {
+      if (file->is_alive)
+        for (ComdatGroupRef<E> &ref : file->comdat_groups)
+          update_minimum(ref.group->owner, file->priority);
+    });
 
-  tbb::parallel_for_each(ctx.objs, [](ObjectFile<E> *file) {
-    if (file->is_alive)
-      for (ComdatGroupRef<E> &ref : file->comdat_groups)
-        if (ref.group->owner != file->priority)
-          for (u32 i : ref.members)
-            if (InputSection<E> *isec = file->sections[i].get())
-              isec->is_alive = false;
-  });
+    tbb::parallel_for_each(ctx.objs, [](ObjectFile<E> *file) {
+      if (file->is_alive)
+        for (ComdatGroupRef<E> &ref : file->comdat_groups)
+          if (ref.group->owner != file->priority)
+            for (u32 i : ref.members)
+              if (InputSection<E> *isec = file->sections[i].get())
+                isec->is_alive = false;
+    });
 
-  // Redo symbol resolution
-  tbb::parallel_for_each(files, [&](InputFile<E> *file) {
-    if (file->is_alive)
-      file->resolve_symbols(ctx);
-  });
+    // Redo symbol resolution
+    tbb::parallel_for_each(files, [&](InputFile<E> *file) {
+      if (file->is_alive)
+        file->resolve_symbols(ctx);
+    });
+
+    // Symbols with hidden visibility need to be resolved within the
+    // output file. If as a result of symbol resolution, a hidden symbol
+    // was resolved to a DSO, we'll redo symbol resolution from scratch
+    // with the flag to skip that symbol next time. This should be rare.
+    std::atomic_bool flag = false;
+
+    tbb::parallel_for_each(ctx.dsos, [&](SharedFile<E> *file) {
+      if (file->is_alive) {
+        for (Symbol<E> *sym : file->symbols) {
+          if (sym->file == file && sym->visibility == STV_HIDDEN) {
+            sym->skip_dso = true;
+            flag = true;
+          }
+        }
+      }
+    });
+
+    if (!flag)
+      return;
+
+    clear_symbols(ctx);
+    resolve_symbols(ctx);
+  }
 }
 
 template <typename E>

test/link-order.sh

@@ -16,7 +16,7 @@ int main() {
 EOF
 
 $CC -B. -o $t/exe $t/b.o -Wl,--as-needed $t/libfoo.so $t/libfoo.a
-! readelf --dynamic $t/exe | grep -q libfoo || false
+readelf --dynamic $t/exe | grep -q libfoo
 
 $CC -B. -o $t/exe $t/b.o -Wl,--as-needed $t/libfoo.a $t/libfoo.so
 ! readelf --dynamic $t/exe | grep -q libfoo || false