[llvm-objcopy] Fix help text

[sameerarora101]

Summary

Update help text for each of the three tools: objcopy, strip and
install-name-tool. For reference: the issue is
highlighted in comments under https://reviews.llvm.org/D81527.

[jhenderson]

llvm-strip doesn't allow a positional argument output either, as it can take multiple input files. This looks like a regression in the help text since an older version I have kicking around. Could you fix it for both (and update the patch description), please? You should also expand the help message test for both llvm-strip and llvm-objcopy to test that the full string has been matched.

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

Rather than relying on a string for the name, I wonder if this code would be more robust with an enum that represents the three different tools, given that there's always potential for more front ends to be added. You would probably still want to pass in the tool name, but that could just be whatever the executable is called, possibly. For example, if the tool was llvm_install_name_tool, it would print that as the name in the help text message, rather than the current llvm-install-name-tool.

[sameerarora101]

Update help text for install-name-tool, strip and objcopy.

[sameerarora101]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

So I made a small enum and passed the appropriate enum types at the various function calls to printHelp. Does this work?

[jhenderson]

LGTM, with one nit.

llvm/test/tools/llvm-objcopy/ELF/help-message.test

17–18

Nit: whilst you're here, please delete one of these two blank lines.

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-help-message.test

1–3

So I just noticed that neither this nor the llvm-objcopy/llvm-strip test belong in their current directories - they should be one level up, since they are not platform specific, and probably then merged into a single test in my opinion. No need to change it in this patch, but if you can afford the hopefully small amount of time to do it, I think merging the two tests and putting them in llvm/test/tools/llvm-objcopy/ would be a nice thing to do.

Note that tool-name.test already tests all three tools.

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

Yup, thanks.

[sameerarora101]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-help-message.test

1–3

yup, i can create a separate diff merging the two help test files into help-message.test

[sameerarora101]

Deleting extra blank line.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

nit: ToolType ToolEnum -> ToolType Tool

ToolName can be derived from ToolType, we probably don't need to pass it as an argument.

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

How would you and others feel about passing in the real executable name and printing that instead? It was an idea I had, but didn't suggest it before.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

I would be fine with that

[sameerarora101]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

@jhenderson What do you mean by "real executable name"? So what should I be doing here now? We keep the ToolName? Sorry for not understanding what you guys meant exactly 😔

Currently I have made the change ToolType ToolEnum -> ToolType Tool. Not sure about ToolName yet?

[sameerarora101]

ToolEnum -> Tool

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

@sameerarora101, sorry about the confusion, if I understand correctly @jhenderson is referring to the global variable ToolName (initialized in llvm-objcopy.cpp) or its basename. My first thought was that it might be useful but now i think that there are some undesirable consequences of this approach - e.g. some unexpected changes to the help message if the binary is moved around. It's already the case to some extent (because that's how we differentiate objcopy vs strip vs install-name-tool), but now it'll happen more frequently + it would create a dependency on the global variable from another file. I don't insist on any particular approach here, it feels like it's simpler to get rid of the duplication which I mentioned above and keep the old behavior.

[sameerarora101]

Removing ToolName as an argument

[sameerarora101]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

oh ok I think I understand now. Thanks a lot for clarifying @alexshap . I'll update the diff to remove duplication.

[jhenderson]

llvm/tools/llvm-objcopy/CopyConfig.cpp

403–419

Thanks for the update. I was suggesting something like the following output:

my-renamed-objcopy-11.exe [options] input [output]

etc, i.e. whatever the real file name is, but I'm not sure about it myself either way, so I'm happy not to bother.

[alexander-shaposhnikov]

LGTM, thanks!

llvm/test/tools/llvm-objcopy/ELF/help-message.test

-# RUN: llvm-objcopy -h | FileCheck --check-prefix=OBJCOPY-USAGE %s
-# RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY-USAGE %s
-# RUN: not llvm-objcopy 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s
+# RUN: llvm-objcopy -h | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
+# RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
+# RUN: not llvm-objcopy 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s --match-full-lines
 # RUN: not llvm-objcopy -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
-# RUN: llvm-strip -h | FileCheck --check-prefix=STRIP-USAGE %s
-# RUN: llvm-strip --help | FileCheck --check-prefix=STRIP-USAGE %s
-# RUN: not llvm-strip 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s
+# RUN: llvm-strip -h | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
+# RUN: llvm-strip --help | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
+# RUN: not llvm-strip 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s --match-full-lines
 # RUN: not llvm-strip -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-strip --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-strip --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
+# OBJCOPY-USAGE:  USAGE: llvm-objcopy [options] input [output]
+# OBJCOPY-USAGE:  Pass @FILE as argument to read options from FILE.
 
