ScanDirForExecutable on Windows fails to find executables with the "exe" extension in name

[iid_iunknown]

Summary

Hello Reid

This is a patch for the problem we discussed some time ago. Would you be able to take a look and share your opinion, please?
Thank you.

Description

When the driver tries to locate a program by its name, e.g. a linker, it scans the paths provided by the toolchain using the ScanDirForExecutable function. If the lookup fails, the driver uses llvm::sys::findProgramByName. Unlike llvm::sys::findProgramByName, ScanDirForExecutable is not aware of file extensions. If the program has the "exe" extension in its name, which is very common on Windows, ScanDirForExecutable won't find it under the toolchain-provided paths.

This patch changes the Windows version of the "access" function called by ScanDirForExecutable to respect file extensions, similarly to llvm::sys::findProgramByName.

[rnk]

I'm not sure I like this approach. This makes the following code do the wrong thing when foo.exe exists but foo doesn't:

if (exists("foo"))
  // try to read it and fail

I think a better approach would be to change only can_execute to deal with PATHEXT.

[iid_iunknown]

In D12711#242038, @rnk wrote:

This makes the following code do the wrong thing...

Thank you for your remark! Yes, you are right. I will update the patch.

[iid_iunknown]

The patch updated according to the Reid's remarks. File extensions are now handled by sys::fs::can_execute.

[rnk]

Seems pretty close. Do you need someone to commit this for you?

lib/Support/Windows/Path.inc

310–311

I think can_execute should really be tied to what llvm::sys::ExecteAndWait can run. ExecuteAndWait uses CreateProcess, which ignores PATHEXT, but does check for files ending in .exe. So, if possible, I would simplify this whole thing to:

return !access(Path, AccessMode::Execute) || !access(Path + ".exe", AccessMode::Execute);

[iid_iunknown]

In D12711#242494, @rnk wrote:

Do you need someone to commit this for you?

Yes.

lib/Support/Windows/Path.inc

310–311

Yes, other extensions are not that common for Windows executables. Anyway, it would be fine for my case.
Does it mean findProgramByName may be simplified in the same way?

[iid_iunknown]

sys::fs::can_execute now ignores PATHEXT similarly to ExecuteAndWait. The "exe" extension is used only.

include/llvm/Support/FileSystem.h

 inline bool exists(const Twine &Path) {
   return !access(Path, AccessMode::Exist);
 }
 
 /// @brief Can we execute this file?
 ///
 /// @param Path Input path.
 /// @returns True if we can execute it, false otherwise.
-inline bool can_execute(const Twine &Path) {
-  return !access(Path, AccessMode::Execute);
-}
+bool can_execute(const Twine &Path);
 
 /// @brief Can we write this file?
 ///
 /// @param Path Input path.
 /// @returns True if we can write to it, false otherwise.
 inline bool can_write(const Twine &Path) {
   return !access(Path, AccessMode::Write);
 }

lib/Support/Unix/Path.inc

if (0 != stat(P.begin(), &buf))
       return errc::permission_denied;
     if (!S_ISREG(buf.st_mode))
       return errc::permission_denied;
   }
 
   return std::error_code();
 }
 
+bool can_execute(const Twine &Path) {
+  return !access(Path, AccessMode::Execute);
+}
+
 bool equivalent(file_status A, file_status B) {
   assert(status_known(A) && status_known(B));
   return A.fs_st_dev == B.fs_st_dev &&
          A.fs_st_ino == B.fs_st_ino;
 }
 
 std::error_code equivalent(const Twine &A, const Twine &B, bool &result) {
   file_status fsA, fsB;

lib/Support/Windows/Path.inc

 //===----------------------------------------------------------------------===//
 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only generic Windows code that
 //===          is guaranteed to work on *all* Windows variants.
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/WindowsError.h"
 #include <fcntl.h>
 #include <io.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 
 // These two headers must be included last, and make sure shlobj is required
 // after Windows.h to make sure it picks up our definition of _WIN32_WINNT
std::error_code access(const Twine &Path, AccessMode Mode) {
   }
 
   if (Mode == AccessMode::Write && (Attributes & FILE_ATTRIBUTE_READONLY))
     return errc::permission_denied;
 
   return std::error_code();
 }
 
+bool can_execute(const Twine &Path) {
+  SmallVector<StringRef, 12> PathExts;
+  PathExts.push_back("");
+  PathExts.push_back(".exe"); // FIXME: This must be in %PATHEXT%.
+  if (const char *PathExtEnv = std::getenv("PATHEXT"))
+    SplitString(PathExtEnv, PathExts, ";");
+
+  for (StringRef Ext : PathExts) {
+    if (!access(Path + Ext, AccessMode::Execute))
+      return true;
+  }
+
+  return false;
+}
+
 bool equivalent(file_status A, file_status B) {
   assert(status_known(A) && status_known(B));
   return A.FileIndexHigh      == B.FileIndexHigh &&
          A.FileIndexLow       == B.FileIndexLow &&
          A.FileSizeHigh       == B.FileSizeHigh &&
          A.FileSizeLow        == B.FileSizeLow &&
          A.LastWriteTimeHigh  == B.LastWriteTimeHigh &&
          A.LastWriteTimeLow   == B.LastWriteTimeLow &&