[ELF] - Refactor lazy symbol duplicated code.

[grimar]

Summary

Our code for LazyObject and LazyArchive duplicates.

This patch extracts the common part to remove
the duplication.

[• espindola]

ELF/SymbolTable.cpp

601 ↗

(On Diff #141984)

You can move this definition to the only use:

replaceOrIgnoreLazy(Name, [&]....)

623 ↗

(On Diff #141984)

Can this be a static helper?

ELF/SymbolTable.h

101 ↗

(On Diff #141984)

Add a comment saying what replaceOrIgnoreLazy does.

[ruiu]

Isn't it more complicated than before? I understand that it is tempting, but generally speaking, I try to *not* abstract things unless doing it is a clear win. I'm a bit worried the direction of your recent "refactoring" patches as it adds more abstractions for a (in my opinion) marginal benefits. Factoring out common code is not always a win in terms of readability, especially when it involves callback functions and such. Simple, repeated code is sometimes better, and the original code was written with that in mind. I really prefer boring code over clever code that uses higher order functions.

[grimar]

• Reimplemented, addressed comments.

What do you think about this version, Rui? Is it still too much?

[• espindola]

ELF/SymbolTable.cpp

608 ↗

(On Diff #142145)

If you change the argument order in the constructors to:

LazyArchive(InputFile &File, uint8_t Type,
            const llvm::object::Archive::Symbol S)

and

LazyObject(InputFile &File, uint8_t Type, StringRef Name)

then it is fairly easy to make replaceOrFetchLazy a variadic template in the same way that replaceSymbol is. That would save one callback argument.

621 ↗

(On Diff #142145)

The implementations of Fetch are not using S. Also, if you template this over ELFT you can write

if (InputFile *File = Fetch())
  Symtab->addFile<ELFT>(File);

And the lambdas are a lot simpler.

[grimar]

• Updated the patch basing on Rafael's comments.

[• espindola]

LGTM

lld/trunk/ELF/SymbolTable.cpp

Symbol *SymbolTable::find(StringRef Name) {
   auto It = SymMap.find(CachedHashStringRef(Name));
   if (It == SymMap.end())
     return nullptr;
   if (It->second == -1)
     return nullptr;
   return SymVector[It->second];
 }
 
-template <class ELFT>
-void SymbolTable::addLazyArchive(StringRef Name, ArchiveFile &F,
-                                 const object::Archive::Symbol Sym) {
+// This is used to handle lazy symbols. May replace existent
+// symbol with lazy version or request to Fetch it.
+template <class ELFT, typename LazyT, typename... ArgT>
+void replaceOrFetchLazy(StringRef Name, InputFile &File,
+                        llvm::function_ref<InputFile *()> Fetch,
+                        ArgT &&... Arg) {
   Symbol *S;
   bool WasInserted;
-  std::tie(S, WasInserted) = insert(Name);
+  std::tie(S, WasInserted) = Symtab->insert(Name);
   if (WasInserted) {
-    replaceSymbol<LazyArchive>(S, F, Sym, Symbol::UnknownType);
+    replaceSymbol<LazyT>(S, File, Symbol::UnknownType,
+                         std::forward<ArgT>(Arg)...);
     return;
   }
   if (!S->isUndefined())
     return;
 
   // An undefined weak will not fetch archive members. See comment on Lazy in
   // Symbols.h for the details.
   if (S->isWeak()) {
-    replaceSymbol<LazyArchive>(S, F, Sym, S->Type);
+    replaceSymbol<LazyT>(S, File, S->Type, std::forward<ArgT>(Arg)...);
     S->Binding = STB_WEAK;
     return;
   }
-  if (InputFile *File = F.fetch(Sym))
-    addFile<ELFT>(File);
+
+  if (InputFile *F = Fetch())
+    Symtab->addFile<ELFT>(F);
 }
 
 template <class ELFT>
-void SymbolTable::addLazyObject(StringRef Name, LazyObjFile &Obj) {
-  Symbol *S;
-  bool WasInserted;
-  std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted) {
-    replaceSymbol<LazyObject>(S, Obj, Name, Symbol::UnknownType);
-    return;
-  }
-  if (!S->isUndefined())
-    return;
+void SymbolTable::addLazyArchive(StringRef Name, ArchiveFile &F,
+                                 const object::Archive::Symbol Sym) {
+  replaceOrFetchLazy<ELFT, LazyArchive>(Name, F, [&]() { return F.fetch(Sym); },
+                                        Sym);
 
-  // See comment for addLazyArchive above.
-  if (S->isWeak()) {
-    replaceSymbol<LazyObject>(S, Obj, Name, S->Type);
-    S->Binding = STB_WEAK;
-    return;
-  }
-  if (InputFile *F = Obj.fetch())
-    addFile<ELFT>(F);
+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);
     return;
   }

lld/trunk/ELF/Symbols.h

 // symbols into consideration.
 
 // This class represents a symbol defined in an archive file. It is
 // created from an archive file header, and it knows how to load an
 // object file from an archive to replace itself with a defined
 // symbol.
 class LazyArchive : public Symbol {
 public:
-  LazyArchive(InputFile &File, const llvm::object::Archive::Symbol S,
-              uint8_t Type)
+  LazyArchive(InputFile &File, uint8_t Type,
+              const llvm::object::Archive::Symbol S)
       : Symbol(LazyArchiveKind, &File, S.getName(), llvm::ELF::STB_GLOBAL,
                llvm::ELF::STV_DEFAULT, Type),
         Sym(S) {}
 
   static bool classof(const Symbol *S) { return S->kind() == LazyArchiveKind; }
 
   InputFile *fetch();
 
 private:
   const llvm::object::Archive::Symbol Sym;
 };
 
 // LazyObject symbols represents symbols in object files between
 // --start-lib and --end-lib options.
 class LazyObject : public Symbol {
 public:
-  LazyObject(InputFile &File, StringRef Name, uint8_t Type)
+  LazyObject(InputFile &File, uint8_t Type, StringRef Name)
       : Symbol(LazyObjectKind, &File, Name, llvm::ELF::STB_GLOBAL,
                llvm::ELF::STV_DEFAULT, Type) {}
 
   static bool classof(const Symbol *S) { return S->kind() == LazyObjectKind; }
 };
 
 // Some linker-generated symbols need to be created as
 // Defined symbols.