Page MenuHomePhabricator

[llvm-objcopy] Add --prefix-symbols option
ClosedPublic

Authored by paulsemel on Aug 7 2018, 4:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Aug 7 2018, 4:41 AM
paulsemel added inline comments.Aug 7 2018, 4:43 AM
tools/llvm-objcopy/llvm-objcopy.cpp
215

This might not be the correct way to retain the new symbols names (and surely not the correct place to have it anyway), but I couldn't find a smarter way to do it..

jhenderson added inline comments.Aug 7 2018, 8:46 AM
test/tools/llvm-objcopy/prefix-symbols.test
23–25

This is a little weird. Section symbols don't usually have a name, since they effectively have the section's name. Does yaml2obj allow you to create a section symbol for a specific section (and thus end up with st_name == 0)?

tools/llvm-objcopy/ObjcopyOpts.td
107

to start -> to the start

tools/llvm-objcopy/llvm-objcopy.cpp
215

The lazy answer would be to just give ownership of symbol names to the symbol class. I don't know if that's the right thing to do, but it feels a bit better than this to me.

372

There's not much point in newing a std::string into existence. Just create a vector of strings.

paulsemel marked an inline comment as done.Aug 7 2018, 9:27 AM
paulsemel added inline comments.
test/tools/llvm-objcopy/prefix-symbols.test
23–25

Does it really matter ? We want to show that the name is unchanged, and indeed yaml2obj is puting a name for this section symbols, so..

tools/llvm-objcopy/llvm-objcopy.cpp
215

Well, not really, as a symbol shouldn't have anything else than a StringRef (as it is to be immutable). The fact is that we need to have a "new" string in memory, to be referenced by this symbol..

372

No, with vector of strings, the address can change (using vectors, we can't ensure the address of elements will remain the same)

paulsemel updated this revision to Diff 159527.Aug 7 2018, 9:27 AM

Changed option name (to start -> to the start)

jhenderson added inline comments.Aug 7 2018, 9:36 AM
tools/llvm-objcopy/llvm-objcopy.cpp
215

But, symbol names are not immutable (as demonstrated by the fact that we have this option). I don't see what StringRef gives us here, apart from a bit of efficiency for cases when we don't need to change the symbol name.

372

Fair (although pre-allocating the vector capacity would solve this issue too). But this becomes moot if we give ownership of names to the symbols...

paulsemel updated this revision to Diff 159738.Aug 8 2018, 9:32 AM

Give ownership of Symbols names to symbol itself.

jakehehrlich accepted this revision.Aug 8 2018, 10:49 PM

Nice. We need the std::string change for names for several other changes. LGTM.

This revision is now accepted and ready to land.Aug 8 2018, 10:49 PM
jhenderson added inline comments.Aug 9 2018, 8:11 AM
test/tools/llvm-objcopy/prefix-symbols.test
34

You can avoid having two near-identical blocks by using multiple check-prefixes:

# RUN: llvm-readobj -symbols %t2 | FileCheck %s --check-prefixes=COMMON,BASIC
...
# RUN: llvm-readobj -symbols %t3 | FileCheck %s --check-prefixes=COMMON,REDEF

...

# COMMON: Symbols [
# COMMON-NEXT: Symbol {
# ...
# COMMON-NEXT: Symbol {
# BASIC-NEXT: Name: prefixbar
# REDEF-NEXT: Name: prefixbaz
# COMMON-NEXT: Value: 0x0
...
tools/llvm-objcopy/llvm-objcopy.cpp
371

Nit: Use .str() on the result of this expression, so as to do the concatenation as a Twine:

Sym.Name = (Config.SymbolsPrefix + Sym.Name).str();
This revision was automatically updated to reflect the committed changes.