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

324–325

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

324–325

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.

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
#ifdef HAVE__CHSIZE_S
   errno_t error = ::_chsize_s(FD, Size);
 #else
   errno_t error = ::_chsize(FD, Size);
 #endif
   return std::error_code(error, std::generic_category());
 }
 
 std::error_code access(const Twine &Path, AccessMode Mode) {
+  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, ";");
+
+  DWORD Attributes = INVALID_FILE_ATTRIBUTES;
+
+  for (StringRef Ext : PathExts) {
     SmallVector<wchar_t, 128> PathUtf16;
 
-  if (std::error_code EC = widenPath(Path, PathUtf16))
+    if (std::error_code EC = widenPath(Path + Ext, PathUtf16))
       return EC;
 
-  DWORD Attributes = ::GetFileAttributesW(PathUtf16.begin());
+    Attributes = ::GetFileAttributesW(PathUtf16.begin());
+
+    if (Attributes != INVALID_FILE_ATTRIBUTES)
+      // the file exists
+      break;
+  }
 
   if (Attributes == INVALID_FILE_ATTRIBUTES) {
     // See if the file didn't actually exist.
     DWORD LastError = ::GetLastError();
     if (LastError != ERROR_FILE_NOT_FOUND &&
         LastError != ERROR_PATH_NOT_FOUND)
       return mapWindowsError(LastError);
     return errc::no_such_file_or_directory;
   }
 
   if (Mode == AccessMode::Write && (Attributes & FILE_ATTRIBUTE_READONLY))
     return errc::permission_denied;
 
   return std::error_code();
 }
 
 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 &&
          A.VolumeSerialNumber == B.VolumeSerialNumber;
 }
 
 std::error_code equivalent(const Twine &A, const Twine &B, bool &result) {
   file_status fsA, fsB;
   if (std::error_code ec = status(A, fsA))