Skip to content

Commit 1b30ea2

Browse files
labathlabath
committedAug 22, 2019
[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
·
llvmorg-21-initllvmorg-10.0.0-rc1
1 parent 7c6b229 commit 1b30ea2

File tree

6 files changed

+154
-121
lines changed

6 files changed

+154
-121
lines changed
 

‎llvm/include/llvm/Support/FileSystem.h

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -991,29 +991,27 @@ file_t getStdoutHandle();
991991
/// Returns kInvalidFile when the stream is closed.
992992
file_t getStderrHandle();
993993

994-
/// Reads \p Buf.size() bytes from \p FileHandle into \p Buf. The number of
995-
/// bytes actually read is returned in \p BytesRead. On Unix, this is equivalent
996-
/// to `*BytesRead = ::read(FD, Buf.data(), Buf.size())`, with error reporting.
997-
/// BytesRead will contain zero when reaching EOF.
994+
/// Reads \p Buf.size() bytes from \p FileHandle into \p Buf. Returns the number
995+
/// of bytes actually read. On Unix, this is equivalent to `return ::read(FD,
996+
/// Buf.data(), Buf.size())`, with error reporting. Returns 0 when reaching EOF.
998997
///
999998
/// @param FileHandle File to read from.
1000999
/// @param Buf Buffer to read into.
1001-
/// @param BytesRead Output parameter of the number of bytes read.
1002-
/// @returns The error, if any, or errc::success.
1003-
std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
1004-
size_t *BytesRead);
1000+
/// @returns The number of bytes read, or error.
1001+
Expected<size_t> readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf);
10051002

10061003
/// Reads \p Buf.size() bytes from \p FileHandle at offset \p Offset into \p
10071004
/// Buf. If 'pread' is available, this will use that, otherwise it will use
1008-
/// 'lseek'. Bytes requested beyond the end of the file will be zero
1009-
/// initialized.
1005+
/// 'lseek'. Returns the number of bytes actually read. Returns 0 when reaching
1006+
/// EOF.
10101007
///
10111008
/// @param FileHandle File to read from.
10121009
/// @param Buf Buffer to read into.
10131010
/// @param Offset Offset into the file at which the read should occur.
1014-
/// @returns The error, if any, or errc::success.
1015-
std::error_code readNativeFileSlice(file_t FileHandle,
1016-
MutableArrayRef<char> Buf, size_t Offset);
1011+
/// @returns The number of bytes read, or error.
1012+
Expected<size_t> readNativeFileSlice(file_t FileHandle,
1013+
MutableArrayRef<char> Buf,
1014+
uint64_t Offset);
10171015

10181016
/// @brief Opens the file with the given name in a write-only or read-write
10191017
/// mode, returning its open file descriptor. If the file does not exist, it

‎llvm/lib/Support/MemoryBuffer.cpp

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -211,15 +211,17 @@ static ErrorOr<std::unique_ptr<WritableMemoryBuffer>>
211211
getMemoryBufferForStream(sys::fs::file_t FD, const Twine &BufferName) {
212212
const ssize_t ChunkSize = 4096*4;
213213
SmallString<ChunkSize> Buffer;
214-
size_t ReadBytes;
215214
// Read into Buffer until we hit EOF.
216-
do {
215+
for (;;) {
217216
Buffer.reserve(Buffer.size() + ChunkSize);
218-
if (auto EC = sys::fs::readNativeFile(
219-
FD, makeMutableArrayRef(Buffer.end(), ChunkSize), &ReadBytes))
220-
return EC;
221-
Buffer.set_size(Buffer.size() + ReadBytes);
222-
} while (ReadBytes != 0);
217+
Expected<size_t> ReadBytes = sys::fs::readNativeFile(
218+
FD, makeMutableArrayRef(Buffer.end(), ChunkSize));
219+
if (!ReadBytes)
220+
return errorToErrorCode(ReadBytes.takeError());
221+
if (*ReadBytes == 0)
222+
break;
223+
Buffer.set_size(Buffer.size() + *ReadBytes);
224+
}
223225

224226
return getMemBufferCopyImpl(Buffer, BufferName);
225227
}
@@ -458,9 +460,20 @@ getOpenFileImpl(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
458460
return make_error_code(errc::not_enough_memory);
459461
}
460462