-# OBJCOPY-USAGE:  USAGE: llvm-objcopy
-# OBJCOPY-USAGE:  @FILE
-
-# STRIP-USAGE:    USAGE: llvm-strip
-# STRIP-USAGE:    @FILE
+# STRIP-USAGE:    USAGE: llvm-strip [options] inputs...
+# STRIP-USAGE:    Pass @FILE as argument to read options from FILE.
 
 # UNKNOWN-ARG:    unknown argument '{{-+}}abcabc'
 # NO-INPUT-FILES: no input file specified

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-help-message.test

-# RUN: llvm-install-name-tool -h | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s
-# RUN: llvm-install-name-tool --help | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s
-# RUN: not llvm-install-name-tool 2>&1 | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s
+# RUN: llvm-install-name-tool -h | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
+# RUN: llvm-install-name-tool --help | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
+# RUN: not llvm-install-name-tool 2>&1 | FileCheck --check-prefix=INSTALL-NAME-TOOL-USAGE %s --match-full-lines
 # RUN: not llvm-install-name-tool -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-install-name-tool --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 
-# INSTALL-NAME-TOOL-USAGE: USAGE: llvm-install-name-tool
-# INSTALL-NAME-TOOL-USAGE: @FILE
+# INSTALL-NAME-TOOL-USAGE: USAGE: llvm-install-name-tool [options] input
+# INSTALL-NAME-TOOL-USAGE: Pass @FILE as argument to read options from FILE.
 
 # UNKNOWN-ARG: unknown argument '{{-+}}abcabc'

llvm/tools/llvm-objcopy/CopyConfig.cpp

 
 template <class T> static ErrorOr<T> getAsInteger(StringRef Val) {
   T Result;
   if (Val.getAsInteger(0, Result))
     return errc::invalid_argument;
   return Result;
 }
 
+namespace {
+
+enum class ToolType { Objcopy, Strip, InstallNameTool };
+
+} // anonymous namespace
+
 static void printHelp(const opt::OptTable &OptTable, raw_ostream &OS,
-                      StringRef ToolName) {
-  OptTable.PrintHelp(OS, (ToolName + " input [output]").str().c_str(),
+                      ToolType Tool) {
+  StringRef HelpText, ToolName;
+  switch (Tool) {
+  case ToolType::Objcopy:
+    ToolName = "llvm-objcopy";
+    HelpText = " [options] input [output]";
+    break;
+  case ToolType::Strip:
+    ToolName = "llvm-strip";
+    HelpText = " [options] inputs...";
+    break;
+  case ToolType::InstallNameTool:
+    ToolName = "llvm-install-name-tool";
+    HelpText = " [options] input";
+    break;
+  }
+  OptTable.PrintHelp(OS, (ToolName + HelpText).str().c_str(),
                      (ToolName + " tool").str().c_str());
   // TODO: Replace this with libOption call once it adds extrahelp support.
   // The CommandLine library has a cl::extrahelp class to support this,
   // but libOption does not have that yet.
   OS << "\nPass @FILE as argument to read options from FILE.\n";
 }
 
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig>
 parseObjcopyOptions(ArrayRef<const char *> ArgsArr,
                     llvm::function_ref<Error(Error)> ErrorCallback) {
   DriverConfig DC;
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
-    printHelp(T, errs(), "llvm-objcopy");
+    printHelp(T, errs(), ToolType::Objcopy);
     exit(1);
   }
 
   if (InputArgs.hasArg(OBJCOPY_help)) {
-    printHelp(T, outs(), "llvm-objcopy");
+    printHelp(T, outs(), ToolType::Objcopy);
     exit(0);
   }
 
   if (InputArgs.hasArg(OBJCOPY_version)) {
     outs() << "llvm-objcopy, compatible with GNU objcopy\n";
     cl::PrintVersionMessage();
     exit(0);
   }
parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
   CopyConfig Config;
   InstallNameToolOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
-    printHelp(T, errs(), "llvm-install-name-tool");
+    printHelp(T, errs(), ToolType::InstallNameTool);
     exit(1);
   }
 
   if (InputArgs.hasArg(INSTALL_NAME_TOOL_help)) {
-    printHelp(T, outs(), "llvm-install-name-tool");
+    printHelp(T, outs(), ToolType::InstallNameTool);
     exit(0);
   }
 
   if (InputArgs.hasArg(INSTALL_NAME_TOOL_version)) {
     outs() << "llvm-install-name-tool, compatible with cctools "
               "install_name_tool\n";
     cl::PrintVersionMessage();
     exit(0);

 parseStripOptions(ArrayRef<const char *> ArgsArr,
                   llvm::function_ref<Error(Error)> ErrorCallback) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
-    printHelp(T, errs(), "llvm-strip");
+    printHelp(T, errs(), ToolType::Strip);
     exit(1);
   }
 
   if (InputArgs.hasArg(STRIP_help)) {
-    printHelp(T, outs(), "llvm-strip");
+    printHelp(T, outs(), ToolType::Strip);
     exit(0);
   }
 
   if (InputArgs.hasArg(STRIP_version)) {
     outs() << "llvm-strip, compatible with GNU strip\n";
     cl::PrintVersionMessage();
     exit(0);
   }