Page MenuHomePhabricator

[llvm-strip] Have --discard-all imply --strip-debug
ClosedPublic

Authored by sidneym on Apr 24 2019, 2:04 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sidneym created this revision.Apr 24 2019, 2:04 PM
grimar added inline comments.Apr 25 2019, 2:15 AM
test/tools/llvm-objcopy/ELF/discard-all-debug.test
2 ↗(On Diff #196515)

When I call GNU strip, it works without --keep-symbol=.L.str for this input.
Do you know why whe have a difference here?

29 ↗(On Diff #196515)

Seems instead of lines 3-29 all you really need is just this line:

# RUN: llvm-readobj --sections %t | FileCheck %s --implicit-check-not=.debug_
sidneym marked 2 inline comments as done.Apr 25 2019, 7:32 AM
sidneym added inline comments.
test/tools/llvm-objcopy/ELF/discard-all-debug.test
2 ↗(On Diff #196515)

GNU strip doesn't issue a message when a reference is still required, it just keeps it.
The fatal error llvm-strip produces can cause some issues when trying to use it as a drop-in replacement for gnu strip. The error is generated in removeSymbols and if there was a way to infer that strip was the executable perhaps that check could be avoided.

29 ↗(On Diff #196515)

I didn't know about this option I will make the change, thanks.

sidneym updated this revision to Diff 196661.Apr 25 2019, 9:45 AM

Update testcase.

bcain accepted this revision.Apr 25 2019, 1:13 PM
This revision is now accepted and ready to land.Apr 25 2019, 1:13 PM

This appears to be correct. StripDebug is a preety simple and minimal field so we can go ahead and add this. The keep symbol thing still looks a bit funny.

Can you add this behavior to llvm-objcopy as well? We don't want them to be inconsistent.

test/tools/llvm-objcopy/ELF/discard-all-debug.test
2 ↗(On Diff #196515)

This doesn't explain grimar's question from my perspective and I have the same question.

I have no more comments, minor nit/suggestion is inline.

test/tools/llvm-objcopy/ELF/discard-all-debug.test
5 ↗(On Diff #196661)

I am not sure it is useful to check the number of sections. All you need is to check that .debug_ sections are gone I think.
And that is done by --sections + --implicit-check-not=.debug_

sidneym marked an inline comment as done.Apr 26 2019, 12:20 PM
sidneym added inline comments.
test/tools/llvm-objcopy/ELF/discard-all-debug.test
2 ↗(On Diff #196515)

The binary used for the testcase has a symbol with a reference that cannot be stripped. GNU-strip will leave it in place with no message or warning, llvm errors out unless explicitly kept. The code doing it is: RelocationSection::removeSymbols in Object.cpp

sidneym updated this revision to Diff 196897.Apr 26 2019, 12:22 PM

Make llvm-objcopy behave the same way strip does wrt --discard-all. Update testcase

rupprecht accepted this revision.Apr 30 2019, 12:10 PM
This revision was automatically updated to reflect the committed changes.