461-
if (std::error_code EC =
462-
sys::fs::readNativeFileSlice(FD, Buf->getBuffer(), Offset))
463-
return EC;
463+
// Read until EOF, zero-initialize the rest.
464+
MutableArrayRef<char> ToRead = Buf->getBuffer();
465+
while (!ToRead.empty()) {
466+
Expected<size_t> ReadBytes =
467+
sys::fs::readNativeFileSlice(FD, ToRead, Offset);
468+
if (!ReadBytes)
469+
return errorToErrorCode(ReadBytes.takeError());
470+
if (*ReadBytes == 0) {
471+
std::memset(ToRead.data(), 0, ToRead.size());
472+
break;
473+
}
474+
ToRead = ToRead.drop_front(*ReadBytes);
475+
Offset += *ReadBytes;
476+
}
464477

465478
return std::move(Buf);
466479
}

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

Lines changed: 17 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -999,44 +999,28 @@ file_t getStdinHandle() { return 0; }
999999
file_t getStdoutHandle() { return 1; }
10001000
file_t getStderrHandle() { return 2; }
10011001

1002-
std::error_code readNativeFile(file_t FD, MutableArrayRef<char> Buf,
1003-
size_t *BytesRead) {
1004-
*BytesRead = sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Buf.size());
1005-
if (ssize_t(*BytesRead) == -1)
1006-
return std::error_code(errno, std::generic_category());
1007-
return std::error_code();
1002+
Expected<size_t> readNativeFile(file_t FD, MutableArrayRef<char> Buf) {
1003+
ssize_t NumRead =
1004+
sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Buf.size());
1005+
if (ssize_t(NumRead) == -1)
1006+
return errorCodeToError(std::error_code(errno, std::generic_category()));
1007+
return NumRead;
10081008
}
10091009

