Page MenuHomePhabricator

[llvm-objcopy] Add --discard-all (-x) option
ClosedPublic

Authored by paulsemel on May 1 2018, 4:27 PM.

Details

Summary

This option removes non-global symbols from the binary.
I tried to follow GNU Binutils behavior as much as I could.

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.May 1 2018, 4:27 PM
paulsemel edited the summary of this revision. (Show Details)May 1 2018, 4:31 PM
alexshap added inline comments.May 1 2018, 4:54 PM
tools/llvm-objcopy/llvm-objcopy.cpp
343

const Symbol &Sym

344
return Config.DiscardAll && Sym.Binding == STB_LOCAL &&
           Sym.getShndx() != SHN_UNDEF && Sym.Type != STT_FILE &&
           Sym.Type != STT_SECTION
jakehehrlich added inline comments.May 1 2018, 6:51 PM
tools/llvm-objcopy/Object.cpp
196–197

Can you implement this in terms of removeSymbols?

212

Can you capture ToRemove by value? Capturing by reference just adds a layer of indirection. Also can you use const SymPtr& for the type?

216–217

Size = Symbols.size() * EntrySize

jhenderson added inline comments.May 2 2018, 1:51 AM
test/tools/llvm-objcopy/discard-all.test
3

Could you extend this test to check that -x works the same as --discard-all, please.

26

Please define this symbol in a section.

tools/llvm-objcopy/Opts.td
76

I know this is what gnu objcopy says, but non-global means not STB_GLOBAL to me, but it doesn't affect some other symbol types either (i.e. STB_WEAK, STT_FILE, STT_SECTION). Could you change the help text to match what it actually does, please.

paulsemel updated this revision to Diff 144858.May 2 2018, 5:11 AM
paulsemel marked 6 inline comments as done.

Added various fixes.

tools/llvm-objcopy/Opts.td
76

Yes, I know, this is a really bad help text. But I was out of ideas, as the real behavior of this option is really tricky... Any thoughts about it guys ?
I was thinking of "Remove all but file and section local symbols" , but even for me that sounds really bad to my ears, so..

tools/llvm-objcopy/llvm-objcopy.cpp
344

Actually, as there is other options that will use this removeSymbols function, this choice was made to make it clearer. I was thinking about something like :

if (Config.DiscardAll && Blabla...)
    return true;

if (Config.DiscardLocals && Blabla..)
    return true;

return false;

I personally find it easier to read, even if this is not the most optimized way for doing it. What do you think ?

jhenderson added inline comments.May 2 2018, 5:36 AM
tools/llvm-objcopy/Opts.td
76

How about "remove all local symbols except file and section symbols"? Similar to yours but feels a little less clunky.

paulsemel updated this revision to Diff 144864.May 2 2018, 5:43 AM
paulsemel marked an inline comment as done.

Changed the option help text to make it more descriptive

tools/llvm-objcopy/Opts.td
76

I like this one :)

jhenderson accepted this revision.May 2 2018, 5:46 AM

LGTM, but please wait for feedback from the other reviewers who have commented.

This revision is now accepted and ready to land.May 2 2018, 5:46 AM
jhenderson added inline comments.May 2 2018, 5:47 AM
tools/llvm-objcopy/Opts.td
76

clang-format?

paulsemel added inline comments.May 2 2018, 5:52 AM
tools/llvm-objcopy/Opts.td
76

Hmm.. Just ran it again, and it doesn't seem I missed indent :)

jhenderson added inline comments.May 2 2018, 5:57 AM
tools/llvm-objcopy/Opts.td
76

Oh, wait, didn't realise this was Options.td! Sorry, never mind :)

alexshap added inline comments.May 2 2018, 9:11 AM
tools/llvm-objcopy/llvm-objcopy.cpp
344

i think having

if (expression which evaluates to bool) 
    return true;
return false

is kinda strange because "if" and the second "return" can be eliminated (there is no actual branching here), thus i would prefer the less verbose form, clang-format (don't forget -style=llvm) will format the long expression in a nice way, so it's not a big issue. That's my preference, but i don't really insist.

paulsemel added inline comments.May 2 2018, 9:31 AM
tools/llvm-objcopy/llvm-objcopy.cpp
344

Sure, I understand that we can eliminate the condition, of course. My point was more that I find it "nicer", and, as I mentioned, there will be other conditions for other options, and I truly think that it might be ugly to have only one big return statement.
About the clang-format, I'm pretty sure I am doing it well, I tried again on this part of the code, but there was no changes :)

alexshap accepted this revision.May 2 2018, 10:02 AM
jakehehrlich accepted this revision.May 2 2018, 11:44 AM
paulsemel closed this revision.May 2 2018, 1:23 PM

committed on r331400