zturner created this revision.
zturner added reviewers: clayborg, beanz, EugeneBi.
zturner added a subscriber: lldb-commits.
The original motivation for this came from https://reviews.llvm.org/D25712, in
which Eugene pointed out that `File::Read()` does not correctly handle short
reads. However, I felt the fix was incomplete because it left the bug in other
functions, and the code in general could have used some cleanup since there was
a ton of duplication, which may be what led to this bug showing up in the first
place.
Changes in this patch are:
1. Have the normal `Read()` and `Write()` functions delegate to the versions
that read and write with offset.
2. Supply thread-safe versions of the Windows codepaths, which were previously
incorrect in a multi-threaded environment.
3. Delete a bunch of dead functions that are not used anywhere in LLDB.
4. Remove the apple specific path due to `MAX_READ_SIZE` and `MAX_WRITE_SIZE`
and merge with the standard non-Windows path.
The only real tricky part about this patch was that when you open a file in
append mode, the old version of `Write()` with no offset would write at the
end, whereas `pwrite()` always writes at the offset you specify. So to fix
this I made the version of `Write()` with no offset explicitly compute the
offset of the end of the file and pass that to the offset-version of `Write()`.
https://reviews.llvm.org/D25783
Files:
include/lldb/Host/File.h
source/Host/common/File.cpp
Index: source/Host/common/File.cpp
===================================================================
--- source/Host/common/File.cpp
+++ source/Host/common/File.cpp
@@ -367,58 +367,6 @@
return result;
}
-off_t File::SeekFromCurrent(off_t offset, Error *error_ptr) {
- off_t result = -1;
- if (DescriptorIsValid()) {
- result = ::lseek(m_descriptor, offset, SEEK_CUR);
-
- if (error_ptr) {
- if (result == -1)
- error_ptr->SetErrorToErrno();
- else
- error_ptr->Clear();
- }
- } else if (StreamIsValid()) {
- result = ::fseek(m_stream, offset, SEEK_CUR);
-
- if (error_ptr) {
- if (result == -1)
- error_ptr->SetErrorToErrno();
- else
- error_ptr->Clear();
- }
- } else if (error_ptr) {
- error_ptr->SetErrorString("invalid file handle");
- }
- return result;
-}
-
-off_t File::SeekFromEnd(off_t offset, Error *error_ptr) {
- off_t result = -1;
- if (DescriptorIsValid()) {
- result = ::lseek(m_descriptor, offset, SEEK_END);
-
- if (error_ptr) {
- if (result == -1)
- error_ptr->SetErrorToErrno();
- else
- error_ptr->Clear();
- }
- } else if (StreamIsValid()) {
- result = ::fseek(m_stream, offset, SEEK_END);
-
- if (error_ptr) {
- if (result == -1)
- error_ptr->SetErrorToErrno();
- else
- error_ptr->Clear();
- }
- } else if (error_ptr) {
- error_ptr->SetErrorString("invalid file handle");
- }
- return result;
-}
-
Error File::Flush() {
Error error;
if (StreamIsValid()) {
@@ -435,218 +383,103 @@
return error;
}
-Error File::Sync() {
- Error error;
- if (DescriptorIsValid()) {
-#ifdef _WIN32
- int err = FlushFileBuffers((HANDLE)_get_osfhandle(m_descriptor));
- if (err == 0)
- error.SetErrorToGenericError();
-#else
- int err = 0;
- do {
- err = ::fsync(m_descriptor);
- } while (err == -1 && errno == EINTR);
-
- if (err == -1)
- error.SetErrorToErrno();
-#endif
- } else {
- error.SetErrorString("invalid file handle");
- }
- return error;
-}
-
#if defined(__APPLE__)
// Darwin kernels only can read/write <= INT_MAX bytes
-#define MAX_READ_SIZE INT_MAX
-#define MAX_WRITE_SIZE INT_MAX
+constexpr size_t MAX_READ_SIZE = INT_MAX;
+constexpr size_t MAX_WRITE_SIZE = INT_MAX;
+#elif defined(LLVM_ON_WIN32)
+constexpr size_t MAX_READ_SIZE = ULONG_MAX;
+constexpr size_t MAX_WRITE_SIZE = ULONG_MAX;
+#else
+constexpr size_t MAX_READ_SIZE = SIZE_MAX;
+constexpr size_t MAX_WRITE_SIZE = SIZE_MAX;
#endif
Error File::Read(void *buf, size_t &num_bytes) {
- Error error;
-
-#if defined(MAX_READ_SIZE)
- if (num_bytes > MAX_READ_SIZE) {
- uint8_t *p = (uint8_t *)buf;
- size_t bytes_left = num_bytes;
- // Init the num_bytes read to zero
+ int fd = GetDescriptor();
+ if (fd == kInvalidDescriptor) {
num_bytes = 0;
-
- while (bytes_left > 0) {
- size_t curr_num_bytes;
- if (bytes_left > MAX_READ_SIZE)
- curr_num_bytes = MAX_READ_SIZE;
- else
- curr_num_bytes = bytes_left;
-
- error = Read(p + num_bytes, curr_num_bytes);
-
- // Update how many bytes were read
- num_bytes += curr_num_bytes;
- if (bytes_left < curr_num_bytes)
- bytes_left = 0;
- else
- bytes_left -= curr_num_bytes;
-
- if (error.Fail())
- break;
- }
- return error;
+ return Error("invalid file handle");
}
-#endif
-
- ssize_t bytes_read = -1;
- if (DescriptorIsValid()) {
- do {
- bytes_read = ::read(m_descriptor, buf, num_bytes);
- } while (bytes_read < 0 && errno == EINTR);
-
- if (bytes_read == -1) {
- error.SetErrorToErrno();
- num_bytes = 0;
- } else
- num_bytes = bytes_read;
- } else if (StreamIsValid()) {
- bytes_read = ::fread(buf, 1, num_bytes, m_stream);
- if (bytes_read == 0) {
- if (::feof(m_stream))
- error.SetErrorString("feof");
- else if (::ferror(m_stream))
- error.SetErrorString("ferror");
- num_bytes = 0;
- } else
- num_bytes = bytes_read;
- } else {
- num_bytes = 0;
- error.SetErrorString("invalid file handle");
- }
- return error;
+ off_t offset = lseek(fd, 0, SEEK_CUR);
+ return Read(buf, num_bytes, offset);
}
Error File::Write(const void *buf, size_t &num_bytes) {
- Error error;
-
-#if defined(MAX_WRITE_SIZE)
- if (num_bytes > MAX_WRITE_SIZE) {
- const uint8_t *p = (const uint8_t *)buf;
- size_t bytes_left = num_bytes;
- // Init the num_bytes written to zero
+ int fd = GetDescriptor();
+ if (fd == kInvalidDescriptor) {
num_bytes = 0;
-
- while (bytes_left > 0) {
- size_t curr_num_bytes;
- if (bytes_left > MAX_WRITE_SIZE)
- curr_num_bytes = MAX_WRITE_SIZE;
- else
- curr_num_bytes = bytes_left;
-
- error = Write(p + num_bytes, curr_num_bytes);
-
- // Update how many bytes were read
- num_bytes += curr_num_bytes;
- if (bytes_left < curr_num_bytes)
- bytes_left = 0;
- else
- bytes_left -= curr_num_bytes;
-
- if (error.Fail())
- break;
- }
- return error;
+ return Error("invalid file handle");
}
-#endif
- ssize_t bytes_written = -1;
- if (DescriptorIsValid()) {
- do {
- bytes_written = ::write(m_descriptor, buf, num_bytes);
- } while (bytes_written < 0 && errno == EINTR);
-
- if (bytes_written == -1) {
- error.SetErrorToErrno();
- num_bytes = 0;
- } else
- num_bytes = bytes_written;
- } else if (StreamIsValid()) {
- bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
-
- if (bytes_written == 0) {
- if (::feof(m_stream))
- error.SetErrorString("feof");
- else if (::ferror(m_stream))
- error.SetErrorString("ferror");
- num_bytes = 0;
- } else
- num_bytes = bytes_written;
-
- } else {
- num_bytes = 0;
- error.SetErrorString("invalid file handle");
+ off_t offset = lseek(fd, 0, SEEK_CUR);
+ if (m_options & File::eOpenOptionAppend) {
+ struct stat S;
+ if (::fstat(fd, &S) != 0)
+ return Error("Invalid file handle!");
+ offset = S.st_size;
}
-
- return error;
+ return Write(buf, num_bytes, offset);
}
Error File::Read(void *buf, size_t &num_bytes, off_t &offset) {
Error error;
-
-#if defined(MAX_READ_SIZE)
- if (num_bytes > MAX_READ_SIZE) {
- uint8_t *p = (uint8_t *)buf;
- size_t bytes_left = num_bytes;
- // Init the num_bytes read to zero
+ size_t bytes_left = num_bytes;
+ int fd = GetDescriptor();
+ if (fd == kInvalidDescriptor) {
num_bytes = 0;
-
- while (bytes_left > 0) {
- size_t curr_num_bytes;
- if (bytes_left > MAX_READ_SIZE)
- curr_num_bytes = MAX_READ_SIZE;
- else
- curr_num_bytes = bytes_left;
-
- error = Read(p + num_bytes, curr_num_bytes, offset);
-
- // Update how many bytes were read
- num_bytes += curr_num_bytes;
- if (bytes_left < curr_num_bytes)
- bytes_left = 0;
- else
- bytes_left -= curr_num_bytes;
-
- if (error.Fail())
- break;
- }
- return error;
+ return Error("invalid file handle");
}
-#endif
+ uint8_t *dest = static_cast<uint8_t *>(buf);
#ifndef _WIN32
- int fd = GetDescriptor();
- if (fd != kInvalidDescriptor) {
- ssize_t bytes_read = -1;
+ ssize_t bytes_read = -1;
+ size_t bytes_to_read = std::min(MAX_READ_SIZE, bytes_left);
+ while (bytes_left != 0 && bytes_read != 0) {
do {
- bytes_read = ::pread(fd, buf, num_bytes, offset);
+ bytes_read = ::pread(fd, dest, num_bytes, offset);
} while (bytes_read < 0 && errno == EINTR);
if (bytes_read < 0) {
num_bytes = 0;
error.SetErrorToErrno();
- } else {
- offset += bytes_read;
- num_bytes = bytes_read;
+ return error;
}
- } else {
- num_bytes = 0;
- error.SetErrorString("invalid file handle");
+
+ dest += bytes_read;
+ offset += bytes_read;
+ bytes_left -= bytes_read;
}
+ num_bytes -= bytes_left;
#else
- long cur = ::lseek(m_descriptor, 0, SEEK_CUR);
- SeekFromStart(offset);
- error = Read(buf, num_bytes);
- if (!error.Fail())
- SeekFromStart(cur);
+ HANDLE H = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+ if (!H || H == INVALID_HANDLE_VALUE)
+ return Error("invalid file handle");
+
+ LARGE_INTEGER large_off;
+ large_off.QuadPart = offset;
+
+ OVERLAPPED OV = {};
+ OV.Offset = large_off.LowPart;
+ OV.OffsetHigh = large_off.HighPart;
+
+ while (bytes_left != 0) {
+ DWORD req_size = static_cast<DWORD>(std::min(MAX_READ_SIZE, bytes_left));
+ DWORD actual_size = 0;
+ BOOL result = ::ReadFile(H, dest, req_size, &actual_size, &OV);
+ if (!result) {
+ error.SetError(GetLastError(), eErrorTypeWin32);
+ return error;
+ }
+ if (actual_size == 0) {
+ // EOF was reached.
+ break;
+ }
+ dest += actual_size;
+ bytes_left -= actual_size;
+ }
+ num_bytes -= bytes_left;
#endif
return error;
}
@@ -697,66 +530,56 @@
Error File::Write(const void *buf, size_t &num_bytes, off_t &offset) {
Error error;
-
-#if defined(MAX_WRITE_SIZE)
- if (num_bytes > MAX_WRITE_SIZE) {
- const uint8_t *p = (const uint8_t *)buf;
- size_t bytes_left = num_bytes;
- // Init the num_bytes written to zero
+ size_t bytes_left = num_bytes;
+ int fd = GetDescriptor();
+ if (fd == kInvalidDescriptor) {
num_bytes = 0;
-
- while (bytes_left > 0) {
- size_t curr_num_bytes;
- if (bytes_left > MAX_WRITE_SIZE)
- curr_num_bytes = MAX_WRITE_SIZE;
- else
- curr_num_bytes = bytes_left;
-
- error = Write(p + num_bytes, curr_num_bytes, offset);
-
- // Update how many bytes were read
- num_bytes += curr_num_bytes;
- if (bytes_left < curr_num_bytes)
- bytes_left = 0;
- else
- bytes_left -= curr_num_bytes;
-
- if (error.Fail())
- break;
- }
- return error;
+ return Error("invalid file handle");
}
-#endif
- int fd = GetDescriptor();
- if (fd != kInvalidDescriptor) {
+ const uint8_t *src = static_cast<const uint8_t *>(buf);
#ifndef _WIN32
- ssize_t bytes_written = -1;
+ ssize_t bytes_written = -1;
+ size_t bytes_to_write = std::min(MAX_WRITE_SIZE, bytes_left);
+ while (bytes_left != 0) {
do {
- bytes_written = ::pwrite(m_descriptor, buf, num_bytes, offset);
+ bytes_written = ::pwrite(fd, src, num_bytes, offset);
} while (bytes_written < 0 && errno == EINTR);
if (bytes_written < 0) {
num_bytes = 0;
error.SetErrorToErrno();
- } else {
- offset += bytes_written;
- num_bytes = bytes_written;
+ return error;
}
-#else
- long cur = ::lseek(m_descriptor, 0, SEEK_CUR);
- error = Write(buf, num_bytes);
- long after = ::lseek(m_descriptor, 0, SEEK_CUR);
- if (!error.Fail())
- SeekFromStart(cur);
-
- offset = after;
-#endif
- } else {
- num_bytes = 0;
- error.SetErrorString("invalid file handle");
+ src += bytes_written;
+ offset += bytes_written;
+ bytes_left -= bytes_written;
}
+#else
+ HANDLE H = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+ if (!H || H == INVALID_HANDLE_VALUE)
+ return Error("invalid file handle");
+
+ LARGE_INTEGER large_off;
+ large_off.QuadPart = offset;
+
+ OVERLAPPED OV = {};
+ OV.Offset = large_off.LowPart;
+ OV.OffsetHigh = large_off.HighPart;
+
+ while (bytes_left != 0) {
+ DWORD req_size = static_cast<DWORD>(std::min(MAX_WRITE_SIZE, bytes_left));
+ DWORD actual_size = 0;
+ BOOL result = ::WriteFile(H, src, req_size, &actual_size, &OV);
+ if (!result) {
+ error.SetError(GetLastError(), eErrorTypeWin32);
+ return error;
+ }
+ src += actual_size;
+ bytes_left -= actual_size;
+ }
+#endif
return error;
}
Index: include/lldb/Host/File.h
===================================================================
--- include/lldb/Host/File.h
+++ include/lldb/Host/File.h
@@ -265,51 +265,6 @@
off_t SeekFromStart(off_t offset, Error *error_ptr = nullptr);
//------------------------------------------------------------------
- /// Seek to an offset relative to the current file position.
- ///
- /// NOTE: This function is NOT thread safe, other threads that
- /// access this object might also change the current file position.
- /// For thread safe reads and writes see the following functions:
- /// @see File::Read (void *, size_t, off_t &)
- /// @see File::Write (const void *, size_t, off_t &)
- ///
- /// @param[in] offset
- /// The offset to seek to within the file relative to the
- /// current file position.
- ///
- /// @param[in] error_ptr
- /// A pointer to a lldb_private::Error object that will be
- /// filled in if non-nullptr.
- ///
- /// @return
- /// The resulting seek offset, or -1 on error.
- //------------------------------------------------------------------
- off_t SeekFromCurrent(off_t offset, Error *error_ptr = nullptr);
-
- //------------------------------------------------------------------
- /// Seek to an offset relative to the end of the file.
- ///
- /// NOTE: This function is NOT thread safe, other threads that
- /// access this object might also change the current file position.
- /// For thread safe reads and writes see the following functions:
- /// @see File::Read (void *, size_t, off_t &)
- /// @see File::Write (const void *, size_t, off_t &)
- ///
- /// @param[in,out] offset
- /// The offset to seek to within the file relative to the
- /// end of the file which gets filled in with the resulting
- /// absolute file offset.
- ///
- /// @param[in] error_ptr
- /// A pointer to a lldb_private::Error object that will be
- /// filled in if non-nullptr.
- ///
- /// @return
- /// The resulting seek offset, or -1 on error.
- //------------------------------------------------------------------
- off_t SeekFromEnd(off_t offset, Error *error_ptr = nullptr);
-
- //------------------------------------------------------------------
/// Read bytes from a file from the specified file offset.
///
/// NOTE: This function is thread safe in that clients manager their
@@ -403,15 +358,6 @@
Error Flush();
//------------------------------------------------------------------
- /// Sync to disk.
- ///
- /// @return
- /// An error object that indicates success or the reason for
- /// failure.
- //------------------------------------------------------------------
- Error Sync();
-
- //------------------------------------------------------------------
/// Get the permissions for a this file.
///
/// @return
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
