[llvm-objcopy] Improve tool selection logic to recognize llvm-strip-$major as strip

[MaskRay]

Summary

Debian and some other distributions install llvm-strip as llvm-strip-$major (e.g. /usr/bin/llvm-strip-9)

D54193 made it work with llvm-strip-$major but did not add a test.
The behavior was regressed by D69146.

Fixes https://github.com/ClangBuiltLinux/linux/issues/940

[abrachet]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

332

StringRef is small enough that its probably faster to capture by value, no?

333

Might as well just explicitly use size_t

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

335

What would you think about smth like this:

const llvm::Regex StripRegex("^(llvm-)?strip(.*)$"); 
const llvm::Regex InstallNameToolRegex("^(llvm-)?install-name-tool(.*)$");
...
if (StripRegex.match(Tool))
  ...
else if (InstallNameToolRegex.match(Tool))
  ...

[nathanchance]

Seems sane to me if I am understanding the string arithmetic correctly. I can confirm this fixes the Debian use case that we came across.

[MaskRay]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

333

The return type of rfind_lower probably should be std::string::size_type. Using auto will be width agnostic. (Although it is unlikely that size_t != std:string::size_type)

335

The current code is to make it similar to D71302 llvm-ar

We need to support something like x86_64-unknown-linux-gnu-strip

[MaskRay]

Add tests about x86_64-linux-gnu-strip

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

335

0/ the title of the diff "Support llvm-strip-11 as argv[0]" seems to be a little bit confusing.
1/ In any case - here and in D71302 the matching logic seems to be a tiny bit more complicated than it can be
(from the readability perspective).
Depending on what you want - if we need a strict well-controlled robust way of matching specific name patterns I'd probably use a regular expression rather than some ad-hoc code, if a simple match is good enough - i suspect StringRef::contains / StringRef::contains_lower could do their job here. Though I don't insist.

[MaskRay]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

335

D71302 has to be complex because "lib" can be in the target triple ("arm-pokymllib32-linux-gnueabi-llvm-ar"). We need to guard against such cases. The logic is slightly more rubust. It can work with powerpc64-stripinvalid-freebsd13-objcopy (not very unrealistic).

I actually think Regex.h can increase code complexity.

[jhenderson]

llvm/test/tools/llvm-objcopy/tool-name.test

14

Maybe worth a test case for "[llvm-]strip" etc being in the middle of the name, i.e. something like "gnu-llvm-strip-11". Perhaps it could replace this last test case, as I don't think that one then provides additional coverage.

33

I would expect this to have the same set of cases as the other tools. I don't see why they are unique, just because it's emulating the install_name_tool.

[MaskRay]

Improve tests

llvm/test/tools/llvm-objcopy/tool-name.test

33

Rearranged the tests. Hope that will provide sufficient coverage without being verbose.

[MaskRay]

Delete an excess empty line

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

335

@MaskRay thanks for your patience and for the quick fix.

