Rename sys::Process::GetArgumentVector -> sys::windows::GetCommandLineArguments

[ruiu]

Summary

GetArgumentVector (or GetCommandLineArguments) is very Windows-specific.
I think it doesn't make much sense to provide that function from sys::Process.

I also made a change so that the function takes a BumpPtrAllocator
instead of SpecificBumpPtrAllocator. The latter is the class to call
dtors, but since char * is trivially destructible, we should use the
former class.

[stella.stamenova]

My concern with this change is that once GetArgumentVector no longer exists, any developer who wants to retrieve the arguments as UTF8 on all platforms will need to be aware that there is a windows-specific function needed to do so. By having GetArgumentVector in sys::Process, we have a unified way of retrieving arguments on all platforms and if in the future it was necessary to specialize on say, Android, the interface wouldn't have to change. I dislike the "GetArgumentVector" name because it is rather unclear what it does (and likely the reason why it was not uniformly used), but I do think it should stay in sys::process.

llvm/lib/Support/Windows/Process.inc

142 ↗

(On Diff #142473)

AllocateString seems like a better name considering what the function does.

[zturner]

So I think one option here that maybe solves both problems at once is to create two functions in sys::Process.

One called sys::Process::SaveCommandLineArguments and another called sys::Process::GetSavedCommandLineArguments.

In the second one, you assert if the first one has not been called.

Then, call the first one from InitLLVM::InitLLVM constructor. Have the second one not require the argument vector to be passed in, instead save it in a global in sys::Process or something.

This way, anyone using InitLLVM (which should be everyone) has to do nothing, and all command line arguments are available from anywhere in the program, without needing to pass in the args from main. This way it is not Windows specific, and provides value everywhere.

Thoughts?

[stella.stamenova]

In D45641#1068965, @zturner wrote:

So I think one option here that maybe solves both problems at once is to create two functions in sys::Process.

Then, call the first one from InitLLVM::InitLLVM constructor. Have the second one not require the argument vector to be passed in, instead save it in a global in sys::Process or something.

This way, anyone using InitLLVM (which should be everyone) has to do nothing, and all command line arguments are available from anywhere in the program, without needing to pass in the args from main. This way it is not Windows specific, and provides value everywhere.

Do we frequently need to get the arguments outside of the main() function? If I recall correctly, most of the tools today have argument processing only in main(), so a function like sys::Process::GetSavedCommandLineArguments wouldn't provide value outside of main(). It's probably also likely that if there were globals in sys::Process for the arguments, people would try to use them directly (assuming they were accessible), rather than calling the function anyway.

@zturner, what scenario do you see where sys::Process::GetSavedCommandLineArguments would be used (other than in main() or InitLLVM)?

[ruiu]

In D45641#1068962, @stella.stamenova wrote:

My concern with this change is that once GetArgumentVector no longer exists, any developer who wants to retrieve the arguments as UTF8 on all platforms will need to be aware that there is a windows-specific function needed to do so. By having GetArgumentVector in sys::Process, we have a unified way of retrieving arguments on all platforms and if in the future it was necessary to specialize on say, Android, the interface wouldn't have to change. I dislike the "GetArgumentVector" name because it is rather unclear what it does (and likely the reason why it was not uniformly used), but I do think it should stay in sys::process.

Does Android really need this functionality? As far as I know, this is actually Windows specific, and the only user of this functionality is InitLLVM at the moment (because I eliminated all the other uses.) The reason why Windows needs this is historical (this is developed in 90s), and it is hard to imagine that some new system does the same mistake again in the future. So I'm not interested in generalizing this to cover non-Windows system. If something is used only by one .cpp file, that feature doesn't (and perhaps shouldn't) live in llvm::sys namespace.

[stella.stamenova]

Android doesn't currently need this, but I can see this being needed on other platforms in the future and I think there is value in having a single point where you can guarantee to get normalized command line arguments on any platform (by exposing something like GetArgumentVector).

That said, since it is only used in InitLLVM right now and InitLLVM should be included in any application, I am convinced InitLLVM can replace that single point for getting normalized command line arguments. Do please rename SaveString, but other than that, it looks good.

[ruiu]

• address a review comment

[stella.stamenova]

LGTM.

llvm/trunk/include/llvm/Support/InitLLVM.h

 public:
   InitLLVM(int &Argc, const char **&Argv);
   InitLLVM(int &Argc, char **&Argv)
       : InitLLVM(Argc, const_cast<const char **&>(Argv)) {}
 
   ~InitLLVM();
 
 private:
-  SpecificBumpPtrAllocator<char> Alloc;
+  BumpPtrAllocator Alloc;
   SmallVector<const char *, 0> Args;
   PrettyStackTraceProgram StackPrinter;
 };
 } // namespace llvm
 
 #endif

