Support target names containing dots in all utilities

[dankm]

Instead of using custom code to find the program name throughout the codebase, write one function as a path helper to consistently determine the program name. This globally correctly finds target names with dots in them (ie freebsd13.2).

This works by moving lld's code for stripping the .exe string from the program name to Path.h, and replaces several instances of bespoke .exe stripping with the new helper.

As an example of the old behaviour:

$ x86_64-unknown-freebsd13.2-llvm-ranlib
x86_64-unknown-freebsd13.2-llvm-ranlib: error: not ranlib, ar, lib or dlltool

The new behaviour expressed by this change:

$ x86_64-unknown-freebsd13.2-llvm-ranlib
x86_64-unknown-freebsd13.2-llvm-ranlib: error: an archive name must be specified
OVERVIEW: LLVM ranlib

Generate an index for archives

[llvmbot]

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-llvm-binary-utilities
@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-platform-windows

Changes

Instead of using custom code to find the program name throughout the codebase, write one function as a path helper to consistently determine the program name. This globally correctly finds target names with dots in them (ie freebsd13.2).

This works by moving lld's code for stripping the .exe string from the program name to Path.h, and replaces several instances of bespoke .exe stripping with the new helper.

Full diff: https://github.com/llvm/llvm-project/pull/65812.diff

16 Files Affected:

• (modified) lld/COFF/Driver.cpp (+1-1)
• (modified) lld/Common/Args.cpp (-6)
• (modified) lld/ELF/Driver.cpp (+1-1)
• (modified) lld/MachO/Driver.cpp (+1-1)
• (modified) lld/include/lld/Common/Args.h (-2)
• (modified) lld/wasm/Driver.cpp (+1-1)
• (modified) llvm/include/llvm/Support/Path.h (+15)
• (modified) llvm/lib/Support/Path.cpp (+8)
• (modified) llvm/tools/llvm-ar/llvm-ar.cpp (+2-1)
• (modified) llvm/tools/llvm-cov/llvm-cov.cpp (+1-1)
• (modified) llvm/tools/llvm-driver/llvm-driver.cpp (+1-1)
• (modified) llvm/tools/llvm-objcopy/llvm-objcopy.cpp (+1-1)
• (modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+1-1)
• (modified) llvm/tools/llvm-rc/llvm-rc.cpp (+1-1)
• (modified) llvm/tools/llvm-readobj/llvm-readobj.cpp (+1-1)
• (modified) llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp (+1-1)

diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index d7476e91e03e384..03a63bf3a306b4d 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -68,7 +68,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
   auto *ctx = new COFFLinkerContext;
 
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
-  ctx->e.logName = args::getFilenameWithoutExe(args[0]);
+  ctx->e.logName = sys::path::program_name(args[0]);
   ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now"
                                  " (use /errorlimit:0 to see all errors)";
 
diff --git a/lld/Common/Args.cpp b/lld/Common/Args.cpp
index 48c934df3a2c931..b02d023fe19aeb5 100644
--- a/lld/Common/Args.cpp
+++ b/lld/Common/Args.cpp
@@ -85,9 +85,3 @@ std::vector<StringRef> lld::args::getLines(MemoryBufferRef mb) {
   }
   return ret;
 }
-
-StringRef lld::args::getFilenameWithoutExe(StringRef path) {
-  if (path.ends_with_insensitive(".exe"))
-    return sys::path::stem(path);
-  return sys::path::filename(path);
-}
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 9219314111610d1..0eef7bbf1baa213 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -143,7 +143,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
 
     SharedFile::vernauxNum = 0;
   };
-  ctx->e.logName = args::getFilenameWithoutExe(args[0]);
+  ctx->e.logName = sys::path::program_name(args[0]);
   ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
                                  "--error-limit=0 to see all errors)";
 
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 00ff3439a043be9..ed0b66d4b2ec6ba 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -1428,7 +1428,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
     InputFile::resetIdCount();
   };
 
-  ctx->e.logName = args::getFilenameWithoutExe(argsArr[0]);
+  ctx->e.logName = sys::path::program_name(argsArr[0]);
 
   MachOOptTable parser;
   InputArgList args = parser.parse(argsArr.slice(1));
diff --git a/lld/include/lld/Common/Args.h b/lld/include/lld/Common/Args.h
index 60f83fbbbf1a3c9..8822707a6a1f212 100644
--- a/lld/include/lld/Common/Args.h
+++ b/lld/include/lld/Common/Args.h
@@ -38,8 +38,6 @@ uint64_t getZOptionValue(llvm::opt::InputArgList &args, int id, StringRef key,
 
 std::vector<StringRef> getLines(MemoryBufferRef mb);
 
-StringRef getFilenameWithoutExe(StringRef path);
-
 } // namespace args
 } // namespace lld
 
