Page MenuHomePhabricator

[llvm-objcopy] Add --keep-file-symbols option
ClosedPublic

Authored by paulsemel on May 14 2018, 6:38 AM.

Details

Summary

This option prevent from removing file symbols while removing symbols.
This patch relies on this one (not merged yet) : https://reviews.llvm.org/D46819

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.May 14 2018, 6:38 AM

Other than explicitly stripping the file symbol, is there any way of actually removing such symbols currently? It seems like adding a switch just to prevent you doing something you asked for is a bit superfluous.

Do you have a specific use-case at the moment?

test/tools/llvm-objcopy/keep-file-symbols-remove-section.test
1 ↗(On Diff #146592)

I don't think you need this test, since the file symbol is by definition not in a section.

Other than explicitly stripping the file symbol, is there any way of actually removing such symbols currently? It seems like adding a switch just to prevent you doing something you asked for is a bit superfluous.

Do you have a specific use-case at the moment?

Well, --strip-unneeded can trigger the file symbols removal. Anyway, for the moment, I stopped implementing it because I have some problems with the error messages we are issuing for the symbol removal.

In all cases, I truly think that this way is the way to prevent from file symbol removal (particularly for the --strip-unneeded option). Maybe I can just remove the section removal test and land it like this, waiting for someone (maybe me) to implement --strip-unneeded and to add a better test case ?

What do you think about it ?

paulsemel updated this revision to Diff 146637.May 14 2018, 9:53 AM

Applied James' suggestions

If it isn't too much trouble I'd like to wait until --strip-unneeded is implemented.

paulsemel updated this revision to Diff 148490.May 24 2018, 2:27 PM

Thanks to @alexshap changes, I think this patch now makes sense !

Code change looks fine, but could you add a test to show that --keep-file-symbols trumps --strip-symbol, assuming that is the desired behaviour (double-check against GNU, I suggest here).

paulsemel updated this revision to Diff 148598.May 25 2018, 6:27 AM

Added test for --strip-symbol and --keep-file-symbols.
This follows the same behavior as GNU objcopy

LGTM, with the suggested test change. The bug needs fixing separately.

test/tools/llvm-objcopy/keep-file-symbols.test
28–35 ↗(On Diff #148598)

You only need two symbols - one file symbol and one non-file symbol.

37–48 ↗(On Diff #148598)

Wait, shouldn't there be a null symbol here? Null symbols are required... That's probably a bug unrelated to your change, but should be fixed ASAP.

paulsemel added inline comments.May 25 2018, 7:17 AM
test/tools/llvm-objcopy/keep-file-symbols.test
28–35 ↗(On Diff #148598)

You're totally right. I'll remove those :)

37–48 ↗(On Diff #148598)

Well, yes, I added a TODO for this in my previous patch, about this dummy symbol. We shouldn't pass it to the removeSymbols function as @jakehehrlich stated :)
Do you want me to work on it ? :)

jhenderson added inline comments.May 25 2018, 7:38 AM
test/tools/llvm-objcopy/keep-file-symbols.test
37–48 ↗(On Diff #148598)

If you don't mind. What we currently generate is actually illegal ELF. I was under the impression that the TODO was for coming up with a better way for handling the null symbol, and I assumed that it wasn't being stripped (unless the whole table was), hence why I didn't comment on it before.

jhenderson accepted this revision.May 25 2018, 7:38 AM

Forgot to accept the revision.

This revision is now accepted and ready to land.May 25 2018, 7:38 AM
paulsemel updated this revision to Diff 148611.May 25 2018, 8:18 AM

Added James' suggestions.

test/tools/llvm-objcopy/keep-file-symbols.test
37–48 ↗(On Diff #148598)

Well, you're actually right. The TODO was for this reason, definitely. But if we handle it the way we planned to handle it, this will also fix this issue :)

jakehehrlich accepted this revision.May 25 2018, 12:39 PM
alexshap accepted this revision.May 25 2018, 12:58 PM
This revision was automatically updated to reflect the committed changes.