1010-
std::error_code readNativeFileSlice(file_t FD, MutableArrayRef<char> Buf,
1011-
size_t Offset) {
1012-
char *BufPtr = Buf.data();
1013-
size_t BytesLeft = Buf.size();
1014-
1015-
#ifndef HAVE_PREAD
1016-
// If we don't have pread, seek to Offset.
1017-
if (lseek(FD, Offset, SEEK_SET) == -1)
1018-
return std::error_code(errno, std::generic_category());
1019-
#endif
1020-
1021-
while (BytesLeft) {
1010+
Expected<size_t> readNativeFileSlice(file_t FD, MutableArrayRef<char> Buf,
1011+
uint64_t Offset) {
10221012
#ifdef HAVE_PREAD
1023-
ssize_t NumRead = sys::RetryAfterSignal(-1, ::pread, FD, BufPtr, BytesLeft,
1024-
Buf.size() - BytesLeft + Offset);
1013+
ssize_t NumRead =
1014+
sys::RetryAfterSignal(-1, ::pread, FD, Buf.data(), Buf.size(), Offset);
10251015
#else
1026-
ssize_t NumRead = sys::RetryAfterSignal(-1, ::read, FD, BufPtr, BytesLeft);
1016+
if (lseek(FD, Offset, SEEK_SET) == -1)
1017+
return errorCodeToError(std::error_code(errno, std::generic_category()));
1018+
ssize_t NumRead =
1019+
sys::RetryAfterSignal(-1, ::read, FD, Buf.data(), Buf.size());
10271020
#endif
1028-
if (NumRead == -1) {
1029-
// Error while reading.
1030-
return std::error_code(errno, std::generic_category());
1031-
}
1032-
if (NumRead == 0) {
1033-
memset(BufPtr, 0, BytesLeft); // zero-initialize rest of the buffer.
1034-
break;
1035-
}
1036-
BytesLeft -= NumRead;
1037-
BufPtr += NumRead;
1038-
}
1039-
return std::error_code();
1021+
if (NumRead == -1)
1022+
return errorCodeToError(std::error_code(errno, std::generic_category()));
1023+
return NumRead;
10401024
}
10411025

10421026
std::error_code closeFile(file_t &F) {

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

Lines changed: 26 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1217,57 +1217,34 @@ file_t getStdinHandle() { return ::GetStdHandle(STD_INPUT_HANDLE); }
12171217
file_t getStdoutHandle() { return ::GetStdHandle(STD_OUTPUT_HANDLE); }
12181218
file_t getStderrHandle() { return ::GetStdHandle(STD_ERROR_HANDLE); }
12191219

1220-
std::error_code readNativeFileImpl(file_t FileHandle, char *BufPtr, size_t BytesToRead,
1221-
size_t *BytesRead, OVERLAPPED *Overlap) {
1220+
Expected<size_t> readNativeFileImpl(file_t FileHandle,
1221+
MutableArrayRef<char> Buf,
1222+
OVERLAPPED *Overlap) {
12221223
// ReadFile can only read 2GB at a time. The caller should check the number of
12231224
// bytes and read in a loop until termination.
1224-
DWORD BytesToRead32 =
1225-
std::min(size_t(std::numeric_limits<DWORD>::max()), BytesToRead);
1226-
DWORD BytesRead32 = 0;
1227-
bool Success =
1228-
::ReadFile(FileHandle, BufPtr, BytesToRead32, &BytesRead32, Overlap);
1229-
*BytesRead = BytesRead32;
1230-
if (!Success) {
1231-
DWORD Err = ::GetLastError();
1232-
// EOF is not an error.
1233-
if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF)
1234-
return std::error_code();
1235-
return mapWindowsError(Err);
1236-
}
1237-
return std::error_code();
1238-
}
1239-
1240-
std::error_code readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf,
1241-
size_t *BytesRead) {
1242-
return readNativeFileImpl(FileHandle, Buf.data(), Buf.size(), BytesRead,
1243-
/*Overlap=*/nullptr);
1244-
}
1245-
1246-
std::error_code readNativeFileSlice(file_t FileHandle,
1247-
MutableArrayRef<char> Buf, size_t Offset) {
1248-
char *BufPtr = Buf.data();
1249-
size_t BytesLeft = Buf.size();
1250-
1251-
while (BytesLeft) {
1252-
uint64_t CurOff = Buf.size() - BytesLeft + Offset;
1253-
OVERLAPPED Overlapped = {};
1254-
Overlapped.Offset = uint32_t(CurOff);
1255-
Overlapped.OffsetHigh = uint32_t(uint64_t(CurOff) >> 32);
1256-
1257-
size_t BytesRead = 0;
1258-
if (auto EC = readNativeFileImpl(FileHandle, BufPtr, BytesLeft, &BytesRead,
1259-
&Overlapped))
1260-
return EC;
1261-
1262-
// Once we reach EOF, zero the remaining bytes in the buffer.
1263-
if (BytesRead == 0) {
1264-
memset(BufPtr, 0, BytesLeft);
1265-
break;
1266-
}
1267-
BytesLeft -= BytesRead;
1268-
BufPtr += BytesRead;
1269-
}
1270-
return std::error_code();
1225+
DWORD BytesToRead =
1226+
std::min(size_t(std::numeric_limits<DWORD>::max()), Buf.size());
1227+
DWORD BytesRead = 0;
1228+
if (::ReadFile(FileHandle, Buf.data(), BytesToRead, &BytesRead, Overlap))
1229+
return BytesRead;
1230+
DWORD Err = ::GetLastError();
1231+
// EOF is not an error.
1232+
if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF)
1233+
return BytesRead;
1234+
return errorCodeToError(mapWindowsError(Err));
1235+
}
1236+
1237+
Expected<size_t> readNativeFile(file_t FileHandle, MutableArrayRef<char> Buf) {
1238+
return readNativeFileImpl(FileHandle, Buf, /*Overlap=*/nullptr);
1239+
}
1240+
1241+
Expected<size_t> readNativeFileSlice(file_t FileHandle,
1242+
MutableArrayRef<char> Buf,
1243+
uint64_t Offset) {
1244+
OVERLAPPED Overlapped = {};
1245+
Overlapped.Offset = uint32_t(Offset);
1246+
Overlapped.OffsetHigh = uint32_t(Offset >> 32);
1247+
return readNativeFileImpl(FileHandle, Buf, &Overlapped);
12711248
}
12721249

