[Support] Improve readNativeFile(Slice) interface

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.

Reviewers: aganea, rnk

Subscribers: kristina, Bigcheese, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66471

llvm-svn: 369627

2 files changed, 77 insertions, 16 deletions

diff --git a/llvm/unittests/Support/MemoryBufferTest.cpp b/llvm/unittests/Support/MemoryBufferTest.cpp
index 629b94d7843..1b72aadebf4 100644
--- a/llvm/unittests/Support/MemoryBufferTest.cpp
+++ b/llvm/unittests/Support/MemoryBufferTest.cpp
@@ -17,6 +17,15 @@
 #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;
 
@@ -152,6 +161,38 @@ TEST_F(MemoryBufferTest, copy) {
   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));
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index 2aadf89161c..9de46a689cd 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -8,6 +8,7 @@
 
 #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"
@@ -1514,28 +1515,47 @@ TEST_F(FileSystemTest, ReadWriteFileCanReadOrWrite) {
   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) {