diff --git a/lld/wasm/Driver.cpp b/lld/wasm/Driver.cpp
index c2f5f0185781f36..f13aa520f35b100 100644
--- a/lld/wasm/Driver.cpp
+++ b/lld/wasm/Driver.cpp
@@ -89,7 +89,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
   auto *ctx = new CommonLinkerContext;
 
   ctx->e.initialize(stdoutOS, stderrOS, exitEarly, disableOutput);
-  ctx->e.logName = args::getFilenameWithoutExe(args[0]);
+  ctx->e.logName = sys::path::program_name(args[0]);
   ctx->e.errorLimitExceededMsg = "too many errors emitted, stopping now (use "
                                  "-error-limit=0 to see all errors)";
 
diff --git a/llvm/include/llvm/Support/Path.h b/llvm/include/llvm/Support/Path.h
index ce69f32b6cc81ba..405fdc03e020851 100644
--- a/llvm/include/llvm/Support/Path.h
+++ b/llvm/include/llvm/Support/Path.h
@@ -390,6 +390,21 @@ StringRef stem(StringRef path, Style style = Style::native);
 /// @result The extension of \a path.
 StringRef extension(StringRef path, Style style = Style::native);
 
+/// Get the program's name
+///
+/// If the path ends with the string .exe, returns the stem of
+/// \a path. Otherwise returns the filename of \a path.
+///
+/// @code
+///   /foo/prog.exe => prog
+///   /bar/prog     => prog
+///   /foo/prog1.2  => prog1.2
+/// @endcode
+///
+/// @param path Input path.
+/// @result The filename of \a path. Without any .exe component
+StringRef program_name(StringRef path, Style style = Style::native);
+
 /// Check whether the given char is a path separator on the host OS.
 ///
 /// @param value a character
diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp
index 7a57c104ef10e7b..5940c99f203c771 100644
--- a/llvm/lib/Support/Path.cpp
+++ b/llvm/lib/Support/Path.cpp
@@ -600,6 +600,14 @@ StringRef extension(StringRef path, Style style) {
   return fname.substr(pos);
 }
 
