This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Match GNU behaviour regarding file symbols
ClosedPublic

Authored by kongyi on Mar 24 2020, 12:45 AM.

Details

Summary

GNU objcopy removes STT_FILE symbols for --strip-debug operations, and keeps them for --discard-all operation. Match their behaviour for llvm-objcopy.

Diff Detail

Event Timeline

kongyi created this revision.Mar 24 2020, 12:45 AM
MaskRay added inline comments.Mar 24 2020, 11:05 PM
llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test
12

We need tests for --strip-debug --keep-file-symbols and --strip-unneeded --keep-file-symbols

110

Align Symbols [

Add a space after #

kongyi updated this revision to Diff 252516.Mar 25 2020, 1:52 AM
kongyi marked 2 inline comments as done.
kongyi added inline comments.
llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test
110

I don't think we should taint the blame history just for minor cosmetic reason. Besides, plenty of other tests does not have space after #.

jhenderson added inline comments.Mar 25 2020, 1:52 AM
llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test
12

Yes, this test is about the "--keep-file-symbols" option. All calls should use it. To test your behaviour, you should add to the strip-debug.test, strip-unneeded.test and discard-all.test (although as @MaskRay says, you should also test things here with --keep-file-symbols).

You also need test cases for both llvm-objcopy and llvm-strip, since you've modified code that affects them each separately.

130

Nit: too many blank lines at end of file.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
424

No need for this code change: --keep-file-symbols is actioned above at line 410.

kongyi updated this revision to Diff 258364.Apr 17 2020, 10:18 AM
kongyi edited the summary of this revision. (Show Details)
jhenderson added inline comments.Apr 20 2020, 12:15 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
425

You still don't need && !Config.KeepFileSymbols. Line 410 of this file handles that case (if you delete it, you should see no changes in test results).

kongyi updated this revision to Diff 258673.Apr 20 2020, 1:20 AM

LGTM, with the suggested comment deletion. Please let @MaskRay comment too.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
423–425

Taking another look, I don't think you need the comment at all as the code clearly describes this behaviour (--strip-debug removing STT_FILE symbols), so I think you can delete it.

MaskRay accepted this revision.Apr 20 2020, 4:02 PM

LGTM, with the comment deleted as requested.

This revision is now accepted and ready to land.Apr 20 2020, 4:02 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 8:36 PM
thakis added a subscriber: thakis.Apr 20 2020, 8:53 PM

Looks like this breaks tests everywhere, e.g http://45.33.8.238/linux/15764/step_12.txt

Please take a look and revert if it takes a while to investigate.

Looks like this breaks tests everywhere, e.g http://45.33.8.238/linux/15764/step_12.txt

Please take a look and revert if it takes a while to investigate.

This is already reverted.