[PATCH] D46407: [tools] Introduce llvm-strip

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/CMakeLists.txt:10
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
-add_public_tablegen_target(ObjcopyTableGen)
+add_public_tablegen_target(OptsTableGen)
+
----------------
jhenderson wrote:
> 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.
if it matters - sure, i just wanted to have a consistent naming scheme Opts -> OptsTableGen, StripOpts -> StripOptsTableGen etc


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:487-490
+  if (InputArgs.size() == 0 || InputArgs.hasArg(STRIP_help)) {
+    T.PrintHelp(outs(), "llvm-strip <input> [ <output> ]", "strip tool");
+    exit(0);
+  }
----------------
jhenderson wrote:
> I think the norm is to exit(1) if the input count is 0, unless help has been specified.
i was looking at the current behavior, would be better to have consistency here, we can update them both, though


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:519
+  CopyConfig Config;
+  if (sys::path::stem(ToolName).endswith_lower("strip"))
+    Config = ParseStripOptions(makeArrayRef(argv + 1, argc));
----------------
jhenderson wrote:
> 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?
@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.   


Repository:
  rL LLVM

https://reviews.llvm.org/D46407