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

[Dmitry Mikulin via Phabricator via llvm-commits]

dmikulin 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;
----------------
davide wrote:
> 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?
3) is also demonstrated by the test case in the PR. I'm working on regression tests for all known cases and will submit them with the new version of the patch.


================
Comment at: lld/ELF/SymbolTable.h:140
   std::unique_ptr<BitcodeCompiler> LTO;
+  llvm::StringMap<bool> RenamedSymbols;
+  bool PostLTO = false;
----------------
davide wrote:
> 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).
OK, thanks. Will fix.


https://reviews.llvm.org/D33621