[llvm-ar] Modify usage printouts to use the correct toolname

[chrisjackson]

Summary

When using 'llvm-ar --help' or 'llvm-ranlib --help' the toolname is currently hardcoded e.g. 'USAGE: llvm-ar ....' . Therefore if a custom toolchain is used the usage is in fact incorrect, 'custom-llvm-ar --help' will still display 'USAGE: llvm-ar'.

This patch brings the usage toolname printout in line with other tools such as llvm-objdump and llvm-readobj. However, the patch stops well short of converting the help options to tablegen, which would bring llvm-ar fully inline with the other tool implementations and remove the string literals for the options altogether.

[gbreynoo]

This looks like a good change to me, just needs that ranlib test fixing.

[MaskRay]

Looks good to me.

llvm/tools/llvm-ar/llvm-ar.cpp

66

printArHelp omits ToolName on the OVERVIEW line. Omit it here?

[jhenderson]

Two test nits, otherwise looks fine to me.

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

17–20

You should add something like {{ }} at the end of these lines, to show that there is nothing else after the tool name. In particular, whether .exe is present in the SUFFIX case.

20

What's the purpose of this line? To show that the prefix is printed as much as the suffix, or something more specific?

Also, it's missing "USAGE:" unlike the other patterns.

[chrisjackson]

Respond to reviewer comments:

• Add missed llvm-ranlib/tool-name.test update
• Make ranlib printout consistent with ar
• Make toolname pattern check more strict

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

20

looking at https://github.com/llvm/llvm-project/commit/4d53b99c5df2c83172d23521d1b1ab8361d6de92, this exists because the tool selection heuristic would mistakenly think it was 'llvm-lib', so I have retained this check.

llvm/tools/llvm-ar/llvm-ar.cpp

66

Agreed, removed for consistency,

[chrisjackson]

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

20

Actually, thinking about it, this check no longer guarantees that the correct tool has been chosed. A different line from the usage output that contains the default tool name will have to be checked

[MaskRay]

LGTM once @jhenderson's comments are addressed.

[chrisjackson]

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

20

On second thoughts, I think my original response stands. @jhenderson please can you confirm if you are you happy with this explanation?

[jhenderson]

LGTM, with suggestion.

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

20

I think it's fine, but might warrant a brief comment saying the purpose of the check.

[chrisjackson]

Address reviewer comments by adding a clarification in the llvm-ar tool-name test.

[jhenderson]

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

13

I'd quote the "lib" word, as per the suggested edit: this should make it clear that "lib" is actually the substring, and not that there's some "lib substring".

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

 ## Don't make symlinks on Windows.
 # UNSUPPORTED: system-windows
 
 # RUN: rm -rf %t
 # RUN: mkdir %t
 # RUN: ln -s llvm-ar %t/llvm-ar-9
 # RUN: ln -s llvm-ar %t/ar.exe
 # RUN: ln -s llvm-ar %t/arm-pokymllib32-linux-gnueabi-llvm-ar-9
 
-# RUN: llvm-ar h | FileCheck %s
-# RUN: %t/llvm-ar-9 h | FileCheck %s
-# RUN: %t/ar.exe h | FileCheck %s
-# RUN: %t/arm-pokymllib32-linux-gnueabi-llvm-ar-9 h | FileCheck %s
+# RUN: llvm-ar h | FileCheck %s --check-prefix=DEFAULT
+# RUN: %t/llvm-ar-9 h | FileCheck %s --check-prefix=VERSION
+# RUN: %t/ar.exe h | FileCheck %s --check-prefix=SUFFIX
+## Ensure that the "lib" substring does not result in misidentification as the
+## llvm-lib tool.
+# RUN: %t/arm-pokymllib32-linux-gnueabi-llvm-ar-9 h | FileCheck %s --check-prefix=ARM
 
-# CHECK: USAGE: llvm-ar
+# DEFAULT: USAGE: llvm-ar{{ }}
+# VERSION: USAGE: llvm-ar-9{{ }}
+# SUFFIX: USAGE: ar{{ }}
+# ARM: USAGE: arm-pokymllib32-linux-gnueabi-llvm-ar-9{{ }}

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

 ## Don't make symlinks on Windows.
 # UNSUPPORTED: system-windows
 
 # RUN: rm -rf %t
 # RUN: mkdir %t
 # RUN: ln -s llvm-ranlib %t/llvm-ranlib-9
 # RUN: ln -s llvm-ranlib %t/ranlib.exe
 
