[libc-commits] [PATCH] D148371: [libc] Add support to compile some syscalls on 32 bit platform

[Mikhail Ramalho via Phabricator via libc-commits]

mikhail.ramalho added inline comments.


================
Comment at: libc/src/__support/File/linux_file.cpp:72
+#elif defined(SYS_llseek)
+  int ret = __llvm_libc::syscall_impl(SYS_llseek, lf->get_fd(), offset, whence);
+  result = ret;
----------------
sivachandra wrote:
> Can you point to some documentation about `llseek` on rv32?
You can find the llseek documentation here: https://man7.org/linux/man-pages/man3/lseek64.3.html


================
Comment at: libc/src/__support/threads/linux/futex_word.h:23
+#elif defined(SYS_futex_time64)
+constexpr auto FUTEX_SYSCALL_ID = SYS_futex_time64;
+#else
----------------
sivachandra wrote:
> Quick googling tells me that `SYS_futex_time64` is rv32 only construct. Can you add comment here saying that?
> 
> Also, WDYT doing it this way?
> 
> ```
> #if <rv32>
> #ifndef SYS_futex_time64
> #error "..."
> #endif
> constexpr auto FUTEX_SYSCALL_ID = SYS_futex_time64;
> #else
> constexpr auto FUTEX_SYSCALL_ID = SYS_futex;
> #endif
> ```
AFAIK, `SYS_futex_time64` is defined in every 32-bit arch that uses 64-bit time, e.g., in my armv7 machine:
```
$ clang -dM -E main1.c | grep futex
#define SYS_futex __NR_futex
#define SYS_futex_time64 __NR_futex_time64
#define __NR_futex (__NR_SYSCALL_BASE + 240)
#define __NR_futex_time64 (__NR_SYSCALL_BASE + 422)
```
The case for rv32 is that is only has `SYS_futex_time64`.


================
Comment at: libc/src/sys/wait/linux/wait4.cpp:26
   pid = __llvm_libc::syscall_impl(SYS_wait4, pid, wait_status, options, usage);
+#elif defined(SYS_waitpid)
+  pid = __llvm_libc::syscall_impl(SYS_waitpid, pid, wait_status, options);
----------------
sivachandra wrote:
> Which one is required for rv32, `SYS_waitpid` or `SYS_waitid`?
`SYS_waitid`


================
Comment at: libc/src/sys/wait/linux/wait4.cpp:44
+  if (pid != -1) {
+    LIBC_ASSERT(wait_status != nullptr);
+    *wait_status = info.si_status;
----------------
sivachandra wrote:
> Why do we need this assert?
Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148371/new/

https://reviews.llvm.org/D148371