0/ Would be good to add a comment explaining what strings this code is intended to match
(so that people don't need to look through the tests).

1/ I suggested Regex primarily because this way it seemed to me that the code would be self-documented / easier to reason about, thus we would avoid having such bugs in the future, although, perhaps, this is subjective.

[MaskRay]

Improve comments in code.

[MaskRay]

A note on how GNU objcopy/strip makes a crunched executable:

binutils/not-strip.c (int is_strip = 0;) is linked into objcopy while binutils/is-strip.c (int is_strip = 1;) is linked into strip. Though it is strange that on my Debian, size(/usr/bin/x86_64-linux-gnu-objcopy) != size(/usr/bin/x86_64-linux-gnu-strip ...

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

335

Thanks! A comment will be very useful here. I should have put up a comment as I did for D71302 ...

[MaskRay]

@hans If it is not too late for LLVM 10.0.0, I hope this can be picked. If it is too late, I hope it can be included in 10.0.1 ...

Debian and some other distributions will ship /usr/bin/llvm-strip-10 but not /usr/bin/llvm-strip (https://packages.debian.org/bullseye/amd64/llvm-9/filelist). Without this patch, /usr/bin/llvm-strip-10 will think it is objcopy.

llvm-{objcopy,strip,install-name-tool} is a crunched executable. llvm-strip-10 a.o (implicit --strip-all) and llvm-objcopy-10 a.o (almost a no-op) are different...
llvm-strip a.o -o b.o is good but llvm-objcopy-10 a.o -o b.o would error.

llvm/test/tools/llvm-objcopy/tool-name.test

+## Don't make symlinks on Windows.
+# UNSUPPORTED: system-windows
+
+# RUN: rm -rf %t
+# RUN: mkdir %t
+
+# RUN: ln -s llvm-objcopy %t/llvm-objcopy-11.exe
+# RUN: ln -s llvm-objcopy %t/powerpc64-unknown-freebsd13-objcopy
+
+# RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY %s
+# RUN: %t/llvm-objcopy-11.exe --help | FileCheck --check-prefix=OBJCOPY %s
+# RUN: %t/powerpc64-unknown-freebsd13-objcopy --help | FileCheck --check-prefix=OBJCOPY %s
+
+# OBJCOPY: OVERVIEW: llvm-objcopy tool
+
+# RUN: ln -s llvm-strip %t/strip.exe
+# RUN: ln -s llvm-strip %t/gnu-llvm-strip-10
+
+# RUN: llvm-strip --help | FileCheck --check-prefix=STRIP %s
+# RUN: %t/strip.exe --help | FileCheck --check-prefix=STRIP %s
+# RUN: %t/gnu-llvm-strip-10 --help | FileCheck --check-prefix=STRIP %s
+
+# STRIP: OVERVIEW: llvm-strip tool
+
+## This driver emulates install_name_tool on macOS.
+# RUN: ln -s llvm-install-name-tool %t/llvm-install-name-tool-10
+# RUN: ln -s llvm-install-name-tool %t/install_name_tool.exe
+
+# RUN: llvm-install-name-tool --help | FileCheck --check-prefix=INSTALL %s
+# RUN: %t/llvm-install-name-tool-10 --help | FileCheck --check-prefix=INSTALL %s
+# RUN: %t/install_name_tool.exe --help | FileCheck --check-prefix=INSTALL %s
+
+# INSTALL: OVERVIEW: llvm-install-name-tool tool

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 
 enum class ToolType { Objcopy, Strip, InstallNameTool };
 
 } // anonymous namespace
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
-  ToolType Tool = StringSwitch<ToolType>(sys::path::stem(ToolName))
-                      .EndsWith("strip", ToolType::Strip)
-                      .EndsWith("install-name-tool", ToolType::InstallNameTool)
-                      .EndsWith("install_name_tool", ToolType::InstallNameTool)
-                      .Default(ToolType::Objcopy);
+
+  StringRef Stem = sys::path::stem(ToolName);
+  auto Is = [=](StringRef Tool) {
+    // We need to recognize the following filenames:
+    //
+    // llvm-objcopy -> objcopy
+    // strip-10.exe -> strip
+    // powerpc64-unknown-freebsd13-objcopy -> objcopy
+    // llvm-install-name-tool -> install-name-tool
+    auto I = Stem.rfind_lower(Tool);
+    return I != StringRef::npos &&
+           (I + Tool.size() == Stem.size() || !isAlnum(Stem[I + Tool.size()]));
+  };
+  ToolType Tool = ToolType::Objcopy;
+  if (Is("strip"))
+    Tool = ToolType::Strip;
+  else if (Is("install-name-tool") || Is("install_name_tool"))
+    Tool = ToolType::InstallNameTool;
+
   // Expand response files.
   // TODO: Move these lines, which are copied from lib/Support/CommandLine.cpp,
   // into a separate function in the CommandLine library and call that function
   // here. This is duplicated code.
   SmallVector<const char *, 20> NewArgv(argv, argv + argc);
   BumpPtrAllocator A;
   StringSaver Saver(A);
   cl::ExpandResponseFiles(Saver,