Skip to content

Commit

Permalink
[libc][NFC] Adjust use of off_t internally
Browse files Browse the repository at this point in the history
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.

1. Fix a warning in mmap where a long offset is expected, but we were
   passing a 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.
2. The code in pread and pwrite were slightly improved to remove a
   #ifdef LIBC_TARGET_ARCH_IS_RISCV32; we are using an if constexpr now.
3. 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.
  • Loading branch information
mikhailramalho committed Oct 4, 2023
1 parent cb7cf62 commit 60401c1
Show file tree
Hide file tree
Showing 10 changed files with 53 additions and 41 deletions.
14 changes: 6 additions & 8 deletions libc/src/__support/File/file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ int File::ungetc_unlocked(int c) {
return c;
}

ErrorOr<int> File::seek(long offset, int whence) {
ErrorOr<int> File::seek(off_t offset, int whence) {
FileLock lock(this);
if (prev_op == FileOp::WRITE && pos > 0) {

Expand All @@ -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() {
Expand Down
7 changes: 4 additions & 3 deletions libc/src/__support/File/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <stddef.h>
#include <stdint.h>
#include <unistd.h> // For off_t.

namespace LIBC_NAMESPACE {

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -179,9 +180,9 @@ class File {
return read_unlocked(data, len);
}

ErrorOr<int> seek(long offset, int whence);
ErrorOr<int> seek(off_t 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.
Expand Down
2 changes: 1 addition & 1 deletion libc/src/__support/File/linux/file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,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())
Expand Down
2 changes: 1 addition & 1 deletion libc/src/__support/File/linux/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 3 additions & 4 deletions libc/src/stdio/fopencookie.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand Down
6 changes: 5 additions & 1 deletion libc/src/stdio/generic/ftell.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,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
5 changes: 4 additions & 1 deletion libc/src/sys/mman/linux/mmap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 12 additions & 9 deletions libc/src/unistd/linux/pread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,18 @@ 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(off_t) == 8 && sizeof(size_t) == 4) {
// This is a 32-bit system with a 64-bit offset.
long offset_low = static_cast<long>(offset);
long offset_high = static_cast<long>(offset >> 32);
ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(
SYS_pread64, fd, buf, count, offset_low, offset_high);
} else {
ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pread64, fd, buf,
count, offset);
}

if (ret < 0) {
libc_errno = static_cast<int>(-ret);
return -1;
Expand Down
22 changes: 13 additions & 9 deletions libc/src/unistd/linux/pwrite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,19 @@ 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(off_t) == 8 && sizeof(size_t) == 4) {
// This is a 32-bit system with a 64-bit offset.
long offset_low = static_cast<long>(offset);
long offset_high = static_cast<long>(offset >> 32);
ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(
SYS_pwrite64, fd, buf, count, offset_low, offset_high);
} else {
ssize_t ret = LIBC_NAMESPACE::syscall_impl<ssize_t>(SYS_pwrite64, fd, buf,
count, offset);
}

if (ret < 0) {
libc_errno = static_cast<int>(-ret);
return -1;
Expand Down
8 changes: 4 additions & 4 deletions libc/test/src/__support/File/file_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 60401c1

Please sign in to comment.