[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;
----------------
pcc wrote:
> dmikulin wrote:
> > 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.
> > _wrap_bar is needed for the case where it is defined by the user but may be eliminated during LTO.
> 
> This one only requires `VisibleToRegularObj`, doesn't it?
Looks like it. Do we want to handle _wrap_* symbols specially and not set the weak binding for LTO?


================
Comment at: lld/ELF/SymbolTable.cpp:167
+template <class ELFT> void SymbolTable<ELFT>::addLTOSymbolAlias(StringRef Name) {
+  RenamedSymbols[Name] = true;
+}
----------------
pcc wrote:
> What about the rename target? If I pass `--defsym=foo=bar`, the linker should not allow IPO past `foo`, right?
Right. I'll add it to the list.


================
Comment at: lld/ELF/SymbolTable.cpp:338-340
+      if ((postLTO && B->isBitcode()) &&
+          Binding == STB_WEAK && S->Binding != STB_WEAK)
+        ;
----------------
pcc wrote:
> dmikulin wrote:
> > davide wrote:
> > > empty ifs are not really common in llvm.
> > I'll fix this. Negating the condition seamed counter-intuitive...
> Is the purpose of this code just to restore the binding of the original symbol? I think it would be simpler to have a post-LTO step that manually restores the binding for each renamed symbol rather than dealing with it here.
Yes, it was to restore the binding. I changed it to have a separate pass over renamed symbols after LTO.


https://reviews.llvm.org/D33621