Properly handle short file names on the command line in Windows

[amccarth]

Summary

This addresses: https://llvm.org/bugs/show_bug.cgi?id=28089

Some build systems use the short (8.3) file names on Windows, especially if the path has spaces in it. The shortening made it impossible for clang to distinguish between clang.exe, clang++.exe, and clang-cl.exe. So, in addition to expanding wildcards, the code now replaces short names with long ones.

Tested with ninja check-clang.

[majnemer]

Any reason not to perform the call to ExpandShortFileName in GetArgumentVector? I think we only need to do this for the first argument.

[amccarth]

In D21420#459506, @majnemer wrote:

Any reason not to perform the call to ExpandShortFileName in GetArgumentVector? I think we only need to do this for the first argument.

I was trying to be complete by treating all the arguments the same way. Note that the existing code calls WildcardExpand on the first argument, even though that one seems really unlikely to have wildcards. And, if it does, you need to expand wild cards before expanding the short file names.

That said, I'm open to treating the first argument as a special case if you can convince me it is indeed special.

[majnemer]

In D21420#459524, @amccarth wrote:

In D21420#459506, @majnemer wrote:

Any reason not to perform the call to ExpandShortFileName in GetArgumentVector? I think we only need to do this for the first argument.

I was trying to be complete by treating all the arguments the same way. Note that the existing code calls WildcardExpand on the first argument, even though that one seems really unlikely to have wildcards. And, if it does, you need to expand wild cards before expanding the short file names.

That said, I'm open to treating the first argument as a special case if you can convince me it is indeed special.

Here's my thinking:
argv[0] is special in that the compilers behavior is dependent on data before the extension suffix (.exe, etc.)
The compiler's behavior for the other argv[1..n] depends on the filename to suss out what mode the compiler should be in, this is not dependent on data before the suffix.

As an aside, I think it's quite strange that we look for wildcards in argv[0].

[amccarth]

Per the discussion, this version expands a short file name only in the first argument and applies the wildcard expansions only in the remaining arguments.

[majnemer]

LGTM w/ GetLongPathNameW > MAX_PATH addressed.

lib/Support/Windows/Process.inc

236–237

Is an error code set for the case where Length > MAX_PATH? The MSDN docs seemed a little ambiguous.

[amccarth]

lib/Support/Windows/Process.inc

236–237

The way I read it is that, if the target buffer isn't big enough, then the return value is the necessary size (in code units). So I checked to see if it's bigger than MAX_PATH, since that's the capacity of the buffer.

In order to extend the buffer beyond MAX_PATH, we'd get into the realm of the \\?\ prefix nonsense. I assume there's lot of code in here that doesn't handle those, so if the expansion won't fit within MAX_PATH, I'm treating it as an error.

I believe GetLastError will return ERROR_NOT_ENOUGH_MEMORY or some other appropriate value in that case. I'll check that now and add a comment.

[amccarth]

Better error handling if the long name is longer than MAX_PATH.

[amccarth]

lib/Support/Windows/Process.inc

267–268

GetLastError was useless. If Length > LongName.capacity(), then the path in LongName is truncated and GetLastError returns ERROR_SUCCESS. So I've broken this case out and returned a more specific error code.

lib/Support/Windows/Process.inc

ConvertAndPushArg(const wchar_t *Arg, SmallVectorImpl<const char *> &Args,
                   SpecificBumpPtrAllocator<char> &Allocator) {
   SmallVector<char, MAX_PATH> ArgString;
   if (std::error_code ec = windows::UTF16ToUTF8(Arg, wcslen(Arg), ArgString))
     return ec;
   AllocateAndPush(ArgString, Args, Allocator);
   return std::error_code();
 }
 
