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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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;
----------------
Can you point to some documentation about `llseek` on rv32?


================
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
----------------
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
```


================
Comment at: libc/src/sched/linux/sched_rr_get_interval.cpp:31
+      __llvm_libc::syscall_impl(SYS_sched_rr_get_interval_time64, tid, &ts32);
+  if (!ret) {
+    tp->tv_sec = ts32.tv_sec;
----------------
`ret > 0` ?


================
Comment at: libc/src/sys/sendfile/linux/sendfile.cpp:26
+#elif defined(SYS_sendfile64)
+  // The only difference here is that offset is expected to be 64 bits long
+  long ret =
----------------
Do you mean that the `offset` should be a pointer to a 64-bit value? If yes, can you add a `static_assert` here?

```
static_assert(sizeof(off_t) == 64);
```


================
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);
----------------
Which one is required for rv32, `SYS_waitpid` or `SYS_waitid`?


================
Comment at: libc/src/sys/wait/linux/wait4.cpp:31
+    // We ignore the return here, since getrusage doesn't set errno in wait4
+    getrusage(info.si_pid, usage);
+#elif defined(SYS_waitid)
----------------
Where is this `getrusage` coming from?


================
Comment at: libc/src/sys/wait/linux/wait4.cpp:38
+  // waitid, we need to use:
+  // * P_PID option o wait for a specific child process with the process ID
+  // specified in the pid parameter
----------------
`to`


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


================
Comment at: libc/src/sys/wait/linux/waitpid.cpp:22
 LLVM_LIBC_FUNCTION(pid_t, waitpid, (pid_t pid, int *wait_status, int options)) {
+#if SYS_waitpid
+  pid = __llvm_libc::syscall_impl(SYS_waitpid, pid, wait_status, options);
----------------
Same question: can we limit to the exact `SYS_wait*` we want for rv32?


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


================
Comment at: libc/src/unistd/linux/dup2.cpp:33
+#elif defined(SYS_fcntl64)
+    long ret = __llvm_libc::syscall_impl(SYS_fcntl64, oldfd, F_GETFD);
+#else
----------------
Can you add comments about how this 64 suffixed syscall number differs from the non-suffixed syscall?


================
Comment at: libc/src/unistd/linux/ftruncate.cpp:24
+#elif defined(SYS_ftruncate64)
+  int ret = __llvm_libc::syscall_impl(SYS_ftruncate64, fd, len);
+#else
----------------
Ditto.


================
Comment at: libc/src/unistd/linux/truncate.cpp:24
+#elif defined(SYS_truncate64)
+  int ret = __llvm_libc::syscall_impl(SYS_truncate64, path, len);
+#else
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148371