[PATCH] D46029: [llvm-objcopy] Implement --redefine-sym option

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 25 01:55:37 PDT 2018


jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/redefine-symbol.test:5
+# RUN: not llvm-objcopy --redefine-sym barbar %t %t2 2>&1 | FileCheck %s --check-prefix=ERROR-MESSAGE
+
+!ELF
----------------
Could we add empty symbol names as a test case here as well, please. Also, multiple instances of the switch in the same command-line. Extra credit for a test-case that uses multiple instances of the same switch and renames the same symbol multiple times.


================
Comment at: tools/llvm-objcopy/Object.cpp:211
 
 void SymbolTableSection::localize(
     std::function<bool(const Symbol &)> ToLocalize) {
----------------
jakehehrlich wrote:
> Can you turn this into a generalized 'foreach symbol' operation that allows you to modify the symbols in the table and use that instead of adding 'renameSymbol'? Both localize and renameSymbol can work in terms of that. That also will support things better going forward. The correction at the end of here is sufficient for any modification to the  symbols since the only invariant that we have to maintain is that local symbols come before non-local symbols.
Not sure if this is exactly what you mean, but I'd love for us not to have to do a separate loop through all the symbols for each operation that manipulates the symbols. By doing both renaming and localizing in the same path, the code will be a bit nicer, in my opinion.


Repository:
  rL LLVM

https://reviews.llvm.org/D46029





More information about the llvm-commits mailing list