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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
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:
> 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).
> 
@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


Repository:
  rL LLVM

https://reviews.llvm.org/D46407