[LLD][ELF] - Remove dead code from ArchiveFile.

[grimar]

Summary

I believe this is NFC change, but posting on review just in case I am missing something.

Currently, the code uses Seen set to prevent fetching the archive files twice,
but I think it is impossible to face this situation. Because if we have lazy symbols
A1..An from object in archive AR, then after facing undefined symbol A1 in some regular object,
we will fetch() and resolve all symbols and will never call fetch() for symbols from archive AR again.

[ruiu]

I think this change is a bit dangerous to land because this patch removes the safeguard to not pull out the same object file more than once from an archive file. If our symbol resolution logic is as simple as you described, and if consistency between archive file index and actual object files in it is maintained for all input files, this change should be fine. But I think taking that risk is unnecessary. (I vaguely remember that there was a some problem if we don't have this code, but I can't remember it now.)

[grimar]

Honestly, I am a bit upset about that our LLD coding style moved from "we do this because of that" to "we do this just in case, I do not remember why".
I'll try to dig why we did that initially.

[ruiu]

No, that's not really the case. I strongly suspect that if you land this, you'll see some weird behavior in the wild.

[pcc]

The issue is that if two object files depend on one another, we can end up in a situation where SymbolTable::fetchLazy is called recursively multiple times on the same object file, so we need to make sure to do nothing on the second call. Reproducer:

$ cat a.s
.globl a, b
a:
$ cat b.s
.globl a, b
b:
$ as -o a.o a.s
$ as -o b.o b.s
$ ar cru foo.a a.o b.o
$ ld.lld -eb foo.a -m elf_x86_64

With your patch I get:

ld.lld: error: duplicate symbol: b
>>> defined at b.o:(.text+0x0) in archive foo.a
>>> defined at b.o:(.text+0x0) in archive foo.a

[grimar]

In D50174#1189892, @pcc wrote:

The issue is that if two object files depend on one another, we can end up in a situation where SymbolTable::fetchLazy is called recursively multiple times on the same object file, so we need to make sure to do nothing on the second call. Reproducer:

$ cat a.s
.globl a, b
a:
$ cat b.s
.globl a, b
b:
$ as -o a.o a.s
$ as -o b.o b.s
$ ar cru foo.a a.o b.o
$ ld.lld -eb foo.a -m elf_x86_64

With your patch I get:

ld.lld: error: duplicate symbol: b
>>> defined at b.o:(.text+0x0) in archive foo.a
>>> defined at b.o:(.text+0x0) in archive foo.a

Thanks a lot for the reproducer, Peter! I'll check it and add a test to LLD.

[grimar]

Abandoning this patch, the test case for the code was committed in rL339114.

ELF/InputFiles.h

 
 // An ArchiveFile object represents a .a file.
 class ArchiveFile : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<Archive> &&File);
   static bool classof(const InputFile *F) { return F->kind() == ArchiveKind; }
   template <class ELFT> void parse();
 
-  // Pulls out an object file that contains a definition for Sym and
-  // returns it. If the same file was instantiated before, this
-  // function returns a nullptr (so we don't instantiate the same file
-  // more than once.)
+  // Pulls out an object file that contains a definition for Sym.
   InputFile *fetch(const Archive::Symbol &Sym);
 
 private:
   std::unique_ptr<Archive> File;
-  llvm::DenseSet<uint64_t> Seen;
 };
 
 class BitcodeFile : public InputFile {
 public:
   BitcodeFile(MemoryBufferRef M, StringRef ArchiveName,
               uint64_t OffsetInArchive);
   static bool classof(const InputFile *F) { return F->kind() == BitcodeKind; }
   template <class ELFT>

ELF/InputFiles.cpp

 
 // Returns a buffer pointing to a member file containing a given symbol.
 InputFile *ArchiveFile::fetch(const Archive::Symbol &Sym) {
   Archive::Child C =
       CHECK(Sym.getMember(), toString(this) +
                                  ": could not get the member for symbol " +
                                  Sym.getName());
 
-  if (!Seen.insert(C.getChildOffset()).second)
-    return nullptr;
-
   MemoryBufferRef MB =
       CHECK(C.getMemoryBufferRef(),
             toString(this) +
                 ": could not get the buffer for the member defining symbol " +
                 Sym.getName());
 
   if (Tar && C.getParent()->isThin())
     Tar->append(relativeToRoot(CHECK(C.getFullName(), this)), MB.getBuffer());

ELF/SymbolTable.cpp

 template <class ELFT>
 void SymbolTable::addLazyObject(StringRef Name, LazyObjFile &Obj) {
   replaceOrFetchLazy<ELFT, LazyObject>(Name, Obj, [&]() { return Obj.fetch(); },
                                        Name);
 }
 
 template <class ELFT> void SymbolTable::fetchLazy(Symbol *Sym) {
   if (auto *S = dyn_cast<LazyArchive>(Sym)) {
-    if (InputFile *File = S->fetch())
-      addFile<ELFT>(File);
+    addFile<ELFT>(S->fetch());
     return;
   }
 
   auto *S = cast<LazyObject>(Sym);
   if (InputFile *File = cast<LazyObjFile>(S->File)->fetch())
     addFile<ELFT>(File);
 }
 

test/ELF/archive-fetch.s

+# REQUIRES: x86
+
+# RUN: echo '.quad foo' | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %tfoo.o
+# RUN: echo '.quad bar' | llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %tbar.o
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: rm -f %t.a
+# RUN: llvm-ar rcs %t.a %t.o
+
+# Test we can link it.
+# RUN: ld.lld %t.a %tfoo.o %tbar.o -o /dev/null
+
+.global foo
+foo:
+
+.global bar
+bar: