[Support] Improve readNativeFile(Slice) interface

[labath]

Summary

There was a subtle, but pretty important difference between the Slice
and regular versions of this function. The Slice function was
zero-initializing the rest of the buffer when the read syscall returned
less bytes than expected, while the regular function did not.

This patch removes the inconsistency by making both functions *not*
zero-initialize the buffer. The zeroing code is moved to the
MemoryBuffer class, which is currently the only user of this code. This
makes the API more consistent, and the code shorter.

While in there, I also refactor the functions to return the number of
bytes through the regular return value (via Expected<size_t>) instead of
a separate by-ref argument.

[aganea]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

Shouldn't we use ToRead here instead of Buf->getBuffer()? Could you please test two sequential reads in the unittests?

lib/Support/Unix/Path.inc

1005 ↗

(On Diff #216104)

The cast is not needed here.

[labath]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

Woops, indeed it should. Thanks for catching that!

As for the test, I am not sure how I could force the read syscall to return partial data here. For the non-slice version, I suppose I could make this read from a pipe, and then make sure I write to the pipe very slowly. However, the slice version requires a seekable fd, and I don't know what to do there... Do you have any suggestion?

[aganea]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

You can probably call MemoryBuffer::getOpenFile with a smaller FileSize & MapSize and IsVolatile = true? And then a subsequent call by setting Offset > 0? Would that work?

[labath]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

I don't fully understand what you mean, but I don't see how could that work. Are you expecting that the second time around the OS will return the partial read first, because that data will already be cached and ready? If so, then I would be very surprised if that works, particularly for a file that has just been created because it will likely be present in the disk cache in its entirety.

[aganea]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

You're right. You would need to test it from the shell with something like: cat foo | pv --quiet --line-mode --rate-limit 1 | clang -emit-llvm -o -. I don't know if pv is available everywhere, or if there's a better solution to slow down cat? What do you think?

[labath]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

Ok, I think I am starting to understand what you mean. There's no problem with slowing down the input for a pipe. I can do that from a unit test relatively easily. The problem is that that the the pipe is not seekable (at least on unix OSs, I don't know what would windows do here). The reason that clang - works is because it invokes a different MemoryBuffer function, which is implemented in terms of readNativeFile and so it works fine with a non-seekable fd. However, calling readNativeFileSlice with a non-seekable fd would result in an error. And unfortunately, I don't know if there's a way to create a seekable fd, which will not satisfy all read requests immediately (short of some FUSE mounts or similarly heroic efforts).

[rnk]

Seems reasonable aside from the bug. :)

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

Right, testing a short read from a seekable file is... very hard. IIRC Linux goes out of its way to avoid surfacing short reads if it can. One way to do it would be to make a 6GB file on Windows, since it has the 2GB ReadFile limitation. Of course, I was happy to commit my code without such a test, so I can only apply the same standard to you. :)

[aganea]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

What about this? This does exercise this loop, including the memset.
And it looks like the 2GB is no more (at least on my Windows 10 1803), calling ::ReadFile with (2^32)-1 works as expected. We should probably update the comment in lib/Support/Windows/Path.inc, L1223 :)

TEST_F(FileSystemTest, bigFile) {
  int FD;
  SmallString<64> TempPath;
  ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD, TempPath));

  // Would cause two file reads on Windows, the limit for each read being
  // (2^32)-1
  size_t Size = (1ULL << 32);
  auto E = fs::resize_file(FD, Size);
  if (E) {
    ASSERT_NO_ERROR(fs::remove(Twine(TempPath)));
    return; // skip test is there's enough temp/ space
  }

  auto Buf = MemoryBuffer::getFile(TempPath, Size+1, true, true);
  EXPECT_TRUE((bool)Buf);
  EXPECT_EQ(Buf.get()->getBufferSize(), Size+1);

  ASSERT_NO_ERROR(fs::remove(Twine(TempPath)));
}

[labath]

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

That would work, if we are OK with creating a 4GB file for the purpose of testing this *and* requiring 4GB of ram to be able to read it.

In fact, linux seems to have the 2GB limitation too (2GB-4KB, to be precise), so this would work there too. Also, most filesystems on linux support sparse files, so we would probably be able to avoid actually wasting that much disk space, if we're really careful. However, the need for a 4GB ram buffer worries me, particularly as detecting OOM conditions reliably is quite hard.

