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 ↗(On Diff #159484)

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
22–24 ↗(On Diff #159484)

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 ↗(On Diff #159484)

to start -> to the start

tools/llvm-objcopy/llvm-objcopy.cpp
215 ↗(On Diff #159484)

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 ↗(On Diff #159484)

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
22–24 ↗(On Diff #159484)

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 ↗(On Diff #159484)

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 ↗(On Diff #159484)

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 ↗(On Diff #159484)

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 ↗(On Diff #159484)

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
33 ↗(On Diff #159738)

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
369 ↗(On Diff #159738)

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.