[patch] Split openFileForWrite into windows and unix versions

[Aaron Ballman]

The patch mostly looks good to me, just had a question (inline).
However, I do think we are missing tests -- text mode on Windows
translates CRLFs in and out of the files, whereas binary mode does
not.  So this could be a problem for writing files which expect a
particular line ending.

> diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp
> index 171ccc3..88137ea 100644
> --- a/lib/Support/Path.cpp
> +++ b/lib/Support/Path.cpp
> @@ -692,36 +692,6 @@ error_code createUniqueDirectory(const Twine &Prefix,
>                              true, 0, FS_Dir);
>  }
>
> -error_code openFileForWrite(const Twine &Name, int &ResultFD,
> -                            sys::fs::OpenFlags Flags, unsigned Mode) {
> -  // Verify that we don't have both "append" and "excl".
> -  assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) &&
> -         "Cannot specify both 'excl' and 'append' file creation flags!");
> -
> -  int OpenFlags = O_WRONLY | O_CREAT;
> -
> -#ifdef O_BINARY
> -  if (Flags & F_Binary)
> -    OpenFlags |= O_BINARY;
> -#endif
> -
> -  if (Flags & F_Append)
> -    OpenFlags |= O_APPEND;
> -  else
> -    OpenFlags |= O_TRUNC;
> -
> -  if (Flags & F_Excl)
> -    OpenFlags |= O_EXCL;
> -
> -  SmallString<128> Storage;
> -  StringRef P = Name.toNullTerminatedStringRef(Storage);
> -  while ((ResultFD = open(P.begin(), OpenFlags, Mode)) < 0) {
> -    if (errno != EINTR)
> -      return error_code(errno, system_category());
> -  }
> -  return error_code::success();
> -}
> -
>  error_code make_absolute(SmallVectorImpl<char> &path) {
>    StringRef p(path.data(), path.size());
>
> diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc
> index 8944008..ccd60e5 100644
> --- a/lib/Support/Unix/Path.inc
> +++ b/lib/Support/Unix/Path.inc
> @@ -766,6 +766,31 @@ error_code openFileForRead(const Twine &Name, int &ResultFD) {
>    return error_code::success();
>  }
>
> +error_code openFileForWrite(const Twine &Name, int &ResultFD,
> +                            sys::fs::OpenFlags Flags, unsigned Mode) {
> +  // Verify that we don't have both "append" and "excl".
> +  assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) &&
> +         "Cannot specify both 'excl' and 'append' file creation flags!");
> +
> +  int OpenFlags = O_WRONLY | O_CREAT;
> +
> +  if (Flags & F_Append)
> +    OpenFlags |= O_APPEND;
> +  else
> +    OpenFlags |= O_TRUNC;
> +
> +  if (Flags & F_Excl)
> +    OpenFlags |= O_EXCL;
> +
> +  SmallString<128> Storage;
> +  StringRef P = Name.toNullTerminatedStringRef(Storage);
> +  while ((ResultFD = open(P.begin(), OpenFlags, Mode)) < 0) {
> +    if (errno != EINTR)
> +      return error_code(errno, system_category());
> +  }
> +  return error_code::success();
> +}
> +
>  } // end namespace fs
>  } // end namespace sys
>  } // end namespace llvm
> diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc
> index dff89c7..90018ea 100644
> --- a/lib/Support/Windows/Path.inc
> +++ b/lib/Support/Windows/Path.inc
> @@ -1071,6 +1071,53 @@ error_code openFileForRead(const Twine &Name, int &ResultFD) {
>    return error_code::success();
>  }
>
> +error_code openFileForWrite(const Twine &Name, int &ResultFD,
> +                            sys::fs::OpenFlags Flags, unsigned Mode) {
> +  // Verify that we don't have both "append" and "excl".
> +  assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) &&
> +         "Cannot specify both 'excl' and 'append' file creation flags!");
> +
> +  SmallString<128> PathStorage;
> +  SmallVector<wchar_t, 128> PathUTF16;
> +
> +  if (error_code EC = UTF8ToUTF16(Name.toStringRef(PathStorage),
> +                                  PathUTF16))
> +    return EC;
> +
> +  DWORD CreationDisposition;
> +  if (Flags & F_Excl)
> +    CreationDisposition = CREATE_NEW;
> +  else if (Flags & F_Append)
> +    CreationDisposition = OPEN_ALWAYS;
> +  else
> +    CreationDisposition = CREATE_ALWAYS;
> +
> +  HANDLE H = ::CreateFileW(PathUTF16.begin(), GENERIC_WRITE,
> +                           FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
> +                           CreationDisposition, FILE_ATTRIBUTE_NORMAL, NULL);

Are we sure that we want to share the ability to write to the file?
What happens if the file is updated by another process while we are
writing to it?

> +
> +  if (H == INVALID_HANDLE_VALUE) {
> +    error_code EC = windows_error(::GetLastError());
> +    // Provide a better error message when trying to open directories.
> +    // This only runs if we failed to open the file, so there is probably
> +    // no performances issues.
> +    if (EC != windows_error::access_denied)
> +      return EC;
> +    if (is_directory(Name))
> +      return error_code(errc::is_a_directory, posix_category());
> +    return EC;
> +  }
> +
> +  int FD = ::_open_osfhandle(intptr_t(H), 0);
> +  if (FD == -1) {
> +    ::CloseHandle(H);
> +    return windows_error::invalid_handle;
> +  }
> +
> +  ResultFD = FD;
> +  return error_code::success();
> +}
> +
>  } // end namespace fs
>  } // end namespace sys
>  } // end namespace llvm
> diff --git a/test/Object/archive-extract-dir.test b/test/Object/archive-extract-dir.test
> new file mode 100644
> index 0000000..c718f90
> --- /dev/null
> +++ b/test/Object/archive-extract-dir.test
> @@ -0,0 +1,13 @@
> +REQUIRES: shell
> +
> +RUN: mkdir -p %t
> +RUN: cd %t
> +RUN: rm -rf foo
> +RUN: echo foo > foo
> +RUN: rm -f test.a
> +RUN: llvm-ar rc          test.a foo
> +RUN: rm foo
> +RUN: mkdir foo
> +RUN: not llvm-ar x test.a foo 2>&1 | FileCheck %s
> +
> +CHECK: foo: Is a directory
>

~Aaron

On Thu, Jul 18, 2013 at 3:45 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> This is hopefully the last file handling patch for now. It is similar
> to 186511, but for creating files for
> writing.
>
> It is interesting that all tests pass even when I don't add _O_TEXT to
> _open_osfhandle. Are we missing tests or we never depend on windows
> handling a file as text?
>
> Cheers,
> Rafael