[libc-commits] [libc] [libc][NFC] Adjust use of off_t internally (PR #68269)
Mikhail R. Gadelha via libc-commits
libc-commits at lists.llvm.org
Fri Oct 6 10:39:49 PDT 2023
================
@@ -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);
----------------
mikhailramalho wrote:
> Bystander comment, is this supposed to split offset 8 bytes into two 4 bytes ?
>
> If so I find this code suspicious for several reasons:
>
> * why test `sizeof(size_t) == 4` if we're actually using the `long` type in the syscall?
This checks only if we are running in a 32-bit system.
> * to get the low bits I would expect to see a mask, I see a cast and so the semantic seems unclear,
The cast will assign the lowest bits of offset to offset_low... It should work like it was before `(long)offset`. If we only use a mask and pass offset to `syscall_impl`, we'll get a warning about the loss of precision, that's why I've added the explicit cast.
> * what happens to the sign bit in case `offset` is negative?
AFAIK, a right shift of negative numbers is implementation-defined, but I assume the compiler will generate an arithmetic right shift here, so the sign bit will be preserved.
>
> I'm not saying the code is wrong, but it's not obviously correct either.
https://github.com/llvm/llvm-project/pull/68269
More information about the libc-commits
mailing list