llvm/trunk/include/llvm/Support/Process.h

public:
   /// considered.
   static Optional<std::string> FindInEnvPath(StringRef EnvName,
                                              StringRef FileName,
                                              ArrayRef<std::string> IgnoreList);
 
   static Optional<std::string> FindInEnvPath(StringRef EnvName,
                                              StringRef FileName);
 
-  /// This function returns a SmallVector containing the arguments passed from
-  /// the operating system to the program.  This function expects to be handed
-  /// the vector passed in from main.
-  static std::error_code
-  GetArgumentVector(SmallVectorImpl<const char *> &Args,
-                    ArrayRef<const char *> ArgsFromMain,
-                    SpecificBumpPtrAllocator<char> &ArgAllocator);
-
   // This functions ensures that the standard file descriptors (input, output,
   // and error) are properly mapped to a file descriptor before we use any of
   // them.  This should only be called by standalone programs, library
   // components should not call this.
   static std::error_code FixupStandardFileDescriptors();
 
   // This function safely closes a file descriptor.  It is not safe to retry
   // close(2) when it returns with errno equivalent to EINTR; this is because

llvm/trunk/lib/Support/InitLLVM.cpp

 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include <string>
 
+#ifdef _WIN32
+#include "Windows/WindowsSupport.h"
+#endif
+
 using namespace llvm;
+using namespace llvm::sys;
 
 InitLLVM::InitLLVM(int &Argc, const char **&Argv) : StackPrinter(Argc, Argv) {
   sys::PrintStackTraceOnErrorSignal(Argv[0]);
 
 #ifdef _WIN32
   // We use UTF-8 as the internal character encoding. On Windows,
   // arguments passed to main() may not be encoded in UTF-8. In order
   // to reliably detect encoding of command line arguments, we use an
   // Windows API to obtain arguments, convert them to UTF-8, and then
   // write them back to the Argv vector.
   //
   // There's probably other way to do the same thing (e.g. using
   // wmain() instead of main()), but this way seems less intrusive
   // than that.
   std::string Banner = std::string(Argv[0]) + ": ";
   ExitOnError ExitOnErr(Banner);
 
-  ExitOnErr(errorCodeToError(
-      sys::Process::GetArgumentVector(Args, makeArrayRef(Argv, Argc), Alloc)));
+  ExitOnErr(errorCodeToError(windows::GetCommandLineArguments(Args, Alloc)));
 
-  // GetArgumentVector doesn't terminate the vector with a nullptr.
-  // Do it to make it compatible with the real argv.
+  // GetCommandLineArguments doesn't terminate the vector with a
+  // nullptr.  Do it to make it compatible with the real argv.
   Args.push_back(nullptr);
 
   Argc = Args.size() - 1;
   Argv = Args.data();
 #endif
 }
 
 InitLLVM::~InitLLVM() { llvm_shutdown(); }

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

 Optional<std::string> Process::GetEnv(StringRef Name) {
   std::string NameStr = Name.str();
   const char *Val = ::getenv(NameStr.c_str());
   if (!Val)
     return None;
   return std::string(Val);
 }
 
-std::error_code
-Process::GetArgumentVector(SmallVectorImpl<const char *> &ArgsOut,
-                           ArrayRef<const char *> ArgsIn,
-                           SpecificBumpPtrAllocator<char> &) {
-  ArgsOut.append(ArgsIn.begin(), ArgsIn.end());
-
-  return std::error_code();
-}
-
 namespace {
 class FDCloser {
 public:
   FDCloser(int &FD) : FD(FD), KeepOpen(false) {}
   void keepOpen() { KeepOpen = true; }
   ~FDCloser() {
     if (!KeepOpen && FD >= 0)
       ::close(FD);

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

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 void AllocateAndPush(const SmallVectorImpl<char> &S,
-                            SmallVectorImpl<const char *> &Vector,
-                            SpecificBumpPtrAllocator<char> &Allocator) {
-  char *Buffer = Allocator.Allocate(S.size() + 1);
-  ::memcpy(Buffer, S.data(), S.size());
-  Buffer[S.size()] = '\0';
-  Vector.push_back(Buffer);
+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,
-                  SpecificBumpPtrAllocator<char> &Allocator) {
+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;
-  AllocateAndPush(ArgString, Args, Allocator);
+  Args.push_back(AllocateString(ArgString, Alloc));
   return std::error_code();
 }
 
 /// \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) {
+static std::error_code WildcardExpand(const wchar_t *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, Allocator);
+    return ConvertAndPushArg(Arg, Args, Alloc);
   }
 
   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 ConvertAndPushArg(Arg, Args, Alloc);
   }
 
   // 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);
+    return ConvertAndPushArg(Arg, Args, Alloc);
   }
 
   std::error_code ec;
   do {
     SmallVector<char, MAX_PATH> FileName;
     ec = windows::UTF16ToUTF8(FileData.cFileName, wcslen(FileData.cFileName),
                               FileName);
     if (ec)
       break;
 
     // Append FileName to Dir, and remove it afterwards.
     llvm::sys::path::append(Dir, StringRef(FileName.data(), FileName.size()));
-    AllocateAndPush(Dir, Args, Allocator);
+    Args.push_back(AllocateString(Dir, Alloc));
     Dir.resize(DirSize);
   } while (FindNextFileW(FindHandle, &FileData));
 
   FindClose(FindHandle);
   return ec;
 }
 
