[Support] Avoid calling CommandLineToArgvW from shell32.dll

[rnk]

Summary

Shell32.dll depends on gdi32.dll and user32.dll, which are mostly DLLs
for Windows GUI functionality. LLVM's utilities don't typically need GUI
functionality, and loading these DLLs seems to be slowing down startup.
Also, we already have an implementation of Windows command line
tokenization in cl::TokenizeWindowsCommandLine, so we can just use it.

The goal is to get the original argv in UTF-8, so that it can pass
through most LLVM string APIs. A Windows process starts life with a
UTF-16 string for its command line, and it can be retreived with
GetCommandLineW from kernel32.dll.

Previously, we would:

1. Get the wide command line
2. Call CommandLineToArgvW to handle quoting rules and separate it into arguments.
3. For each wide argument, expand wildcards (* and ?) using FindFirstFileW.
4. Convert each argument to UTF-8

Now we:

1. Get the wide command line, convert the whole thing to UTF-8
2. Tokenize the UTF-8 command line with cl::TokenizeWindowsCommandLine
3. For each argument, expand wildcards if present
   • This requires converting back to UTF-16 to call FindFirstFileW
   • Results of FindFirstFileW must be converted back to UTF-8

[zturner]

When you commit this, make sure to include in the commit message how much speedup you're seeing as a result of this.

llvm/lib/Support/Windows/Process.inc

147 ↗

