[libc-commits] [libc] 7776fba - [libc][NFC] Adjust use of off_t internally (#68269)
via libc-commits
libc-commits at lists.llvm.org
Mon Jul 8 09:19:10 PDT 2024
Author: Mikhail R. Gadelha
Date: 2024-07-08T13:19:06-03:00
New Revision: 7776fba473a216b2d1a765491bdc5db710cdff8f
URL: https://github.com/llvm/llvm-project/commit/7776fba473a216b2d1a765491bdc5db710cdff8f
DIFF: https://github.com/llvm/llvm-project/commit/7776fba473a216b2d1a765491bdc5db710cdff8f.diff
LOG: [libc][NFC] Adjust use of off_t internally (#68269)
This patch includes changes related to the use of off_t in libc,
targeted at 32-bit systems: in several places, the offset is used either
as a long or an off_t (64-bit signed int), but in 32-bit systems a long
type is only 32 bits long.
Fix a warning in mmap where a long offset is expected, but we were
passing an off_t. A static_cast and a comment were added to explain
that we know we are ignoring the upper 32-bit of the off_t in 32-bit
systems.
The code in pread and pwrite was slightly improved to remove a
#ifdef LIBC_TARGET_ARCH_IS_RISCV32; we are using an if constexpr now.
The Linux file operations were changed to use off_t instead of a long
where applicable. No changes were made to the standard API, e.g.,
ftell returns the offset as an int so we added a static_cast and a
comment explaining that this will cause a loss of integer precision
in 32-bit systems.
Added:
Modified:
libc/src/__support/File/file.cpp
libc/src/__support/File/file.h
libc/src/__support/File/linux/file.cpp
libc/src/__support/File/linux/file.h
libc/src/stdio/fopencookie.cpp
libc/src/stdio/generic/ftell.cpp
libc/src/sys/mman/linux/mmap.cpp
libc/src/unistd/linux/pread.cpp
libc/src/unistd/linux/pwrite.cpp
libc/test/src/__support/File/file_test.cpp
Removed:
################################################################################
diff --git a/libc/src/__support/File/file.cpp b/libc/src/__support/File/file.cpp
index 58097d017a23f..0b1a8cac21f77 100644
--- a/libc/src/__support/File/file.cpp
+++ b/libc/src/__support/File/file.cpp
@@ -305,23 +305,21 @@ ErrorOr<int> File::seek(long offset, int whence) {
auto result = platform_seek(this, offset, whence);
if (!result.has_value())
return Error(result.error());
- else
- return 0;
+ return 0;
}
-ErrorOr<long> File::tell() {
+ErrorOr<off_t> File::tell() {
FileLock lock(this);
auto seek_target = eof ? SEEK_END : SEEK_CUR;
auto result = platform_seek(this, 0, seek_target);
if (!result.has_value() || result.value() < 0)
return Error(result.error());
- long platform_offset = result.value();
+ off_t platform_offset = result.value();
if (prev_op == FileOp::READ)
return platform_offset - (read_limit - pos);
- else if (prev_op == FileOp::WRITE)
+ if (prev_op == FileOp::WRITE)
return platform_offset + pos;
- else
- return platform_offset;
+ return platform_offset;
}
int File::flush_unlocked() {
diff --git a/libc/src/__support/File/file.h b/libc/src/__support/File/file.h
index 06154871485ce..ec543ac1ac5f3 100644
--- a/libc/src/__support/File/file.h
+++ b/libc/src/__support/File/file.h
@@ -16,6 +16,7 @@
#include <stddef.h>
#include <stdint.h>
+#include <unistd.h> // For off_t.
namespace LIBC_NAMESPACE {
@@ -45,7 +46,7 @@ class File {
using ReadFunc = FileIOResult(File *, void *, size_t);
// The SeekFunc is expected to return the current offset of the external
// file position indicator.
- using SeekFunc = ErrorOr<long>(File *, long, int);
+ using SeekFunc = ErrorOr<off_t>(File *, off_t, int);
using CloseFunc = int(File *);
using ModeFlags = uint32_t;
@@ -182,7 +183,7 @@ class File {
ErrorOr<int> seek(long offset, int whence);
- ErrorOr<long> tell();
+ ErrorOr<off_t> tell();
// If buffer has data written to it, flush it out. Does nothing if the
// buffer is currently being used as a read buffer.
diff --git a/libc/src/__support/File/linux/file.cpp b/libc/src/__support/File/linux/file.cpp
index 00ff93846c6bb..0f6ed4f0a5ef4 100644
--- a/libc/src/__support/File/linux/file.cpp
+++ b/libc/src/__support/File/linux/file.cpp
@@ -42,7 +42,7 @@ FileIOResult linux_file_read(File *f, void *buf, size_t size) {
return ret;
}
-ErrorOr<long> linux_file_seek(File *f, long offset, int whence) {
+ErrorOr<off_t> linux_file_seek(File *f, off_t offset, int whence) {
auto *lf = reinterpret_cast<LinuxFile *>(f);
auto result = internal::lseekimpl(lf->get_fd(), offset, whence);
if (!result.has_value())
diff --git a/libc/src/__support/File/linux/file.h b/libc/src/__support/File/linux/file.h
index 7d3770e1cdd71..63b820529932b 100644
--- a/libc/src/__support/File/linux/file.h
+++ b/libc/src/__support/File/linux/file.h
@@ -12,7 +12,7 @@ namespace LIBC_NAMESPACE {
FileIOResult linux_file_write(File *, const void *, size_t);
FileIOResult linux_file_read(File *, void *, size_t);
-ErrorOr<long> linux_file_seek(File *, long, int);
+ErrorOr<off_t> linux_file_seek(File *, off_t, int);
int linux_file_close(File *);
class LinuxFile : public File {
diff --git a/libc/src/stdio/fopencookie.cpp b/libc/src/stdio/fopencookie.cpp
index 2cb7ad2f46ebf..17b8ae199d3db 100644
--- a/libc/src/stdio/fopencookie.cpp
+++ b/libc/src/stdio/fopencookie.cpp
@@ -24,7 +24,7 @@ class CookieFile : public LIBC_NAMESPACE::File {
static FileIOResult cookie_write(File *f, const void *data, size_t size);
static FileIOResult cookie_read(File *f, void *data, size_t size);
- static ErrorOr<long> cookie_seek(File *f, long offset, int whence);
+ static ErrorOr<off_t> cookie_seek(File *f, off_t offset, int whence);
static int cookie_close(File *f);
public:
@@ -52,7 +52,7 @@ FileIOResult CookieFile::cookie_read(File *f, void *data, size_t size) {
reinterpret_cast<char *>(data), size);
}
-ErrorOr<long> CookieFile::cookie_seek(File *f, long offset, int whence) {
+ErrorOr<off_t> CookieFile::cookie_seek(File *f, off_t offset, int whence) {
auto cookie_file = reinterpret_cast<CookieFile *>(f);
if (cookie_file->ops.seek == nullptr) {
return Error(EINVAL);
@@ -61,8 +61,7 @@ ErrorOr<long> CookieFile::cookie_seek(File *f, long offset, int whence) {
int result = cookie_file->ops.seek(cookie_file->cookie, &offset64, whence);
if (result == 0)
return offset64;
- else
- return -1;
+ return -1;
}
int CookieFile::cookie_close(File *f) {
diff --git a/libc/src/stdio/generic/ftell.cpp b/libc/src/stdio/generic/ftell.cpp
index d55bad2828541..16487cabf6380 100644
--- a/libc/src/stdio/generic/ftell.cpp
+++ b/libc/src/stdio/generic/ftell.cpp
@@ -19,7 +19,11 @@ LLVM_LIBC_FUNCTION(long, ftell, (::FILE * stream)) {
libc_errno = result.error();
return -1;
}
- return result.value();
+ // tell() returns an off_t (64-bit signed integer), but this function returns
+ // a long (32-bit signed integer in 32-bit systems). We add a cast here to
+ // silence a "implicit conversion loses integer precision" warning when
+ // compiling for 32-bit systems.
+ return static_cast<long>(result.value());
}
} // namespace LIBC_NAMESPACE
diff --git a/libc/src/sys/mman/linux/mmap.cpp b/libc/src/sys/mman/linux/mmap.cpp
index 16111c66859f5..2aa7003f342a9 100644
--- a/libc/src/sys/mman/linux/mmap.cpp
+++ b/libc/src/sys/mman/linux/mmap.cpp
@@ -39,9 +39,12 @@ LLVM_LIBC_FUNCTION(void *, mmap,
#error "mmap or mmap2 syscalls not available."
#endif
+ // We add an explicit cast to silence a "implicit conversion loses integer
+ // precision" warning when compiling for 32-bit systems.
+ long mmap_offset = static_cast<long>(offset);
long ret =
LIBC_NAMESPACE::syscall_impl(syscall_number, reinterpret_cast<long>(addr),
- size, prot, flags, fd, offset);
+ size, prot, flags, fd, mmap_offset);
// The mmap/mmap2 syscalls return negative values on error. These negative
// values are actually the negative values of the error codes. So, fix them
diff --git a/libc/src/unistd/linux/pread.cpp b/libc/src/unistd/linux/pread.cpp
index 11cefc5c2f3a8..247a7b4d9d509 100644
--- a/libc/src/unistd/linux/pread.cpp
+++ b/libc/src/unistd/linux/pread.cpp
@@ -19,15 +19,21 @@ namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(ssize_t, pread,
(int fd, void *buf, size_t count, off_t offset)) {
-#ifdef LIBC_TARGET_ARCH_IS_RISCV32
- static_assert(sizeof(off_t) == 8);
- ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(
- SYS_pread64, fd, buf, count, (long)offset,
- (long)(((uint64_t)(offset)) >> 32));
-#else
- ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pread64, fd, buf,
- count, offset);
-#endif
+ ssize_t ret;
+ if constexpr (sizeof(long) == sizeof(uint32_t) &&
+ sizeof(off_t) == sizeof(uint64_t)) {
+ // This is a 32-bit system with a 64-bit offset, offset must be split.
+ const uint64_t bits = cpp::bit_cast<uint64_t>(offset);
+ const uint32_t lo = bits & UINT32_MAX;
+ const uint32_t hi = bits >> 32;
+ const long offset_low = cpp::bit_cast<long>(static_cast<long>(lo));
+ const long offset_high = cpp::bit_cast<long>(static_cast<long>(hi));
+ ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pread64, fd, buf, count,
+ offset_low, offset_high);
+ } else {
+ ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pread64, fd, buf, count,
+ offset);
+ }
// The cast is important since there is a check that dereferences the pointer
// which fails on void*.
MSAN_UNPOISON(reinterpret_cast<char *>(buf), count);
diff --git a/libc/src/unistd/linux/pwrite.cpp b/libc/src/unistd/linux/pwrite.cpp
index 6c6a0b555ac13..b0540a09710e7 100644
--- a/libc/src/unistd/linux/pwrite.cpp
+++ b/libc/src/unistd/linux/pwrite.cpp
@@ -19,15 +19,23 @@ namespace LIBC_NAMESPACE {
LLVM_LIBC_FUNCTION(ssize_t, pwrite,
(int fd, const void *buf, size_t count, off_t offset)) {
-#ifdef LIBC_TARGET_ARCH_IS_RISCV32
- static_assert(sizeof(off_t) == 8);
- ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(
- SYS_pwrite64, fd, buf, count, (long)offset,
- (long)(((uint64_t)(offset)) >> 32));
-#else
- ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pwrite64, fd, buf,
- count, offset);
-#endif
+
+ ssize_t ret;
+ if constexpr (sizeof(long) == sizeof(uint32_t) &&
+ sizeof(off_t) == sizeof(uint64_t)) {
+ // This is a 32-bit system with a 64-bit offset, offset must be split.
+ const uint64_t bits = cpp::bit_cast<uint64_t>(offset);
+ const uint32_t lo = bits & UINT32_MAX;
+ const uint32_t hi = bits >> 32;
+ const long offset_low = cpp::bit_cast<long>(static_cast<long>(lo));
+ const long offset_high = cpp::bit_cast<long>(static_cast<long>(hi));
+ ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pwrite64, fd, buf, count,
+ offset_low, offset_high);
+ } else {
+ ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pwrite64, fd, buf, count,
+ offset);
+ }
+
if (ret < 0) {
libc_errno = static_cast<int>(-ret);
return -1;
diff --git a/libc/test/src/__support/File/file_test.cpp b/libc/test/src/__support/File/file_test.cpp
index fbcedc163de10..2f68c3faa0ad0 100644
--- a/libc/test/src/__support/File/file_test.cpp
+++ b/libc/test/src/__support/File/file_test.cpp
@@ -31,8 +31,8 @@ class StringFile : public File {
static FileIOResult str_read(LIBC_NAMESPACE::File *f, void *data, size_t len);
static FileIOResult str_write(LIBC_NAMESPACE::File *f, const void *data,
size_t len);
- static ErrorOr<long> str_seek(LIBC_NAMESPACE::File *f, long offset,
- int whence);
+ static ErrorOr<off_t> str_seek(LIBC_NAMESPACE::File *f, off_t offset,
+ int whence);
static int str_close(LIBC_NAMESPACE::File *f) {
delete reinterpret_cast<StringFile *>(f);
return 0;
@@ -93,8 +93,8 @@ FileIOResult StringFile::str_write(LIBC_NAMESPACE::File *f, const void *data,
return i;
}
-ErrorOr<long> StringFile::str_seek(LIBC_NAMESPACE::File *f, long offset,
- int whence) {
+ErrorOr<off_t> StringFile::str_seek(LIBC_NAMESPACE::File *f, off_t offset,
+ int whence) {
StringFile *sf = static_cast<StringFile *>(f);
if (whence == SEEK_SET)
sf->pos = offset;
More information about the libc-commits
mailing list