[PATCH] D33621: Fix for -wrap linker option and LTO, PR 33145

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Config.h:107
   std::vector<uint8_t> BuildIdVector;
+  llvm::MapVector<Symbol*, RenamedSymbol> RenamedSymbols;
   bool AllowMultipleDefinition;
----------------
Please run clang-format-diff. It should be formatted as `<Symbol *, RenamedSymbol>`


================
Comment at: lld/ELF/Driver.cpp:973
 
+  // Create wrapped symbols
+  for (auto *Arg : Args.filtered(OPT_wrap))
----------------
I prefer a more concrete comment. `Handle -wrap option`.


================
Comment at: lld/ELF/Driver.cpp:977
+
+  // Create alias symbols
+  for (std::pair<StringRef, StringRef> &Def : getDefsym(Args))
----------------
Ditto. `Handle --defsym=sym=alias option.`


================
Comment at: lld/ELF/SymbolTable.cpp:166
   Symbol *Wrap = addUndefined(Saver.save("__wrap_" + Name));
+  // Tell LTO not to eliminate this symbol
+  Wrap->IsUsedInRegularObj = true;
----------------
Add a blank line before a comment.


================
Comment at: lld/ELF/SymbolTable.cpp:183
   Symbol *AliasSym = addUndefined(Alias);
-  memcpy(AliasSym->Body.buffer, Sym->Body.buffer, sizeof(AliasSym->Body));
+  // Tell LTO not to eliminate this symbol
+  Sym->IsUsedInRegularObj = true;
----------------
Add a blank line before a comment.


================
Comment at: lld/ELF/SymbolTable.cpp:189
+// Apply symbol renames created by -wrap and -defsym
+template <class ELFT> void SymbolTable<ELFT>::applySymbolRenames() {
+  for (auto &RSI : Config->RenamedSymbols) {
----------------
It is not obvious why you need this function from the source code. You need to add a comment.


================
Comment at: lld/ELF/SymbolTable.cpp:190
+template <class ELFT> void SymbolTable<ELFT>::applySymbolRenames() {
+  for (auto &RSI : Config->RenamedSymbols) {
+    Symbol *Sym = RSI.first;
----------------
What is RSI? I found just `KV` (short for key-value) makes sense in most cases.


================
Comment at: lld/ELF/SymbolTable.cpp:194
+    Sym->Binding = RSI.second.OrigBinding;
+    // We rename symbols by replacing the old symbol's SymbolBody with the new
+    // symbol's SymbolBody. This causes all SymbolBody pointers referring to the
----------------
Ditto


https://reviews.llvm.org/D33621