12731250
std::error_code closeFile(file_t &F) {

‎llvm/unittests/Support/MemoryBufferTest.cpp

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,15 @@
1717
#include "llvm/Support/raw_ostream.h"
1818
#include "llvm/Testing/Support/Error.h"
1919
#include "gtest/gtest.h"
20+
#if LLVM_ENABLE_THREADS
21+
#include <thread>
22+
#endif
23+
#if LLVM_ON_UNIX
24+
#include <unistd.h>
25+
#endif
26+
#if _WIN32
27+
#include <windows.h>
28+
#endif
2029

2130
using namespace llvm;
2231

@@ -152,6 +161,38 @@ TEST_F(MemoryBufferTest, copy) {
152161
EXPECT_NE(MBC1->getBufferStart(), MBC2->getBufferStart());
153162
}
154163

164+
#if LLVM_ENABLE_THREADS
165+
TEST_F(MemoryBufferTest, createFromPipe) {
166+
sys::fs::file_t pipes[2];
167+
#if LLVM_ON_UNIX
168+
ASSERT_EQ(::pipe(pipes), 0) << strerror(errno);
169+
#else
170+
ASSERT_TRUE(::CreatePipe(&pipes[0], &pipes[1], nullptr, 0))
171+
<< ::GetLastError();
172+
#endif
173+
auto ReadCloser = make_scope_exit([&] { sys::fs::closeFile(pipes[0]); });
174+
std::thread Writer([&] {
175+
auto WriteCloser = make_scope_exit([&] { sys::fs::closeFile(pipes[1]); });
176+
for (unsigned i = 0; i < 5; ++i) {
177+
std::this_thread::sleep_for(std::chrono::milliseconds(10));
178+
#if LLVM_ON_UNIX
179+
ASSERT_EQ(::write(pipes[1], "foo", 3), 3) << strerror(errno);
180+
#else
181+
DWORD Written;
182+
ASSERT_TRUE(::WriteFile(pipes[1], "foo", 3, &Written, nullptr))
183+
<< ::GetLastError();
184+
ASSERT_EQ(Written, 3u);
185+
#endif
186+
}
187+
});
188+
ErrorOr<OwningBuffer> MB =
189+
MemoryBuffer::getOpenFile(pipes[0], "pipe", /*FileSize*/ -1);
190+
Writer.join();
191+
ASSERT_NO_ERROR(MB.getError());
192+
EXPECT_EQ(MB.get()->getBuffer(), "foofoofoofoofoo");
193+
}
194+
#endif
195+
155196
TEST_F(MemoryBufferTest, make_new) {
156197
// 0-sized buffer
157198
OwningBuffer Zero(WritableMemoryBuffer::getNewUninitMemBuffer(0));

‎llvm/unittests/Support/Path.cpp

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include "llvm/Support/Path.h"
1010
#include "llvm/ADT/STLExtras.h"
11+
#include "llvm/ADT/ScopeExit.h"
1112
#include "llvm/ADT/SmallVector.h"
1213
#include "llvm/ADT/Triple.h"
1314
#include "llvm/BinaryFormat/Magic.h"
@@ -1514,28 +1515,47 @@ TEST_F(FileSystemTest, ReadWriteFileCanReadOrWrite) {
15141515
verifyWrite(FD, "Buzz", true);
15151516
}
15161517

1518+
TEST_F(FileSystemTest, readNativeFile) {
1519+
createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "01234");
1520+
FileRemover Cleanup(NonExistantFile);
1521+
const auto &Read = [&](size_t ToRead) -> Expected<std::string> {
1522+
std::string Buf(ToRead, '?');
1523+
Expected<fs::file_t> FD = fs::openNativeFileForRead(NonExistantFile);
1524+
if (!FD)
1525+
return FD.takeError();
1526+
auto Close = make_scope_exit([&] { fs::closeFile(*FD); });
1527+
if (Expected<size_t> BytesRead = fs::readNativeFile(
1528+
*FD, makeMutableArrayRef(&*Buf.begin(), Buf.size())))
1529+
return Buf.substr(0, *BytesRead);
1530+
else
1531+
return BytesRead.takeError();
1532+
};
1533+
EXPECT_THAT_EXPECTED(Read(5), HasValue("01234"));
1534+
EXPECT_THAT_EXPECTED(Read(3), HasValue("012"));
1535+
EXPECT_THAT_EXPECTED(Read(6), HasValue("01234"));
1536+
}
1537+
15171538
TEST_F(FileSystemTest, readNativeFileSlice) {
1518-
char Data[10] = {'0', '1', '2', '3', '4', 0, 0, 0, 0, 0};
1519-
createFileWithData(NonExistantFile, false, fs::CD_CreateNew,
1520-
StringRef(Data, 5));
1539+
createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "01234");
15211540
FileRemover Cleanup(NonExistantFile);
15221541
Expected<fs::file_t> FD = fs::openNativeFileForRead(NonExistantFile);
15231542
ASSERT_THAT_EXPECTED(FD, Succeeded());
1524-
char Buf[10];
1543+
auto Close = make_scope_exit([&] { fs::closeFile(*FD); });
15251544
const auto &Read = [&](size_t Offset,
1526-
size_t ToRead) -> Expected<ArrayRef<char>> {
1527-
std::memset(Buf, 0x47, sizeof(Buf));
1528-
if (std::error_code EC = fs::readNativeFileSlice(
1529-
*FD, makeMutableArrayRef(Buf, ToRead), Offset))
1530-
return errorCodeToError(EC);
1531-
return makeArrayRef(Buf, ToRead);
1545+
size_t ToRead) -> Expected<std::string> {
1546+
std::string Buf(ToRead, '?');
1547+
if (Expected<size_t> BytesRead = fs::readNativeFileSlice(
1548+
*FD, makeMutableArrayRef(&*Buf.begin(), Buf.size()), Offset))
1549+
return Buf.substr(0, *BytesRead);
1550+
else
1551+
return BytesRead.takeError();
15321552
};
1533-
EXPECT_THAT_EXPECTED(Read(0, 5), HasValue(makeArrayRef(Data + 0, 5)));
1534-
EXPECT_THAT_EXPECTED(Read(0, 3), HasValue(makeArrayRef(Data + 0, 3)));
1535-
EXPECT_THAT_EXPECTED(Read(2, 3), HasValue(makeArrayRef(Data + 2, 3)));
1536-
EXPECT_THAT_EXPECTED(Read(0, 6), HasValue(makeArrayRef(Data + 0, 6)));
1537-
EXPECT_THAT_EXPECTED(Read(2, 6), HasValue(makeArrayRef(Data + 2, 6)));
1538-
EXPECT_THAT_EXPECTED(Read(5, 5), HasValue(makeArrayRef(Data + 5, 5)));
1553+
EXPECT_THAT_EXPECTED(Read(0, 5), HasValue("01234"));
1554+
EXPECT_THAT_EXPECTED(Read(0, 3), HasValue("012"));
1555+
EXPECT_THAT_EXPECTED(Read(2, 3), HasValue("234"));
1556+
EXPECT_THAT_EXPECTED(Read(0, 6), HasValue("01234"));
1557+
EXPECT_THAT_EXPECTED(Read(2, 6), HasValue("234"));
1558+
EXPECT_THAT_EXPECTED(Read(5, 5), HasValue(""));
15391559
}
15401560

15411561
TEST_F(FileSystemTest, is_local) {

0 commit comments

Comments
 (0)
Please sign in to comment.