Add a addAbsolute static function to Writer.cpp

[arichardson]

Summary

SymbolTable::addAbsolute() was removed in rL344305.
To me is more readable than the lambda named Add and in our out-of-tree
CHERI target we use addAbsolute() in another function.

[kristina]

For what it's worth, I'm in favor, it seems friendlier to out of tree targets, and I agree on the readability remark. Just my two cents in.

[ruiu]

First, let me explain my motivation as to why I made changes to the symbol table in the first place, so that you understand the background of my changes.

One of the most important goals of lld is speed, and we know that hash table lookup is one of the slowest operations in the linker because hash table lookup is not very fast and we have massive number of symbols to be inserted to a hash table. In lld, we strictly do only one symbol table lookup per a symbol, which I believe the minimum number of hash table lookup, to make the linker as fast as possible.

But I noticed a few weeks ago that we do insert the same symbol more than once if we are using --start-lib and --end-lib options. These options give archive file semantics to object files between the options, so that you can avoid using "ar" command (which tend to be slow) to create archive files from object files. Let's call object files between --{start,end}-lib lazy objects. In lld, when we visit lazy objects for the first time, we add lazy symbols for lazy objects as placeholders, to memorize that symbols in lazy objects can be resolvable if we really need them. When the linker knows that it needs lazy objects, it then create real symbols (defined and undefined) to replace lazy symbols. To do that, it looks up the symbol table again with the same set of symbols. That is a violation of our design policy.

So, I started fixing it, and noticed that that's not very easy to do, because all SymbolTable functions takes not a Symbol but a symbol name (which is StringRef) as a key. As long as we are using that interface, we cannot avoid hash table lookups. I also noticed that SymbolTable has too many utility functions that are not really necessary, which makes it hard to make changes to the code. So, I started off with simplifying the class first.

That is the motivation to reduce number of SymbolTable's member functions. My goal was not achieved yet, but I needed to simplify it first.

Back to your change, this change itself seems neutral to me from the readability perspective, but it is *very* likely to "break" again, possibly soon, because this is not really a public API or anything, and I didn't finish my job there yet. Are you fine with that? You can always write the same function in your local patch.

[arichardson]

Thank you very much for the detailed explanation.

I am totally fine with future breakage if you don't mind me committing this.
In addition to fixing our out-of-tree target, this patch really helps my understanding of the function because there is another lambda named Add a few lines down that adds a symbol that might point at the elf header instead of being an absolute symbol. If you'd rather keep it as a local lambda instead of a static function just before, I could also rename it to AddAbsolute

[ruiu]

LGTM

Please commit.

ELF/Writer.cpp

static Defined *addOptionalRegular(StringRef Name, SectionBase *Sec,
   if (!S || S->isDefined())
     return nullptr;
   Symbol *Sym = Symtab->addDefined(Name, StOther, STT_NOTYPE, Val,
                                    /*Size=*/0, Binding, Sec,
                                    /*File=*/nullptr);
   return cast<Defined>(Sym);
 }
 
+static Defined *addAbsolute(StringRef Name) {
+  return cast<Defined>(Symtab->addDefined(Name, STV_HIDDEN, STT_NOTYPE, 0, 0,
+                                          STB_GLOBAL, nullptr, nullptr));
+};
+
 // The linker is expected to define some symbols depending on
 // the linking result. This function defines such symbols.
 void elf::addReservedSymbols() {
   if (Config->EMachine == EM_MIPS) {
-    auto Add = [](StringRef Name) {
-      return cast<Defined>(Symtab->addDefined(Name, STV_HIDDEN, STT_NOTYPE, 0,
-                                              0, STB_GLOBAL, nullptr, nullptr));
-    };
-
     // Define _gp for MIPS. st_value of _gp symbol will be updated by Writer
     // so that it points to an absolute address which by default is relative
     // to GOT. Default offset is 0x7ff0.
     // See "Global Data Symbols" in Chapter 6 in the following document:
     // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-    ElfSym::MipsGp = Add("_gp");
+    ElfSym::MipsGp = addAbsolute("_gp");
 
     // On MIPS O32 ABI, _gp_disp is a magic symbol designates offset between
     // start of function and 'gp' pointer into GOT.
     if (Symtab->find("_gp_disp"))
-      ElfSym::MipsGpDisp = Add("_gp_disp");
+      ElfSym::MipsGpDisp = addAbsolute("_gp_disp");
 
     // The __gnu_local_gp is a magic symbol equal to the current value of 'gp'
     // pointer. This symbol is used in the code generated by .cpload pseudo-op
     // in case of using -mno-shared option.
     // https://sourceware.org/ml/binutils/2004-12/msg00094.html
     if (Symtab->find("__gnu_local_gp"))
-      ElfSym::MipsLocalGp = Add("__gnu_local_gp");
+      ElfSym::MipsLocalGp = addAbsolute("__gnu_local_gp");
   }
 
   // The Power Architecture 64-bit v2 ABI defines a TableOfContents (TOC) which
   // combines the typical ELF GOT with the small data sections. It commonly
   // includes .got .toc .sdata .sbss. The .TOC. symbol replaces both
   // _GLOBAL_OFFSET_TABLE_ and _SDA_BASE_ from the 32-bit ABI. It is used to
   // represent the TOC base which is offset by 0x8000 bytes from the start of
   // the .got section.