An attempt to fix PR33145, --wrap doesn't work with lto.
Wrapped symbols need to excluded from inter-procedural optimizations to prevent dropping symbols and allow the linker to process re-directs.
Current limitation: doesn't work with ThinLTO.
Details
Diff Detail
Event Timeline
There are no tests associated with this patch.
lld/ELF/SymbolTable.cpp | ||
---|---|---|
167–171 | Can you explain why you need all three? | |
363–365 | empty ifs are not really common in llvm. | |
lld/ELF/SymbolTable.h | ||
140 | A StringMap<bool> is a little silly. | |
llvm/include/llvm/LTO/LTO.h | ||
384 | I'd say, LinkerRedefined. | |
llvm/lib/LTO/LTO.cpp | ||
423 | why do you need this being part of GlobalResolution? |
Do you really need this much change? I expected something smaller than this. I mean, I thought what we need to do is to just pass a list of symbols which LTO should keep.
lld/ELF/SymbolTable.cpp | ||
---|---|---|
167–171 | We need all three if we want to match non-LTO behavior.
| |
363–365 | I'll fix this. Negating the condition seamed counter-intuitive... | |
lld/ELF/SymbolTable.h | ||
140 | 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. | |
llvm/include/llvm/LTO/LTO.h | ||
384 | I'll change this. | |
llvm/lib/LTO/LTO.cpp | ||
423 | You're right, I can do it sooner, in addRegularLTO(). I'll take out the GlobalRes stuff. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
167–171 |
This one only requires VisibleToRegularObj, doesn't it? | |
363–365 | 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. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
175 | What about the rename target? If I pass --defsym=foo=bar, the linker should not allow IPO past foo, right? |
Addressed review comments. Added test cases. Tweaked saved pre-LTO bindings to more accurately reflect the state of the symbol table.
This is getting better.
Please rebase the patch because it didn't apply cleanly.
lld/ELF/SymbolTable.cpp | ||
---|---|---|
131 | Convention says all Variable names should start with capital letter. Also, i is a little vague. | |
llvm/include/llvm/LTO/LTO.h | ||
368–371 | Is this formatted? Doesn't seem so. | |
llvm/lib/LTO/LTO.cpp | ||
421 | If I remove this line, all the tests still pass. | |
441–442 | If I comment out this line, all the tests still pass. | |
547–548 | This needs a comment. |
lld/test/ELF/lto/defsym.ll | ||
---|---|---|
14–15 | These are addresses of PLT entries, right? Can you avoid magic numbers here and make what you are testing for more explicit? I think you can do that by making the symbols hidden and checking that the calls correspond to the correct symbol addresses. Same for wrap-2.ll. | |
llvm/lib/LTO/LTO.cpp | ||
442 | You also need to add support for 'r' to llvm-lto2. Please also add an llvm-lto2 based test that verifies that the linkage has changed. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
194–220 | Can we now replace these functions with something that applies the rename to each symbol in Config->RenamedSymbols? I think it should be possible with this change to how we declare RenamedSymbols. struct RenamedSymbol { uint8_t OrigBinding; Symbol *Target; }; MapVector<Symbol*, RenamedSymbol> RenamedSymbols; |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
194–220 | But __wrap_ symbols are different: they need to be on the list, but they are not being replaced. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
194–220 | They only need to be marked as IsUsedInRegularObj. And that's taken care of for you by addUndefined. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
194–220 | Map it to NULL? |
lld/ELF/Config.h | ||
---|---|---|
102 | This is the only field in Config that has a comment. Not sure how I feel about it, I'll defer it to Rui. | |
lld/ELF/SymbolTable.cpp | ||
194–197 | Is this bit needed? I don't see a test for it. | |
198 | Can this ever be null? If so, you should check for it. | |
lld/ELF/SymbolTable.h | ||
42–43 | Unsorted. | |
llvm/lib/LTO/LTO.cpp | ||
407–410 | This comment is now stale after your change. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
194–220 | I mean that you can just set IsUsedInRegularObj on the __wrap_ symbol before you assign it to Target. Then LTO will keep it alive. There's no need to add it (as a key) to RenamedSymbols then. |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
194–220 | This works fine for -wrap, but not for -defsym. For -wrap, we want to keep the __real_ and the original symbols on the list, so the mappings are: For -defsym, we want to preserve the original symbol, so we would add Orig --> Alias mapping to communicate with LTO. But after we're done, we need to copy Orig --> Alias. I added a flag to the mapping structure to reverse the direction of the copy, so now in the -defsym test the call to bar3() gets inlined and the call to bar2() goes to its alias, bar3. This sounds reasonable to me, just want to make sure it's acceptable. |
lld/ELF/Driver.cpp | ||
---|---|---|
972 | This comment and the one below aren't really accurate any more because this code is used in the non-LTO case as well. | |
lld/ELF/LTO.cpp | ||
139 | I think this can just be Config->RenamedSymbols.count(Sym). | |
lld/ELF/SymbolTable.cpp | ||
167–169 | Move this comment to applySymbolRenames. | |
169 | Is this function necessary? All callers pass a non-null SymbolBody, so they can just query the binding directly. | |
197 | I'd minimise the diff here and rename this variable back. |
This LGTM. I am not a big fan of keeping a copy or the original binding, but it sure saves a lot of plumbing.
Please just wait to see if Davide and Peter have something to add.
LGTM as well
lld/test/ELF/lto/wrap-1.ll | ||
---|---|---|
14 | Please update this comment. I think you also want to test 'r' for __real_bar. |
I'm fine with this once Peter's comments are addressed. I'm not a fan of having to carry around the original binding as well, but I think it's tolerable.
lld/ELF/Config.h | ||
---|---|---|
101 | Please run clang-format-diff. It should be formatted as <Symbol *, RenamedSymbol> | |
lld/ELF/Driver.cpp | ||
972 | I prefer a more concrete comment. Handle -wrap option. | |
976 | Ditto. Handle --defsym=sym=alias option. | |
lld/ELF/SymbolTable.cpp | ||
201 | Add a blank line before a comment. | |
219 | Add a blank line before a comment. | |
225 | It is not obvious why you need this function from the source code. You need to add a comment. | |
226 | What is RSI? I found just KV (short for key-value) makes sense in most cases. | |
230 | Ditto |
lld/ELF/SymbolTable.cpp | ||
---|---|---|
226 | RSI is supposed to be Renamed Symbol Iterator, but KV is ok with me too | |
lld/test/ELF/lto/wrap-1.ll | ||
14 | Resolutions file is driven by input file symbol tables. Since __real_bar is not defined in an object, it does not show up there. I added a definition and a test for the 'r' flag. |
There were some comments pending by Rui. You addressed them, but you still need to wait for an explicit LGTM before committing.
I don't think it's worth reverting this patch, but this is not okay and you should consider it next time.
Please run clang-format-diff. It should be formatted as <Symbol *, RenamedSymbol>