-static std::error_code
-ExpandShortFileName(const wchar_t *Arg, SmallVectorImpl<const char *> &Args,
-                    SpecificBumpPtrAllocator<char> &Allocator) {
+static std::error_code ExpandShortFileName(const wchar_t *Arg,
+                                           SmallVectorImpl<const char *> &Args,
+                                           BumpPtrAllocator &Alloc) {
   SmallVector<wchar_t, MAX_PATH> LongPath;
   DWORD Length = GetLongPathNameW(Arg, LongPath.data(), LongPath.capacity());
   if (Length == 0)
     return mapWindowsError(GetLastError());
   if (Length > LongPath.capacity()) {
     // We're not going to try to deal with paths longer than MAX_PATH, so we'll
     // treat this as an error.  GetLastError() returns ERROR_SUCCESS, which
     // isn't useful, so we'll hardcode an appropriate error value.
     return mapWindowsError(ERROR_INSUFFICIENT_BUFFER);
   }
   LongPath.set_size(Length);
-  return ConvertAndPushArg(LongPath.data(), Args, Allocator);
+  return ConvertAndPushArg(LongPath.data(), Args, Alloc);
 }
 
 std::error_code
-Process::GetArgumentVector(SmallVectorImpl<const char *> &Args,
-                           ArrayRef<const char *>,
-                           SpecificBumpPtrAllocator<char> &ArgAllocator) {
+windows::GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
+                                 BumpPtrAllocator &Alloc) {
   int ArgCount;
   wchar_t **UnicodeCommandLine =
       CommandLineToArgvW(GetCommandLineW(), &ArgCount);
   if (!UnicodeCommandLine)
     return mapWindowsError(::GetLastError());
 
   Args.reserve(ArgCount);
   std::error_code ec;
 
   // 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];
   int Length = ::GetModuleFileNameW(NULL, ModuleName, MAX_PATH);
   if (0 < Length && Length < MAX_PATH)
     UnicodeCommandLine[0] = ModuleName;
 
   // If the first argument is a shortened (8.3) name (which is possible even
   // if we got the module name), the driver will have trouble distinguishing it
   // (e.g., clang.exe v. clang++.exe), so expand it now.
-  ec = ExpandShortFileName(UnicodeCommandLine[0], Args, ArgAllocator);
+  ec = ExpandShortFileName(UnicodeCommandLine[0], Args, Alloc);
 
   for (int i = 1; i < ArgCount && !ec; ++i) {
-    ec = WildcardExpand(UnicodeCommandLine[i], Args, ArgAllocator);
+    ec = WildcardExpand(UnicodeCommandLine[i], Args, Alloc);
     if (ec)
       break;
   }
 
   LocalFree(UnicodeCommandLine);
   return ec;
 }
 

llvm/trunk/lib/Support/Windows/WindowsSupport.h

 std::error_code UTF8ToUTF16(StringRef utf8, SmallVectorImpl<wchar_t> &utf16);
 /// Convert to UTF16 from the current code page used in the system
 std::error_code CurCPToUTF16(StringRef utf8, SmallVectorImpl<wchar_t> &utf16);
 std::error_code UTF16ToUTF8(const wchar_t *utf16, size_t utf16_len,
                             SmallVectorImpl<char> &utf8);
 /// Convert from UTF16 to the current code page used in the system
 std::error_code UTF16ToCurCP(const wchar_t *utf16, size_t utf16_len,
                              SmallVectorImpl<char> &utf8);
+
+// Returns command line arguments. Unlike arguments given to main(),
+// this function guarantees that the returned arguments are encoded in
+// UTF-8 regardless of the current code page setting.
+std::error_code GetCommandLineArguments(SmallVectorImpl<const char *> &Args,
+                                        BumpPtrAllocator &Alloc);
 } // end namespace windows
 } // end namespace sys
 } // end namespace llvm.
 
 #endif