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

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lld/ELF/SymbolTable.cpp:159-163
+  Twine WrappedName = "__wrap_" + Name;
+  Twine RealName = "__real_" + Name;
+  RenamedSymbols[WrappedName.str()] = true;
+  RenamedSymbols[RealName.str()] = true;
+  RenamedSymbols[Name] = true;
----------------
dmikulin wrote:
> davide wrote:
> > Can you explain why you need all three?
> We need all three if we want to match non-LTO behavior. 
> *  _wrap_bar is needed for the case where it is defined by the user but may be eliminated during LTO. 
> *  _real_bar is needed for the test case in the PR: the direct call to _real_bar() from foo() will be bound and inlined by LTO to the user-defined _real_bar() instead of bar() if _real_bar() is not on the list.
> *  similarly, if bar() is not on the list, the call to bar() from foo() will be inlined instead of being routed to _wrap_bar().
Can you please add a test showing when 3) happens?


================
Comment at: lld/ELF/SymbolTable.h:140
   std::unique_ptr<BitcodeCompiler> LTO;
+  llvm::StringMap<bool> RenamedSymbols;
+  bool PostLTO = false;
----------------
dmikulin wrote:
> davide wrote:
> > A `StringMap<bool>` is a little silly.
> > If you really want a set container, you should use a proper one.
> Is a set container like SmallSet good enough in this case? What's the worst case for the number of -wrap functions? I picked a map because we have to do a lookup in this container for every symbol.
What I'm trying to say is that a map from something to bool is just a Set.
If you think your set will grow a lot (unlikely, but OK, I'd recommend DenseSet).


https://reviews.llvm.org/D33621