+static std::error_code
+ExpandShortFileName(const wchar_t *Arg, SmallVectorImpl<const char *> &Args,
+                    SpecificBumpPtrAllocator<char> &Allocator) {
+  SmallVector<wchar_t, MAX_PATH> LongPath;
+  DWORD Length = GetLongPathNameW(Arg, LongPath.data(), LongPath.capacity());
+  if (Length == 0) {
+    DWORD LastError = GetLastError();
+    if (LastError == ERROR_INVALID_NAME ||
+        LastError == ERROR_FILE_NOT_FOUND ||
+        LastError == ERROR_PATH_NOT_FOUND) {
+      // This argument is probably not a file name.
+      return ConvertAndPushArg(Arg, Args, Allocator);
+    }
+    return mapWindowsError(LastError);
+  } else if (Length > MAX_PATH)
+    return mapWindowsError(GetLastError());
+  LongPath.set_size(Length);
+
+  return ConvertAndPushArg(LongPath.data(), Args, Allocator);
+}
+
 /// \brief Perform wildcard expansion of Arg, or just push it into Args if it
 /// doesn't have wildcards or doesn't match any files.
 static std::error_code
 WildcardExpand(const wchar_t *Arg, SmallVectorImpl<const char *> &Args,
                SpecificBumpPtrAllocator<char> &Allocator) {
   if (!wcspbrk(Arg, L"*?")) {
     // Arg does not contain any wildcard characters. This is the common case.
-    return ConvertAndPushArg(Arg, Args, Allocator);
+    return ExpandShortFileName(Arg, Args, Allocator);
   }
 
   if (wcscmp(Arg, L"/?") == 0 || wcscmp(Arg, L"-?") == 0) {
     // Don't wildcard expand /?. Always treat it as an option.
-    return ConvertAndPushArg(Arg, Args, Allocator);
+    return ExpandShortFileName(Arg, Args, Allocator);
   }
 
   // Extract any directory part of the argument.
   SmallVector<char, MAX_PATH> Dir;
   if (std::error_code ec = windows::UTF16ToUTF8(Arg, wcslen(Arg), Dir))
     return ec;
   sys::path::remove_filename(Dir);
   const int DirSize = Dir.size();
 
   // Search for matching files.
+  // FIXME:  This assumes the wildcard is only in the file name and not in the
+  // directory portion of the file path.  For example, it doesn't handle
+  // "*\foo.c" nor "s?c\bar.cpp".
   WIN32_FIND_DATAW FileData;
   HANDLE FindHandle = FindFirstFileW(Arg, &FileData);
   if (FindHandle == INVALID_HANDLE_VALUE) {
     return ConvertAndPushArg(Arg, Args, Allocator);
   }
 
   std::error_code ec;
   do {
     SmallVector<char, MAX_PATH> FileName;
     ec = windows::UTF16ToUTF8(FileData.cFileName, wcslen(FileData.cFileName),
                               FileName);
     if (ec)
       break;
 
-    // Push the filename onto Dir, and remove it afterwards.
+    // Append FileName to Dir, and remove it afterwards.
     llvm::sys::path::append(Dir, StringRef(FileName.data(), FileName.size()));
-    AllocateAndPush(Dir, Args, Allocator);
+    // Now we have a file spec in Dir, but it's been prematurely converted to
+    // UTF-8.  In order to expand the short file names, we need to once again
+    // convert to UTF-16.
+    SmallVector<wchar_t, MAX_PATH> WideFileSpec;
+    ec = windows::UTF8ToUTF16(llvm::StringRef(Dir.data(), Dir.size()),
+                              WideFileSpec);
+    if (ec)
+      break;
+    ec = ExpandShortFileName(WideFileSpec.data(), Args, Allocator);
+    if (ec)
+      break;
     Dir.resize(DirSize);
   } while (FindNextFileW(FindHandle, &FileData));
 
   FindClose(FindHandle);
   return ec;
 }
 
 std::error_code
 Process::GetArgumentVector(SmallVectorImpl<const char *> &Args,
                            ArrayRef<const char *>,
                            SpecificBumpPtrAllocator<char> &ArgAllocator) {
   int ArgCount;
   wchar_t **UnicodeCommandLine =
       CommandLineToArgvW(GetCommandLineW(), &ArgCount);
   if (!UnicodeCommandLine)
     return mapWindowsError(::GetLastError());
 
   Args.reserve(ArgCount);
   std::error_code ec;
 
   for (int i = 0; i < ArgCount; ++i) {
     ec = WildcardExpand(UnicodeCommandLine[i], Args, ArgAllocator);