Page MenuHomePhabricator

[tools] Introduce llvm-strip
ClosedPublic

Authored by alexshap on May 3 2018, 1:46 PM.

Details

Summary

To start the ball rolling this diff adds the initial bits for llvm-strip.
I think more options & tests can be added incrementally.

Test plan:
make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

alexshap created this revision.May 3 2018, 1:46 PM
alexshap edited the summary of this revision. (Show Details)May 3 2018, 1:57 PM

Will be good idea to add documentation and item in Release Notes.

jakehehrlich accepted this revision.May 3 2018, 5:30 PM

LGTM but please wait on James

test/tools/llvm-objcopy/strip-all.test
11 ↗(On Diff #145088)

Can you add a diff here to ensure that there is binary equivalence?

test/tools/llvm-objcopy/strip-debug.test
11 ↗(On Diff #145088)

diff here as well.

This revision is now accepted and ready to land.May 3 2018, 5:30 PM
jhenderson added inline comments.May 4 2018, 1:26 AM
tools/llvm-objcopy/CMakeLists.txt
10 ↗(On Diff #145088)

I'm not really familiar with cmake, but does this name have to be unique? If so, we might want to go back to the old name.

tools/llvm-objcopy/llvm-objcopy.cpp
487–490 ↗(On Diff #145088)

I think the norm is to exit(1) if the input count is 0, unless help has been specified.

498–499 ↗(On Diff #145088)

Test case?

519 ↗(On Diff #145088)

I'm not a massive fan of using the tool's filename to determine the expected behaviour (example reason: if I change the executable from llvm-strip to llvm-strip-backup, when testing two versions of the tool side-by-side, I'd switch to objcopy style behaviour unexpectedly).

How feasible would it be to build two different executables, using the same source code, but a command-line specified define to control this?

alexshap added inline comments.May 4 2018, 1:37 AM
tools/llvm-objcopy/CMakeLists.txt
10 ↗(On Diff #145088)

if it matters - sure, i just wanted to have a consistent naming scheme Opts -> OptsTableGen, StripOpts -> StripOptsTableGen etc

tools/llvm-objcopy/llvm-objcopy.cpp
487–490 ↗(On Diff #145088)

i was looking at the current behavior, would be better to have consistency here, we can update them both, though

519 ↗(On Diff #145088)

@jhenderson
alexshap-mbp:tools alexshap$ grep -r -n add_llvm_tool_symlink ./*
./llvm-ar/CMakeLists.txt:17:add_llvm_tool_symlink(llvm-ranlib llvm-ar)
./llvm-ar/CMakeLists.txt:18:add_llvm_tool_symlink(llvm-lib llvm-ar)
./llvm-ar/CMakeLists.txt:19:add_llvm_tool_symlink(llvm-dlltool llvm-ar)
./llvm-ar/CMakeLists.txt:22: add_llvm_tool_symlink(ar llvm-ar)
./llvm-ar/CMakeLists.txt:23: add_llvm_tool_symlink(dlltool llvm-ar)
./llvm-ar/CMakeLists.txt:24: add_llvm_tool_symlink(ranlib llvm-ar)
./llvm-cxxfilt/CMakeLists.txt:11: add_llvm_tool_symlink(c++filt llvm-cxxfilt)
./llvm-dwp/CMakeLists.txt:20: add_llvm_tool_symlink(dwp llvm-dwp)
./llvm-nm/CMakeLists.txt:19: add_llvm_tool_symlink(nm llvm-nm)
./llvm-objcopy/CMakeLists.txt:21: add_llvm_tool_symlink(objcopy llvm-objcopy)
./llvm-objdump/CMakeLists.txt:30: add_llvm_tool_symlink(objdump llvm-objdump)
./llvm-readobj/CMakeLists.txt:26:add_llvm_tool_symlink(llvm-readelf llvm-readobj)
./llvm-readobj/CMakeLists.txt:29: add_llvm_tool_symlink(readelf llvm-readobj)
./llvm-size/CMakeLists.txt:11: add_llvm_tool_symlink(size llvm-size)
./llvm-strings/CMakeLists.txt:12: add_llvm_tool_symlink(strings llvm-strings)
./llvm-symbolizer/CMakeLists.txt:20: add_llvm_tool_symlink(addr2line llvm-symbolizer)

personally, I'm against building two different executables, it increases the build time, the size of the package, occupies the disk space.

jhenderson added inline comments.May 4 2018, 4:40 AM
tools/llvm-objcopy/CMakeLists.txt
10 ↗(On Diff #145088)

If it doesn't need to be unique, I'm happy with it as is. If it does, I think we need a name that is more tool-specific (even if it's just the old name), to avoid clashing with some other tool.

tools/llvm-objcopy/llvm-objcopy.cpp
487–490 ↗(On Diff #145088)

Yes, I think we should update them both. Happy for that to be a separate change though.

519 ↗(On Diff #145088)

personally, I'm against building two different executables, it increases the build time, the size of the package, occupies the disk space.

I think this is just a philosophical difference. Happy to defer to the code owner's preference.

FWIW, most of those examples aren't fair comparisons, since they are just mapping the llvm tool name to the equivalent GNU name (which I have no issue with). In this case, we actually have different behaviour depending on the name, which I doubt is the case in any of those tools (although I haven't looked). That being said, LLD is also an example that does something similar (different behaviour depending on the file name).

alexshap added inline comments.May 4 2018, 9:02 AM
tools/llvm-objcopy/llvm-objcopy.cpp
519 ↗(On Diff #145088)

@jhenderson

which I doubt is the case in any of those tool

FWIW - that's not correct:

./llvm-ar/llvm-ar.cpp:957: Stem = sys::path::stem(ToolName);
./llvm-ar/llvm-ar.cpp-958- if (Stem.contains_lower("dlltool"))

./llvm-cov/llvm-cov.cpp:64: if (sys::path::stem(argv[0]).endswith_lower("gcov"))
./llvm-cov/llvm-cov.cpp-65- return gcovMain(argc, argv);

./llvm-readobj/llvm-readobj.cpp:564: if (sys::path::stem(argv[0]).find("readelf") != StringRef::npos)
./llvm-readobj/llvm-readobj.cpp-565- opts::Output = opts::GNU;

maybe more, i didn't look further

alexshap added inline comments.May 4 2018, 9:05 AM
tools/llvm-objcopy/llvm-objcopy.cpp
487–490 ↗(On Diff #145088)

ok, sure,
I will send a separate diff to update it once this gets committed.

498–499 ↗(On Diff #145088)

yeah, will add

alexshap marked 4 inline comments as done.May 4 2018, 12:07 PM
alexshap updated this revision to Diff 145250.May 4 2018, 12:18 PM

Address comments

alexshap marked 2 inline comments as done.May 4 2018, 12:19 PM
jakehehrlich added inline comments.May 4 2018, 12:31 PM
tools/llvm-objcopy/CMakeLists.txt
10 ↗(On Diff #145088)

Actully, can we rename Opts.td to ObjcopyOpts.td?

tools/llvm-objcopy/llvm-objcopy.cpp
519 ↗(On Diff #145088)

I tried a while back, quite hard, to get cmake to build two tools using the same code. Unless we want to put the effort into making this a proper library (and e.g. making sure there is a sufficiently common interface to all object formats, making everything super nice, etc...) we can't make two separate binaries. Also teams that use llvm-objcopy care about toolchain size and this lets them add llvm-strip without worry. I thought a lot about how to make this a binary vs doing what every other tool has done and I decided I preferred the symlink option. Most of the point of moving to TableGen was to allow llvm-strip to exist. In a nutshell, I'd like to strongly advocate for this approach over what I see as the only alternative, making objcopy a library.

alexshap updated this revision to Diff 145262.May 4 2018, 1:07 PM

Opts.td -> ObjcopyOpts.td

Closed by commit rL331663: [tools] Introduce llvm-strip (authored by alexshap, committed by ). · Explain WhyMay 7 2018, 12:35 PM
This revision was automatically updated to reflect the committed changes.

As I had open comments, I'd have preferred this not been committed until after I'd given the thumbs up too...

Test comments are all that I've got though.

llvm/trunk/test/tools/llvm-objcopy/strip-debug.test
13

This should probably be a distinct test that doesn't use any command-line options (similar to the basic-copy style tests for objcopy).

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp
494

Test case for this as well.

alexshap added inline comments.May 8 2018, 10:17 AM
llvm/trunk/test/tools/llvm-objcopy/strip-debug.test
13

ok, i'll send a follow-up diff today