(On Diff #164927)

Can you make the first arg a StringRef?

150 ↗

(On Diff #164927)

EC?

155 ↗

(On Diff #164927)

Then you can write this as if (Arg.find_first_of("*?") == StringRef::npos || Arg == "/?" || Arg == "-?")

(one could argue we should have a function called StringRef::contains_any_of(StringRef Chars)). In any case, even though it's a bit longer I find the additional clarity from meaningful function names better than cryptic names like strpbrk which I have to look up on cppreference every time I see it.

179 ↗

(On Diff #164927)

This can just be a SmallString<MAX_PATH>

185 ↗

(On Diff #164927)

I think this can just be a SmallString<MAX_PATH>

192 ↗

(On Diff #164927)

And you can just write append(Dir, FileName);

[rnk]

In D51941#1230893, @zturner wrote:

When you commit this, make sure to include in the commit message how much speedup you're seeing as a result of this.

There is no speedup without adding /delayload:shell32.dll, which will be a separate patch.

[rnk]

• Be more string-y

[rnk]

Unfortunately, it looks like this changed the behavior of newlines embedded in arguments. When parsing response files, we want to treat \n as whitespace. When parsing a command line, CommandLineToArgvW apparently does not. The documentation says it separates arguments on "whitespace characters", but it doesn't say what it considers to be whitespace. Apparently \n is not whitespace.

In the meantime, I made LLVM quote newlines in arguments to fix the clang tests that relied on this behavior in rL341992.

llvm/trunk/lib/Support/Windows/Process.inc

 //===- Win32/Process.cpp - Win32 Process Implementation ------- -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides the Win32 specific implementation of the Process class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WindowsError.h"
 #include <malloc.h>
 
 // The Windows.h header must be after LLVM and standard headers.
 #include "WindowsSupport.h"
 
 #include <direct.h>
 #include <io.h>
Optional<std::string> Process::GetEnv(StringRef Name) {
 
   // Convert the result from UTF-16 to UTF-8.
   SmallVector<char, MAX_PATH> Res;
   if (windows::UTF16ToUTF8(Buf.data(), Size, Res))
     return None;
   return std::string(Res.data());
 }
 
-static const char *AllocateString(const SmallVectorImpl<char> &S,
-                                  BumpPtrAllocator &Alloc) {
-  char *Buf = reinterpret_cast<char *>(Alloc.Allocate(S.size() + 1, 1));
-  ::memcpy(Buf, S.data(), S.size());
-  Buf[S.size()] = '\0';
-  return Buf;
-}
-
-/// Convert Arg from UTF-16 to UTF-8 and push it onto Args.
-static std::error_code ConvertAndPushArg(const wchar_t *Arg,
-                                         SmallVectorImpl<const char *> &Args,
-                                         BumpPtrAllocator &Alloc) {
-  SmallVector<char, MAX_PATH> ArgString;
-  if (std::error_code ec = windows::UTF16ToUTF8(Arg, wcslen(Arg), ArgString))
-    return ec;
-  Args.push_back(AllocateString(ArgString, Alloc));
-  return std::error_code();
-}
-
-/// 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,
+/// 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(StringRef Arg,
                                       SmallVectorImpl<const char *> &Args,
-                                      BumpPtrAllocator &Alloc) {
-  if (!wcspbrk(Arg, L"*?")) {
-    // Arg does not contain any wildcard characters. This is the common case.
-    return ConvertAndPushArg(Arg, Args, Alloc);
-  }
+                                      StringSaver &Saver) {
+  std::error_code EC;
 
-  if (wcscmp(Arg, L"/?") == 0 || wcscmp(Arg, L"-?") == 0) {
-    // Don't wildcard expand /?. Always treat it as an option.
-    return ConvertAndPushArg(Arg, Args, Alloc);
+  // Don't expand Arg if it does not contain any wildcard characters. This is
+  // the common case. Also don't wildcard expand /?. Always treat it as an
+  // option.
+  if (Arg.find_first_of("*?") == StringRef::npos || Arg == "/?" ||
+      Arg == "-?") {
+    Args.push_back(Arg.data());
+    return EC;
   }
 
-  // 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();
+  // Convert back to UTF-16 so we can call FindFirstFileW.
+  SmallVector<wchar_t, MAX_PATH> ArgW;
+  EC = windows::UTF8ToUTF16(Arg, ArgW);
+  if (EC)
+    return EC;
 
   // 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);
+  HANDLE FindHandle = FindFirstFileW(ArgW.data(), &FileData);
   if (FindHandle == INVALID_HANDLE_VALUE) {
-    return ConvertAndPushArg(Arg, Args, Alloc);
+    Args.push_back(Arg.data());
+    return EC;
   }
 
-  std::error_code ec;
+  // Extract any directory part of the argument.
+  SmallString<MAX_PATH> Dir = Arg;
+  sys::path::remove_filename(Dir);
+  const int DirSize = Dir.size();
+
   do {
-    SmallVector<char, MAX_PATH> FileName;
-    ec = windows::UTF16ToUTF8(FileData.cFileName, wcslen(FileData.cFileName),
+    SmallString<MAX_PATH> FileName;
+    EC = windows::UTF16ToUTF8(FileData.cFileName, wcslen(FileData.cFileName),
                               FileName);
-    if (ec)
+    if (EC)
       break;
 
     // Append FileName to Dir, and remove it afterwards.
-    llvm::sys::path::append(Dir, StringRef(FileName.data(), FileName.size()));
-    Args.push_back(AllocateString(Dir, Alloc));
+    llvm::sys::path::append(Dir, FileName);
+    Args.push_back(Saver.save(StringRef(Dir)).data());
     Dir.resize(DirSize);
   } while (FindNextFileW(FindHandle, &FileData));
 
   FindClose(FindHandle);
-  return ec;
+  return EC;
 }
 
 static std::error_code GetExecutableName(SmallVectorImpl<char> &Filename) {
   // The first argument may contain just the name of the executable (e.g.,
   // "clang") rather than the full path, so swap it with the full path.
   wchar_t ModuleName[MAX_PATH];
   size_t Length = ::GetModuleFileNameW(NULL, ModuleName, MAX_PATH);
   if (Length == 0 || Length == MAX_PATH) {
static std::error_code GetExecutableName(SmallVectorImpl<char> &Filename) {
   StringRef Base = sys::path::filename(Filename.data());
   Filename.assign(Base.begin(), Base.end());
   return std::error_code();
 }
 
 std::error_code
 windows::GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
                                  BumpPtrAllocator &Alloc) {
-  int ArgCount;
-  std::unique_ptr<wchar_t *[], decltype(&LocalFree)> UnicodeCommandLine{
-    CommandLineToArgvW(GetCommandLineW(), &ArgCount), &LocalFree};
-  if (!UnicodeCommandLine)
-    return mapWindowsError(::GetLastError());
-
+  const wchar_t *CmdW = GetCommandLineW();
+  assert(CmdW);
   std::error_code EC;
+  SmallString<MAX_PATH> Cmd;
+  EC = windows::UTF16ToUTF8(CmdW, wcslen(CmdW), Cmd);
+  if (EC)
+    return EC;
 
-  Args.reserve(ArgCount);
+  SmallVector<const char *, 20> TmpArgs;
+  StringSaver Saver(Alloc);
+  cl::TokenizeWindowsCommandLine(Cmd, Saver, TmpArgs, /*MarkEOLs=*/false);
 
-  for (int I = 0; I < ArgCount; ++I) {
-    EC = WildcardExpand(UnicodeCommandLine[I], Args, Alloc);
+  for (const char *Arg : TmpArgs) {
+    EC = WildcardExpand(Arg, Args, Saver);
     if (EC)
       return EC;
   }
 
   SmallVector<char, MAX_PATH> Arg0(Args[0], Args[0] + strlen(Args[0]));
   SmallVector<char, MAX_PATH> Filename;
   sys::path::remove_filename(Arg0);
   EC = GetExecutableName(Filename);
   if (EC)
     return EC;
   sys::path::append(Arg0, Filename);
-  Args[0] = AllocateString(Arg0, Alloc);
+  Args[0] = Saver.save(Arg0).data();
   return std::error_code();
 }
 
 std::error_code Process::FixupStandardFileDescriptors() {
   return std::error_code();
 }
 
 std::error_code Process::SafelyCloseFileDescriptor(int FD) {