[PATCH] D33621: Fix for -wrap linker option and LTO, PR 33145
Davide Italiano via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 26 21:41:27 PDT 2017
davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.
There are no tests associated with this patch.
================
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;
----------------
Can you explain why you need all three?
================
Comment at: lld/ELF/SymbolTable.cpp:338-340
+ if ((postLTO && B->isBitcode()) &&
+ Binding == STB_WEAK && S->Binding != STB_WEAK)
+ ;
----------------
empty ifs are not really common in llvm.
================
Comment at: lld/ELF/SymbolTable.h:140
std::unique_ptr<BitcodeCompiler> LTO;
+ llvm::StringMap<bool> RenamedSymbols;
+ bool PostLTO = false;
----------------
A `StringMap<bool>` is a little silly.
If you really want a set container, you should use a proper one.
================
Comment at: llvm/include/llvm/LTO/LTO.h:385
+ /// option.
+ unsigned IsRenamed : 1;
};
----------------
I'd say, `LinkerRedefined`.
================
Comment at: llvm/lib/LTO/LTO.cpp:423
Partition == GlobalResolution::RegularLTO);
+ GlobalRes.IsRenamed = Res.IsRenamed;
}
----------------
why do you need this being part of GlobalResolution?
If you do, please add a test showing that you fail switching the linkage earlier.
https://reviews.llvm.org/D33621
More information about the llvm-commits
mailing list