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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich 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:
> 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.
Actully, can we rename Opts.td to ObjcopyOpts.td?


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


Repository:
  rL LLVM

https://reviews.llvm.org/D46407