+StringRef program_name(StringRef path, Style style) {
+  // In the future this may need to be extended to other
+  // program suffixes.
+  if (path.ends_with_insensitive(".exe"))
+    return stem(path, style);
+  return filename(path, style);
+}
+
 bool is_separator(char value, Style style) {
   if (value == '/')
     return true;
diff --git a/llvm/tools/llvm-ar/llvm-ar.cpp b/llvm/tools/llvm-ar/llvm-ar.cpp
index 9889ad9de627d51..68b79cb007d1e98 100644
--- a/llvm/tools/llvm-ar/llvm-ar.cpp
+++ b/llvm/tools/llvm-ar/llvm-ar.cpp
@@ -1508,7 +1508,8 @@ int llvm_ar_main(int argc, char **argv, const llvm::ToolContext &) {
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
-  Stem = sys::path::stem(ToolName);
+  Stem = sys::path::program_name(ToolName);
+
   auto Is = [](StringRef Tool) {
     // We need to recognize the following filenames.
     //
diff --git a/llvm/tools/llvm-cov/llvm-cov.cpp b/llvm/tools/llvm-cov/llvm-cov.cpp
index 5ada55789b24677..e0b9e75f57c441f 100644
--- a/llvm/tools/llvm-cov/llvm-cov.cpp
+++ b/llvm/tools/llvm-cov/llvm-cov.cpp
@@ -59,7 +59,7 @@ int main(int argc, const char **argv) {
   InitLLVM X(argc, argv);
 
   // If argv[0] is or ends with 'gcov', always be gcov compatible
-  if (sys::path::stem(argv[0]).ends_with_insensitive("gcov"))
+  if (sys::path::program_name(argv[0]).ends_with_insensitive("gcov"))
     return gcovMain(argc, argv);
 
   // Check if we are invoking a specific tool command.
diff --git a/llvm/tools/llvm-driver/llvm-driver.cpp b/llvm/tools/llvm-driver/llvm-driver.cpp
index a0f1ca831d93b6a..ffb3bc0f3a6fdc3 100644
--- a/llvm/tools/llvm-driver/llvm-driver.cpp
+++ b/llvm/tools/llvm-driver/llvm-driver.cpp
@@ -48,7 +48,7 @@ static int findTool(int Argc, char **Argv, const char *Argv0) {
     return 0;
   }
 
-  StringRef Stem = sys::path::stem(ToolName);
+  StringRef Stem = sys::path::program_name(ToolName);
   auto Is = [=](StringRef Tool) {
     auto IsImpl = [=](StringRef Stem) {
       auto I = Stem.rfind_insensitive(Tool);
diff --git a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
index 2afa97601f5cfd8..9c51958198cb109 100644
--- a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -74,7 +74,7 @@ static ErrorSuccess reportWarning(Error E) {
 }
 
 static Expected<DriverConfig> getDriverConfig(ArrayRef<const char *> Args) {
-  StringRef Stem = sys::path::stem(ToolName);
+  StringRef Stem = sys::path::program_name(ToolName);
   auto Is = [=](StringRef Tool) {
     // We need to recognize the following filenames:
     //
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index e4e1ceb9b3f97f6..f6a8417afd72c26 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -3297,7 +3297,7 @@ int llvm_objdump_main(int argc, char **argv, const llvm::ToolContext &) {
   std::unique_ptr<CommonOptTable> T;
   OptSpecifier Unknown, HelpFlag, HelpHiddenFlag, VersionFlag;
 
-  StringRef Stem = sys::path::stem(ToolName);
+  StringRef Stem = sys::path::program_name(ToolName);
   auto Is = [=](StringRef Tool) {
     // We need to recognize the following filenames:
     //
diff --git a/llvm/tools/llvm-rc/llvm-rc.cpp b/llvm/tools/llvm-rc/llvm-rc.cpp
index b955347f2a8646e..2f93a4931f55a6e 100644
--- a/llvm/tools/llvm-rc/llvm-rc.cpp
+++ b/llvm/tools/llvm-rc/llvm-rc.cpp
@@ -276,7 +276,7 @@ void preprocess(StringRef Src, StringRef Dst, const RcOptions &Opts,
 }
 
 static std::pair<bool, std::string> isWindres(llvm::StringRef Argv0) {
-  StringRef ProgName = llvm::sys::path::stem(Argv0);
+  StringRef ProgName = sys::path::program_name(Argv0);
   // x86_64-w64-mingw32-windres -> x86_64-w64-mingw32, windres
   // llvm-rc -> "", llvm-rc
   // aarch64-w64-mingw32-llvm-windres-10.exe -> aarch64-w64-mingw32, llvm-windres
diff --git a/llvm/tools/llvm-readobj/llvm-readobj.cpp b/llvm/tools/llvm-readobj/llvm-readobj.cpp
index ca633ceff90800e..2735d31202c6e70 100644
--- a/llvm/tools/llvm-readobj/llvm-readobj.cpp
+++ b/llvm/tools/llvm-readobj/llvm-readobj.cpp
@@ -657,7 +657,7 @@ int llvm_readobj_main(int argc, char **argv, const llvm::ToolContext &) {
     return 0;
   }
 
-  if (sys::path::stem(argv[0]).contains("readelf"))
+  if (sys::path::filename(argv[0]).contains("readelf"))
     opts::Output = opts::GNU;
   parseOptions(Args);
 
diff --git a/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp b/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
index 78a0e6772f3fb36..e06a4a0b8fa4827 100644
--- a/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
+++ b/llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
@@ -432,7 +432,7 @@ int llvm_symbolizer_main(int argc, char **argv, const llvm::ToolContext &) {
   sys::InitializeCOMRAII COM(sys::COMThreadingMode::MultiThreaded);
 
   ToolName = argv[0];
-  bool IsAddr2Line = sys::path::stem(ToolName).contains("addr2line");
+  bool IsAddr2Line = sys::path::filename(ToolName).contains("addr2line");
   BumpPtrAllocator A;
   StringSaver Saver(A);
   SymbolizerOptTable Tbl;

[dankm]

That windows build check looks like a problem with its configuration, am I mistaken?

[dankm]

That windows build check looks like a problem with its configuration, am I mistaken?

I was not mistaken. #66192 fixed this, so hopefully we build now.

[tru]

Instead of merging in changes to your PR branch - please rebase instead.

[jrtc27]

You now have not one but four merge commits

[dankm]

Sure, I was misinterpreting the new github contributor guides. I'll rebase for instead.

[tru]

I think we need some tests around this. It can easily break otherwise.

[dankm]

Tests make sense. Took some doing to find the unit tests for the clang version of this code. I'll add some for these other users.

[dankm]

Hm. I'm happy with this file, but I also discovered a bunch of other tool-name tests that can be updated. I'll add those too.

[dankm]

Okay, added tool-name tests for the utilities for ELF and such. Not sure if I should add similar tests for Windows targets; they never have dots in their names.

[MaskRay]

LGTM, but @jh7370 usually wants to have an eye on such changes.

[jh7370]

Just making sure I follow: the problem with stem is that if a filename includes a dot, but no ".exe" or equivalent suffix, it would drop the bit from the last dot onwards, even though it might be part of the tool's actual name. Is that correct?

It looks like a number of tools have changed, but I don't see equivalent test changes/new tests for all of them. It seems like we should have testing for all cases where the code goes to some attempt to print the tool name.
Assuming it is, I note that some tools no longer will drop "other" file extensions. However, I don't know of any real such extensions other than Window's ".exe". Do any others exist on other platforms?

[jh7370]

I'm struggling to understand the logic in the llvm-ar test case gaining two new test cases, but the ranlib one only gaining one.

(On further investigation, I'm struggling to understand why the llvm-ranlib tool-name test even needs to exist, given that the code is the same code as llvm-ar at the relevant point).

[dankm]

Yep, your second point is why I didn't bother making the second case for llvm-ranlib. On my first pass I just updated the existing testcases I found, but for ranlib I realised the same thing you did. I may just drop the ranlib test entirely.

[jh7370]

Let's put the new test files and deletion of this old test in a different PR. The old code was untested, so we're not making things worse, but it also helps keep the PRs focused. Aside: if we're deleting this old file, I think it would be a good idea to add one or two cases to the llvm-ar test showing the "llvm-ranlib" name.

[jh7370]

WorseThanFailure doesn't describe the case particularly well, especially as the behaviour here is the same as stem. Could it simply be OnlyExe?

[dankm]

Yeah, that one was purely for my own benefit. I was describing behaviour I wasn't sure I wanted to keep, just what it was currently doing. It was intended as a reminder to me to come up 1. A better name, and 2. what the intended output should be.

[dankm]

Just making sure I follow: the problem with stem is that if a filename includes a dot, but no ".exe" or equivalent suffix, it would drop the bit from the last dot onwards, even though it might be part of the tool's actual name. Is that correct?

Yes, that's correct. The real examples I've run into have been x86_64-unknown-freebsd13.2-clang, fixed in D135284, and the llvm-ar and llvm-ranlib cases in this PR. In both cases they had previously been using stem and returning x86_64-unknown-freebsd13 and then failing to determine the tool name. I was mostly concerned with the cases where behaviour was affected. Tool name printing is valid too, though, of course.

It looks like a number of tools have changed, but I don't see equivalent test changes/new tests for all of them. It seems like we should have testing for all cases where the code goes to some attempt to print the tool name. Assuming it is, I note that some tools no longer will drop "other" file extensions. However, I don't know of any real such extensions other than Window's ".exe". Do any others exist on other platforms?

This makes sense. For my first round I did leave out the cases where having multiple dots in the tool name didn't make sense, such as for dlltool or llvm-rc. You're right though that we should test those. I'll add more tests, and see if I can find more examples of using stem or filename when printing a tool name.

[dankm]

Do any others exist on other platforms?

I don't know. I explicitly tested that .sh remains, but that was just because it's relatively common (in my experience) to see tools like clang wrapped in a shell script. On Windows there are several executable extensions but I think the only one that makes sense in our use case is .exe, though .ex_ might also be a possibility.

[jh7370]

Force pushing (and therefore rebasing) is supposed to be avoided unless it's absolutely necessary. Was a rebase actually required for this change? One of the motivations to avoid force pushes is that I can no longer see what changed versus my previous review, which is a fairly major usability issue for reviewing. Fortunately, this is a small review...

By the way, did you mean to have two commits in your PR?

LGTM, aside from a comment on the TODO.

[jh7370]

Let's put the new test files and deletion of this old test in a different PR. The old code was untested, so we're not making things worse, but it also helps keep the PRs focused. Aside: if we're deleting this old file, I think it would be a good idea to add one or two cases to the llvm-ar test showing the "llvm-ranlib" name.

[jh7370]

@dankm, is there a particular reason you haven't merged this change in yet?

FWIW, the formatter check failed for some reason, but I'm not sure it's related to any formatting issue. Please verify by running clang-format on your changes before merging.

[dankm]

No good particular reason. I got hung up on the formatting issues then ran out of steam, and busy with $job. I just ran clang-format on this change and it came up clean.

And now that I've done that the only reason I have left is I'm unable to merge my own changes. Would you mind, @jh7370?

[dankm]

Hm. I have "fixup" commits in this branch, should I rebase those, or can we squash merge as-is?

[jh7370]

I can merge it for you, but first I just want to confirm that all 4 commits in this PR are intended to be for the PR? If not, you'll need to rebase and force push.

There's no need for you to manually squash the fixups (although you might as well if you do end up rebasing). When I do the merge via the UI, it'll be using the Squash and merge button, which squashes everything into a single commit.

The final commit message will be the PR description. I found the description a little confusing at first, without reading the code and test, so it may be beneficial to add an example of the before and after behaviour to that description before I merge it. Please could you update the patch description accordingly.

[dankm]

Sounds good, @jh7370. Everything I want is in the pull request, I'm going to create a new pull request to update some tests as we discussed above. I'll update the description to be a bit more clear.

Thanks for all help.