The standard approach to testing code which relies on external stuff which cannot be (easily) controlled in a test is to mock the external component. However, this requires changes to code being tested, such as parameterizing the MemoryBuffer class with a filesystem implementation, and I am not aware of this approach being used anywhere in llvm...

[labath]

• fix the partial read bug
• add a partial read test for the streaming case (not tested on windows yet)

[aganea]

LGTM, just a missing include (see below).

In D66471#1638966, @labath wrote:

• add a partial read test for the streaming case (not tested on windows yet)

Works on Windows.

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

I am equally happy to commit that above test later on. I could possibly scope it to Windows and ensure, for example, available memory > 16 GB and available free heap > 4 GB.

unittests/Support/MemoryBufferTest.cpp

23 ↗

(On Diff #216359)

Missing:

#ifdef _WIN32
#include <windows.h> 
#endif

[labath]

In D66471#1639774, @aganea wrote:

In D66471#1638966, @labath wrote:

• add a partial read test for the streaming case (not tested on windows yet)

Works on Windows.

Thanks for checking it out.

lib/Support/MemoryBuffer.cpp

467 ↗

(On Diff #216104)

Sounds good. I think doing that separately is a good idea. I might be overcautious, but I am expecting it will take several tries before all the bots are happy with a test like that.

llvm/trunk/include/llvm/Support/FileSystem.h

 /// Return an open handle to standard out. On Unix, this is typically FD 1.
 /// Returns kInvalidFile when the stream is closed.
 file_t getStdoutHandle();
 
 /// Return an open handle to standard error. On Unix, this is typically FD 2.
 /// Returns kInvalidFile when the stream is closed.
 file_t getStderrHandle();
 
-/// Reads \p Buf.size() bytes from \p FileHandle into \p Buf. The number of
-/// bytes actually read is returned in \p BytesRead. On Unix, this is equivalent
-/// to `*BytesRead = ::read(FD, Buf.data(), Buf.size())`, with error reporting.
-/// BytesRead will contain zero when reaching EOF.
+/// Reads \p Buf.size() bytes from \p FileHandle into \p Buf. Returns the number
+/// of bytes actually read. On Unix, this is equivalent to `return ::read(FD,
+/// Buf.data(), Buf.size())`, with error reporting. Returns 0 when reaching EOF.
 ///
 /// @param FileHandle File to read from.
 /// @param Buf Buffer to read into.
-/// @param BytesRead Output parameter of the number of bytes read.
-/// @returns The error, if any, or errc::success.
-std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
-                               size_t *BytesRead);
+/// @returns The number of bytes read, or error.
+Expected<size_t> readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf);
 
 /// Reads \p Buf.size() bytes from \p FileHandle at offset \p Offset into \p
 /// Buf. If 'pread' is available, this will use that, otherwise it will use
-/// 'lseek'. Bytes requested beyond the end of the file will be zero
-/// initialized.
+/// 'lseek'. Returns the number of bytes actually read. Returns 0 when reaching
+/// EOF.
 ///
 /// @param FileHandle File to read from.
 /// @param Buf Buffer to read into.
 /// @param Offset Offset into the file at which the read should occur.
-/// @returns The error, if any, or errc::success.
-std::error_code readNativeFileSlice(file_t FileHandle,
-                                    MutableArrayRef<char> Buf, size_t Offset);
+/// @returns The number of bytes read, or error.
+Expected<size_t> readNativeFileSlice(file_t FileHandle,
+                                     MutableArrayRef<char> Buf,
+                                     uint64_t Offset);
 
 /// @brief Opens the file with the given name in a write-only or read-write
 /// mode, returning its open file descriptor. If the file does not exist, it
 /// is created.
 ///
 /// The caller is responsible for closing the file descriptor once they are
 /// finished with it.
 ///

llvm/trunk/lib/Support/MemoryBuffer.cpp