-# RUN: llvm-ranlib -h | FileCheck %s
-# RUN: %t/llvm-ranlib-9 -h | FileCheck %s
-# RUN: %t/ranlib.exe -h | FileCheck %s
+# RUN: llvm-ranlib -h | FileCheck %s --check-prefix=DEFAULT
+# RUN: %t/llvm-ranlib-9 -h | FileCheck %s --check-prefix=VERSION
+# RUN: %t/ranlib.exe -h | FileCheck %s --check-prefix=SUFFIX
 
-# CHECK: USAGE: llvm-ranlib
+# DEFAULT: USAGE: llvm-ranlib{{ }}
+# VERSION: USAGE: llvm-ranlib-9{{ }}
+# SUFFIX: USAGE: ranlib{{ }}

llvm/tools/llvm-ar/llvm-ar.cpp

 using namespace llvm;
 
 // The name this program was invoked as.
 static StringRef ToolName;
 
 // The basename of this program.
 static StringRef Stem;
 
-const char RanlibHelp[] = R"(OVERVIEW: LLVM Ranlib (llvm-ranlib)
-
-  This program generates an index to speed access to archives
-
-USAGE: llvm-ranlib <archive-file>
-
-OPTIONS:
-  -h --help             - Display available options
-  -v --version          - Display the version of this program
-  -D                    - Use zero for timestamps and uids/gids (default)
-  -U                    - Use actual timestamps and uids/gids
-)";
-
-const char ArHelp[] = R"(OVERVIEW: LLVM Archiver
-
-USAGE: llvm-ar [options] [-]<operation>[modifiers] [relpos] [count] <archive> [files]
-       llvm-ar -M [<mri-script]
+static void printRanLibHelp(StringRef ToolName) {
+  outs() << "OVERVIEW: LLVM Ranlib\n\n"
+         << "This program generates an index to speed access to archives\n\n"
+         << "USAGE: " + ToolName + " <archive-file>\n\n"
+         << "OPTIONS:\n"
+         << "  -h --help             - Display available options\n"
+         << "  -v --version          - Display the version of this program\n"
+         << "  -D                    - Use zero for timestamps and uids/gids "
+            "(default)\n"
+         << "  -U                    - Use actual timestamps and uids/gids\n";
+}
 
-OPTIONS:
+static void printArHelp(StringRef ToolName) {
+  const char ArOptions[] =
+      R"(OPTIONS:
   --format              - archive format to create
     =default            -   default
     =gnu                -   gnu
     =darwin             -   darwin
     =bsd                -   bsd
   --plugin=<string>     - ignored for compatibility
   -h --help             - display this help and exit
   --rsp-quoting         - quoting style for response files
MODIFIERS:
   [S] - do not build a symbol table
   [T] - deprecated, use --thin instead
   [u] - update only [files] newer than archive contents
   [U] - use actual timestamps and uids/gids
   [v] - be verbose about actions taken
   [V] - display the version and exit
 )";
 
+  outs() << "OVERVIEW: LLVM Archiver\n\n"
+         << "USAGE: " + ToolName +
+                " [options] [-]<operation>[modifiers] [relpos] "
+                "[count] <archive> [files]\n"
+         << "       " + ToolName + " -M [<mri-script]\n\n";
+
+  outs() << ArOptions;
+}
+
 static void printHelpMessage() {
   if (Stem.contains_insensitive("ranlib"))
-    outs() << RanlibHelp;
+    printRanLibHelp(Stem);
   else if (Stem.contains_insensitive("ar"))
-    outs() << ArHelp;
+    printArHelp(Stem);
 }
 
 static unsigned MRILineNumber;
 static bool ParsingMRIScript;
 
 // Show the error plus the usage message, and exit.
 [[noreturn]] static void badUsage(Twine Error) {
   WithColor::error(errs(), ToolName) << Error << "\n";