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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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)
+
----------------
alexshap wrote:
> 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
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.


================
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);
+  }
----------------
alexshap wrote:
> 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
Yes, I think we should update them both. Happy for that to be a separate change 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));
----------------
alexshap wrote:
> 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.   
> 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).



Repository:
  rL LLVM

https://reviews.llvm.org/D46407