Page MenuHomePhabricator

[llvm-objcopy] Add --strip-unneeded-symbol(s)

Authored by evgeny777 on Feb 11 2019, 12:28 AM.

Diff Detail


Event Timeline

evgeny777 created this revision.Feb 11 2019, 12:28 AM
mstorsjo added inline comments.
29 ↗(On Diff #186189)

All the newly added tests produce results identical to some of the existing tests, removing all symbols. Wouldn't it be useful with a test that only removes some of the unneeded symbols but not all of them, to showcase the difference?

evgeny777 marked an inline comment as done.Feb 11 2019, 4:53 AM
evgeny777 added inline comments.
29 ↗(On Diff #186189)

--strip-unneeded removes only bar, foobar and foobaz. File and section symbols are not removed, also foo is not removed because it's in comdat. This is checked by original test case (line 6). Additional tests check that

  1. Unneeded symbols are removed, but not STT_FILE / STT_SECTON nor comdat (%t3)
  2. Stripping all symbols by regex pattern .* is identical to ---strip-unneeded (%t4)
  3. Stripping just bar, foobar and foobaz is identical to --strip-unneeded, because all others should be preserved (%t5)
  4. The %t6 is identical to %t4, but using a file

What kind of extra test do you feel is needed?

mstorsjo added inline comments.Feb 11 2019, 5:00 AM
29 ↗(On Diff #186189)

A test that removes only one of bar/foobar/foobaz but not all of them.

Thanks, looks good to me now. I'll leave someone else to do the overall approval though.

rupprecht accepted this revision.Feb 11 2019, 11:33 AM
rupprecht added inline comments.
368 ↗(On Diff #186237)

nit: these checks should be inverted, i.e. first check if the command line flag has been set, and then call isUnneededSymbol() to see if it applies.

This revision is now accepted and ready to land.Feb 11 2019, 11:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2019, 11:35 PM