From 3ffae1923a53a2532f7564a0be44e41193e7bed2 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Thu, 22 Aug 2019 08:13:30 +0000 Subject: [PATCH] [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) instead of a separate by-ref argument. Reviewers: aganea, rnk Subscribers: kristina, Bigcheese, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66471 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@369627 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/FileSystem.h | 24 ++++----- lib/Support/MemoryBuffer.cpp | 33 ++++++++---- lib/Support/Unix/Path.inc | 50 ++++++----------- lib/Support/Windows/Path.inc | 75 +++++++++----------------- unittests/Support/MemoryBufferTest.cpp | 41 ++++++++++++++ unittests/Support/Path.cpp | 52 ++++++++++++------ 6 files changed, 154 insertions(+), 121 deletions(-) diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h index 1bec27bddad..7707306a9be 100644 --- a/include/llvm/Support/FileSystem.h +++ b/include/llvm/Support/FileSystem.h @@ -991,29 +991,27 @@ file_t getStdoutHandle(); /// 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 Buf, - size_t *BytesRead); +/// @returns The number of bytes read, or error. +Expected readNativeFile(file_t FileHandle, MutableArrayRef 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 Buf, size_t Offset); +/// @returns The number of bytes read, or error. +Expected readNativeFileSlice(file_t FileHandle, + MutableArrayRef 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 diff --git a/lib/Support/MemoryBuffer.cpp b/lib/Support/MemoryBuffer.cpp index 2f31f059ddb..e4027ca7bbf 100644 --- a/lib/Support/MemoryBuffer.cpp +++ b/lib/Support/MemoryBuffer.cpp @@ -211,15 +211,17 @@ static ErrorOr> getMemoryBufferForStream(sys::fs::file_t FD, const Twine &BufferName) { const ssize_t ChunkSize = 4096*4; SmallString 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 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); } @@ -458,9 +460,20 @@ getOpenFileImpl(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize, 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 ToRead = Buf->getBuffer(); + while (!ToRead.empty()) { + Expected 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); } diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc index cbc03821940..a617eca3566 100644 --- a/lib/Support/Unix/Path.inc +++ b/lib/Support/Unix/Path.inc @@ -999,44 +999,28 @@ file_t getStdinHandle() { return 0; } file_t getStdoutHandle() { return 1; } file_t getStderrHandle() { return 2; } -std::error_code readNativeFile(file_t FD, MutableArrayRef 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 readNativeFile(file_t FD, MutableArrayRef 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 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 readNativeFileSlice(file_t FD, MutableArrayRef 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); + 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) { - // 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 (NumRead == -1) + return errorCodeToError(std::error_code(errno, std::generic_category())); + return NumRead; } std::error_code closeFile(file_t &F) { diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc index 00a4e669abb..dd6ea995d23 100644 --- a/lib/Support/Windows/Path.inc +++ b/lib/Support/Windows/Path.inc @@ -1217,57 +1217,34 @@ 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 readNativeFileImpl(file_t FileHandle, + MutableArrayRef 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::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(); -} - -std::error_code readNativeFile(file_t FileHandle, MutableArrayRef Buf, - size_t *BytesRead) { - return readNativeFileImpl(FileHandle, Buf.data(), Buf.size(), BytesRead, - /*Overlap=*/nullptr); -} - -std::error_code readNativeFileSlice(file_t FileHandle, - MutableArrayRef 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(); + DWORD BytesToRead = + std::min(size_t(std::numeric_limits::max()), Buf.size()); + DWORD BytesRead = 0; + if (::ReadFile(FileHandle, Buf.data(), BytesToRead, &BytesRead, Overlap)) + return BytesRead; + DWORD Err = ::GetLastError(); + // EOF is not an error. + if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF) + return BytesRead; + return errorCodeToError(mapWindowsError(Err)); +} + +Expected readNativeFile(file_t FileHandle, MutableArrayRef Buf) { + return readNativeFileImpl(FileHandle, Buf, /*Overlap=*/nullptr); +} + +Expected readNativeFileSlice(file_t FileHandle, + MutableArrayRef Buf, + uint64_t Offset) { + OVERLAPPED Overlapped = {}; + Overlapped.Offset = uint32_t(Offset); + Overlapped.OffsetHigh = uint32_t(Offset >> 32); + return readNativeFileImpl(FileHandle, Buf, &Overlapped); } std::error_code closeFile(file_t &F) { diff --git a/unittests/Support/MemoryBufferTest.cpp b/unittests/Support/MemoryBufferTest.cpp index 629b94d7843..1b72aadebf4 100644 --- a/unittests/Support/MemoryBufferTest.cpp +++ b/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 +#endif +#if LLVM_ON_UNIX +#include +#endif +#if _WIN32 +#include +#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 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/unittests/Support/Path.cpp b/unittests/Support/Path.cpp index 2aadf89161c..9de46a689cd 100644 --- a/unittests/Support/Path.cpp +++ b/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 Buf(ToRead, '?'); + Expected FD = fs::openNativeFileForRead(NonExistantFile); + if (!FD) + return FD.takeError(); + auto Close = make_scope_exit([&] { fs::closeFile(*FD); }); + if (Expected 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 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> { - 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 Buf(ToRead, '?'); + if (Expected 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) { -- 2.40.0