public:
   }
 };
 }
 
 static ErrorOr<std::unique_ptr<WritableMemoryBuffer>>
 getMemoryBufferForStream(sys::fs::file_t FD, const Twine &BufferName) {
   const ssize_t ChunkSize = 4096*4;
   SmallString<ChunkSize> Buffer;
-  size_t ReadBytes;
   // Read into Buffer until we hit EOF.
-  do {
+  for (;;) {
     Buffer.reserve(Buffer.size() + ChunkSize);
-    if (auto EC = sys::fs::readNativeFile(
-            FD, makeMutableArrayRef(Buffer.end(), ChunkSize), &ReadBytes))
-      return EC;
-    Buffer.set_size(Buffer.size() + ReadBytes);
-  } while (ReadBytes != 0);
+    Expected<size_t> ReadBytes = sys::fs::readNativeFile(
+        FD, makeMutableArrayRef(Buffer.end(), ChunkSize));
+    if (!ReadBytes)
+      return errorToErrorCode(ReadBytes.takeError());
+    if (*ReadBytes == 0)
+      break;
+    Buffer.set_size(Buffer.size() + *ReadBytes);
+  }
 
   return getMemBufferCopyImpl(Buffer, BufferName);
 }
 
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFile(const Twine &Filename, int64_t FileSize,
                       bool RequiresNullTerminator, bool IsVolatile) {
getOpenFileImpl(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
 
   auto Buf = WritableMemoryBuffer::getNewUninitMemBuffer(MapSize, Filename);
   if (!Buf) {
     // Failed to create a buffer. The only way it can fail is if
     // new(std::nothrow) returns 0.
     return make_error_code(errc::not_enough_memory);
   }
 
-  if (std::error_code EC =
-          sys::fs::readNativeFileSlice(FD, Buf->getBuffer(), Offset))
-    return EC;
+  // Read until EOF, zero-initialize the rest.
+  MutableArrayRef<char> ToRead = Buf->getBuffer();
+  while (!ToRead.empty()) {
+    Expected<size_t> ReadBytes =
+        sys::fs::readNativeFileSlice(FD, ToRead, Offset);
+    if (!ReadBytes)
+      return errorToErrorCode(ReadBytes.takeError());
+    if (*ReadBytes == 0) {
+      std::memset(ToRead.data(), 0, ToRead.size());
+      break;
+    }
+    ToRead = ToRead.drop_front(*ReadBytes);
+    Offset += *ReadBytes;
+  }
 
   return std::move(Buf);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getOpenFile(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
                           bool RequiresNullTerminator, bool IsVolatile) {
   return getOpenFileImpl<MemoryBuffer>(FD, Filename, FileSize, FileSize, 0,

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

if (EC)
     return errorCodeToError(EC);
   return ResultFD;
 }
 
 file_t getStdinHandle() { return 0; }
 file_t getStdoutHandle() { return 1; }
 file_t getStderrHandle() { return 2; }
 
-std::error_code readNativeFile(file_t FD, MutableArrayRef<char> Buf,
-                               size_t *BytesRead) {
-  *BytesRead = sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Buf.size());
-  if (ssize_t(*BytesRead) == -1)
-    return std::error_code(errno, std::generic_category());
-  return std::error_code();
+Expected<size_t> readNativeFile(file_t FD, MutableArrayRef<char> Buf) {
+  ssize_t NumRead =
+      sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Buf.size());
+  if (ssize_t(NumRead) == -1)
+    return errorCodeToError(std::error_code(errno, std::generic_category()));
+  return NumRead;
 }
 
-std::error_code readNativeFileSlice(file_t FD, MutableArrayRef<char> Buf,
-                                    size_t Offset) {
-  char *BufPtr = Buf.data();
-  size_t BytesLeft = Buf.size();
-
-#ifndef HAVE_PREAD
-  // If we don't have pread, seek to Offset.
-  if (lseek(FD, Offset, SEEK_SET) == -1)
-    return std::error_code(errno, std::generic_category());
-#endif
-
-  while (BytesLeft) {
+Expected<size_t> readNativeFileSlice(file_t FD, MutableArrayRef<char> Buf,
+                                     uint64_t Offset) {
 #ifdef HAVE_PREAD
-    ssize_t NumRead = sys::RetryAfterSignal(-1, ::pread, FD, BufPtr, BytesLeft,
-                                            Buf.size() - BytesLeft + Offset);
+  ssize_t NumRead =
+      sys::RetryAfterSignal(-1, ::pread, FD, Buf.data(), Buf.size(), Offset);
 #else
-    ssize_t NumRead = sys::RetryAfterSignal(-1, ::read, FD, BufPtr, BytesLeft);
-#endif
-    if (NumRead == -1) {
-      // Error while reading.
-      return std::error_code(errno, std::generic_category());
-    }
-    if (NumRead == 0) {
-      memset(BufPtr, 0, BytesLeft); // zero-initialize rest of the buffer.
-      break;
-    }
-    BytesLeft -= NumRead;
-    BufPtr += NumRead;
-  }
-  return std::error_code();
+  if (lseek(FD, Offset, SEEK_SET) == -1)
+    return errorCodeToError(std::error_code(errno, std::generic_category()));
+  ssize_t NumRead =
+      sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Buf.size());
+#endif
+  if (NumRead == -1)
+    return errorCodeToError(std::error_code(errno, std::generic_category()));
+  return NumRead;
 }
 
 std::error_code closeFile(file_t &F) {
   file_t TmpF = F;
   F = kInvalidFile;
   return Process::SafelyCloseFileDescriptor(TmpF);
 }
 

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

 file_t convertFDToNativeFile(int FD) {
   return reinterpret_cast<HANDLE>(::_get_osfhandle(FD));
 }
 
 file_t getStdinHandle() { return ::GetStdHandle(STD_INPUT_HANDLE); }
 file_t getStdoutHandle() { return ::GetStdHandle(STD_OUTPUT_HANDLE); }
 file_t getStderrHandle() { return ::GetStdHandle(STD_ERROR_HANDLE); }
 
-std::error_code readNativeFileImpl(file_t FileHandle, char *BufPtr, size_t BytesToRead,
-                                   size_t *BytesRead, OVERLAPPED *Overlap) {
+Expected<size_t> readNativeFileImpl(file_t FileHandle,
+                                    MutableArrayRef<char> Buf,
+                                    OVERLAPPED *Overlap) {
   // ReadFile can only read 2GB at a time. The caller should check the number of
   // bytes and read in a loop until termination.
-  DWORD BytesToRead32 =
-      std::min(size_t(std::numeric_limits<DWORD>::max()), BytesToRead);
-  DWORD BytesRead32 = 0;
-  bool Success =
-      ::ReadFile(FileHandle, BufPtr, BytesToRead32, &BytesRead32, Overlap);
-  *BytesRead = BytesRead32;
-  if (!Success) {
-    DWORD Err = ::GetLastError();
-    // EOF is not an error.
-    if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF)
-      return std::error_code();
-    return mapWindowsError(Err);
-  }
-  return std::error_code();
+  DWORD BytesToRead =
+      std::min(size_t(std::numeric_limits<DWORD>::max()), Buf.size());
+  DWORD BytesRead = 0;
+  if (::ReadFile(FileHandle, Buf.data(), BytesToRead, &BytesRead, Overlap))
+    return BytesRead;
+    return BytesRead;
+  return errorCodeToError(mapWindowsError(Err));
 }
 
-std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
-                               size_t *BytesRead) {
-  return readNativeFileImpl(FileHandle, Buf.data(), Buf.size(), BytesRead,
-                            /*Overlap=*/nullptr);
+Expected<size_t> readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf) {
+  return readNativeFileImpl(FileHandle, Buf, /*Overlap=*/nullptr);
 }
 
-std::error_code readNativeFileSlice(file_t FileHandle,
-                                    MutableArrayRef<char> Buf, size_t Offset) {
-  char *BufPtr = Buf.data();
-  size_t BytesLeft = Buf.size();
-
-  while (BytesLeft) {
-    uint64_t CurOff = Buf.size() - BytesLeft + Offset;
-    OVERLAPPED Overlapped = {};
-    Overlapped.Offset = uint32_t(CurOff);
-    Overlapped.OffsetHigh = uint32_t(uint64_t(CurOff) >> 32);
-
-    size_t BytesRead = 0;
-    if (auto EC = readNativeFileImpl(FileHandle, BufPtr, BytesLeft, &BytesRead,
-                                     &Overlapped))
-      return EC;
-
-    // Once we reach EOF, zero the remaining bytes in the buffer.
-    if (BytesRead == 0) {
-      memset(BufPtr, 0, BytesLeft);
-      break;
-    }
-    BytesLeft -= BytesRead;
-    BufPtr += BytesRead;
-  }
-  return std::error_code();
+Expected<size_t> readNativeFileSlice(file_t FileHandle,
+                                     MutableArrayRef<char> Buf,
+                                     uint64_t Offset) {
+  Overlapped.Offset = uint32_t(Offset);
+  Overlapped.OffsetHigh = uint32_t(Offset >> 32);
+  return readNativeFileImpl(FileHandle, Buf, &Overlapped);
 }
 
 std::error_code closeFile(file_t &F) {
   file_t TmpF = F;
   F = kInvalidFile;
   if (!::CloseHandle(TmpF))
     return mapWindowsError(::GetLastError());
   return std::error_code();

llvm/trunk/unittests/Support/MemoryBufferTest.cpp

 
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
+#if LLVM_ENABLE_THREADS
+#include <thread>
+#endif
+#if LLVM_ON_UNIX
+#include <unistd.h>
+#endif
+#if _WIN32
+#include <windows.h>
+#endif
 
 using namespace llvm;
 
 #define ASSERT_NO_ERROR(x)                                                     \
   if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
     SmallString<128> MessageStorage;                                           \
     raw_svector_ostream Message(MessageStorage);                               \
     Message << #x ": did not return errc::success.\n"                          \
TEST_F(MemoryBufferTest, copy) {
   // copy with a name
   OwningBuffer MBC2(MemoryBuffer::getMemBufferCopy(data, "copy"));
   EXPECT_TRUE(nullptr != MBC2.get());
 
   // verify the two copies do not point to the same place
   EXPECT_NE(MBC1->getBufferStart(), MBC2->getBufferStart());
 }
 
+#if LLVM_ENABLE_THREADS
+TEST_F(MemoryBufferTest, createFromPipe) {
+  sys::fs::file_t pipes[2];
+#if LLVM_ON_UNIX
+  ASSERT_EQ(::pipe(pipes), 0) << strerror(errno);
+#else
+  ASSERT_TRUE(::CreatePipe(&pipes[0], &pipes[1], nullptr, 0))
+      << ::GetLastError();
+#endif
+  auto ReadCloser = make_scope_exit([&] { sys::fs::closeFile(pipes[0]); });
+  std::thread Writer([&] {
+    auto WriteCloser = make_scope_exit([&] { sys::fs::closeFile(pipes[1]); });
+    for (unsigned i = 0; i < 5; ++i) {
+      std::this_thread::sleep_for(std::chrono::milliseconds(10));
+#if LLVM_ON_UNIX
+      ASSERT_EQ(::write(pipes[1], "foo", 3), 3) << strerror(errno);
+#else
+      DWORD Written;
+      ASSERT_TRUE(::WriteFile(pipes[1], "foo", 3, &Written, nullptr))
+          << ::GetLastError();
+      ASSERT_EQ(Written, 3u);
+#endif
+    }
+  });
+  ErrorOr<OwningBuffer> MB =
+      MemoryBuffer::getOpenFile(pipes[0], "pipe", /*FileSize*/ -1);
+  Writer.join();
+  ASSERT_NO_ERROR(MB.getError());
+  EXPECT_EQ(MB.get()->getBuffer(), "foofoofoofoofoo");
+}
+#endif
+
 TEST_F(MemoryBufferTest, make_new) {
   // 0-sized buffer
   OwningBuffer Zero(WritableMemoryBuffer::getNewUninitMemBuffer(0));
   EXPECT_TRUE(nullptr != Zero.get());
 
   // uninitialized buffer with no name
   OwningBuffer One(WritableMemoryBuffer::getNewUninitMemBuffer(321));
   EXPECT_TRUE(nullptr != One.get());

llvm/trunk/unittests/Support/Path.cpp

 //===- llvm/unittest/Support/Path.cpp - Path tests ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Path.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
TEST_F(FileSystemTest, ReadWriteFileCanReadOrWrite) {
   int FD;
   ASSERT_NO_ERROR(fs::openFileForReadWrite(NonExistantFile, FD,
                                            fs::CD_OpenExisting, fs::OF_None));
   FileDescriptorCloser Closer(FD);
   verifyRead(FD, "Fizz", true);
   verifyWrite(FD, "Buzz", true);
 }
 
+TEST_F(FileSystemTest, readNativeFile) {
+  createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "01234");
+  FileRemover Cleanup(NonExistantFile);
+  const auto &Read = [&](size_t ToRead) -> Expected<std::string> {
+    std::string Buf(ToRead, '?');
+    Expected<fs::file_t> FD = fs::openNativeFileForRead(NonExistantFile);
+    if (!FD)
+      return FD.takeError();
+    auto Close = make_scope_exit([&] { fs::closeFile(*FD); });
+    if (Expected<size_t> BytesRead = fs::readNativeFile(
+            *FD, makeMutableArrayRef(&*Buf.begin(), Buf.size())))
+      return Buf.substr(0, *BytesRead);
+    else
+      return BytesRead.takeError();
+  };
+  EXPECT_THAT_EXPECTED(Read(5), HasValue("01234"));
+  EXPECT_THAT_EXPECTED(Read(3), HasValue("012"));
+  EXPECT_THAT_EXPECTED(Read(6), HasValue("01234"));
+}
+
 TEST_F(FileSystemTest, readNativeFileSlice) {
-  char Data[10] = {'0', '1', '2', '3', '4', 0, 0, 0, 0, 0};
-  createFileWithData(NonExistantFile, false, fs::CD_CreateNew,
-                     StringRef(Data, 5));
+  createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "01234");
   FileRemover Cleanup(NonExistantFile);
   Expected<fs::file_t> FD = fs::openNativeFileForRead(NonExistantFile);
   ASSERT_THAT_EXPECTED(FD, Succeeded());
-  char Buf[10];
+  auto Close = make_scope_exit([&] { fs::closeFile(*FD); });
   const auto &Read = [&](size_t Offset,
-                         size_t ToRead) -> Expected<ArrayRef<char>> {
-    std::memset(Buf, 0x47, sizeof(Buf));
-    if (std::error_code EC = fs::readNativeFileSlice(
-            *FD, makeMutableArrayRef(Buf, ToRead), Offset))
-      return errorCodeToError(EC);
-    return makeArrayRef(Buf, ToRead);
+                         size_t ToRead) -> Expected<std::string> {
+    std::string Buf(ToRead, '?');
+    if (Expected<size_t> BytesRead = fs::readNativeFileSlice(
+            *FD, makeMutableArrayRef(&*Buf.begin(), Buf.size()), Offset))
+      return Buf.substr(0, *BytesRead);
+    else
+      return BytesRead.takeError();
   };
-  EXPECT_THAT_EXPECTED(Read(0, 5), HasValue(makeArrayRef(Data + 0, 5)));
-  EXPECT_THAT_EXPECTED(Read(0, 3), HasValue(makeArrayRef(Data + 0, 3)));
-  EXPECT_THAT_EXPECTED(Read(2, 3), HasValue(makeArrayRef(Data + 2, 3)));
-  EXPECT_THAT_EXPECTED(Read(0, 6), HasValue(makeArrayRef(Data + 0, 6)));
-  EXPECT_THAT_EXPECTED(Read(2, 6), HasValue(makeArrayRef(Data + 2, 6)));
-  EXPECT_THAT_EXPECTED(Read(5, 5), HasValue(makeArrayRef(Data + 5, 5)));
+  EXPECT_THAT_EXPECTED(Read(0, 5), HasValue("01234"));
+  EXPECT_THAT_EXPECTED(Read(0, 3), HasValue("012"));
+  EXPECT_THAT_EXPECTED(Read(2, 3), HasValue("234"));
+  EXPECT_THAT_EXPECTED(Read(0, 6), HasValue("01234"));
+  EXPECT_THAT_EXPECTED(Read(2, 6), HasValue("234"));
+  EXPECT_THAT_EXPECTED(Read(5, 5), HasValue(""));
 }
 
 TEST_F(FileSystemTest, is_local) {
   bool TestDirectoryIsLocal;
   ASSERT_NO_ERROR(fs::is_local(TestDirectory, TestDirectoryIsLocal));
   EXPECT_EQ(TestDirectoryIsLocal, fs::is_local(TestDirectory));
 
   int FD;