This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add support to compile some syscalls on 32 bit platform
ClosedPublic

Authored by mikhail.ramalho on Apr 14 2023, 1:21 PM.

Details

Summary

This patch adds a bunch of ifdefs to handle the 32 bit versions of
some syscalls, which often only append a 64 to the name of the syscall
(with exception of SYS_lseek -> SYS_llseek and SYS_futex ->
SYS_futex_time64)

This patch also tries to handle cases where wait4 is not available
(as in riscv32): to implement wait, wait4 and waitpid when wait4 is
not available, we check for alternative wait calls and ultimately rely
on waitid to implement them all.

In riscv32, only waitid is available, so we need it to support this
platform.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 14 2023, 1:21 PM
mikhail.ramalho requested review of this revision.Apr 14 2023, 1:21 PM
michaelrj added inline comments.Apr 14 2023, 2:43 PM
libc/src/sys/wait/linux/wait.cpp
23

it might be set by the extra argument for this syscall, the man page mentions that the syscall is slightly different from the function. I've pasted the text below since I can't find it online:

The raw waitid() system call takes a fifth argument, of type struct rusage *.  If this argument  is  non-NULL,
then  it  is  used  to return resource usage information about the child, in the same manner as wait4(2).  See
getrusage(2) for details.

Even if it ends up not being relevant to this specific implementation, it'd probably still be good to put a NULL on the end of the syscall.

libc/src/sys/wait/linux/wait4.cpp
42

this list should include waitid

mikhail.ramalho marked 2 inline comments as done.Apr 14 2023, 4:31 PM
mikhail.ramalho added inline comments.
libc/src/sys/wait/linux/wait.cpp
23

Thanks! This actually simplifies the code!

mikhail.ramalho marked an inline comment as done.

Update waitid syscall

Added SYS_futex_time64 to syscall_numbers.h.inc

Added SYS_sched_rr_get_interval_time64 support to fix sched compilation in riscv32

we should probably figure out what the appropriate waitpid errno values are before landing this. It's possible that the syscall returns them as negative numbers, but I haven't tested it yet.

libc/src/sched/linux/sched_rr_get_interval.cpp
32

should this return ret be here or should it instead fall down to the handling below?

also should ret be a long to match the other syscall above?

  • Mostly LGTM but one suggestion: at all places where you added alternate paths to *64 syscalls, can you add commentary on when they will kick in and why they are to used?
  • I will make another detailed pass over the wait* wrappers.
libc/src/__support/threads/linux/mutex.h
82

Can we add a constant, say FUTEX_SYSCALL_ID in https://github.com/llvm/llvm-project/blob/main/libc/src/__support/threads/linux/futex_word.h and avoid these widespread ifdefs?

mikhail.ramalho marked 2 inline comments as done.
  • added siginfo_t dependency to wait.h
  • added FUTEX_SYSCALL_ID
  • changed SYS_sched_rr_get_interval_time64 to use struct __kernel_timespec instead of a long arr[2]
  • improve wait* documentation

Apply clang-format to syscall_numbers.h.inc

sivachandra added inline comments.May 22 2023, 11:15 PM
libc/src/__support/File/linux_file.cpp
72 ↗(On Diff #517369)

Can you point to some documentation about llseek on rv32?

libc/src/__support/threads/linux/futex_word.h
23

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
libc/src/sched/linux/sched_rr_get_interval.cpp
31

ret > 0 ?

libc/src/sys/sendfile/linux/sendfile.cpp
26

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);
libc/src/sys/wait/linux/wait4.cpp
20

Which one is required for rv32, SYS_waitpid or SYS_waitid?

25

Where is this getrusage coming from?

32

to

38

Why do we need this assert?

libc/src/sys/wait/linux/waitpid.cpp
18

Same question: can we limit to the exact SYS_wait* we want for rv32?

29

Why do we need this assert?

libc/src/unistd/linux/dup2.cpp
33

Can you add comments about how this 64 suffixed syscall number differs from the non-suffixed syscall?

libc/src/unistd/linux/ftruncate.cpp
25

Ditto.

libc/src/unistd/linux/truncate.cpp
25

Ditto.

mikhail.ramalho marked 13 inline comments as done.
  • Added comments about sys_*64 syscalls
  • Added assertion about offset size in sys_*64 syscalls
  • Fixed wait4 implementation
  • Rewrote wait and waitpid to use wait4
libc/src/__support/File/linux_file.cpp
72 ↗(On Diff #517369)

You can find the llseek documentation here: https://man7.org/linux/man-pages/man3/lseek64.3.html

libc/src/__support/threads/linux/futex_word.h
23

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.

libc/src/sys/wait/linux/wait4.cpp
20

SYS_waitid

38

Fixed.

michaelrj added inline comments.Jun 5 2023, 12:46 PM
libc/src/sys/wait/linux/wait.cpp
23

we generally don't allow calling public libc functions from other libc functions, so wait4 should be moved to a shared internal function. This allows platforms to include functions without having to deal with dependency chains. You can see how this is generally done with the string to integer functions that all include strtointeger, though in this case it probably doesn't need to go in __support.

libc/src/sys/wait/linux/wait4.cpp
20–22

We generally try to avoid setting errno in internal functions, so if you move wait4 to be an internal function you'll probably have to change its return type to ErrorOr<pid_t> and set errno after the call. This will be similar to how fseek works..

This is good to go after addressing @michaelrj's comments.

libc/src/sys/wait/linux/wait4.cpp
37

I think you should #include <signal.h> for CLD_* macros?

  • Address comment about wait4 implementation
  • Fix sched_getscheduler return value
  • Fix sprintf test case for 32-bit systems
mikhail.ramalho marked an inline comment as done.Jul 25 2023, 9:22 AM

This PR is not done yet as I'm getting a weird error on the chmod test on rv32:

FAILED: projects/libc/test/src/sys/stat/CMakeFiles/libc.test.src.sys.stat.chmod_test /home/root/llvm-project/build/projects/libc/test/src/sys/stat/CMakeFiles/libc.test.src.sys.stat.chmod_test 
cd /home/root/llvm-project/build/projects/libc/test/src/sys/stat && /home/root/llvm-project/build/projects/libc/test/src/sys/stat/libc.test.src.sys.stat.chmod_test.__build__
[ RUN      ] LlvmLibcChmodTest.ChangeAndOpen
/home/root/llvm-project/libc/test/src/sys/stat/chmod_test.cpp:46: FAILURE
      Expected: __llvm_libc::open(TEST_FILE, 02000 | 01)
      Which is: 3
To be equal to: -1
      Which is: -1
/home/root/llvm-project/libc/test/src/sys/stat/chmod_test.cpp:47: FAILURE
          Expected: __llvm_libc::__llvmlibc_internal_errno
          Which is: 0
To be not equal to: 0
          Which is: 0

Apparently, I'm able to set the file as read-only but can also open it in write mode.

  • Fixed ftruncate, truncate, pwrite and pread syscalls
  • Rebased

Removed wrong ifdef

mikhail.ramalho marked an inline comment as done.

Changed wait4impl to return an ErrorOr object

mikhail.ramalho marked an inline comment as done.Aug 2 2023, 9:54 AM

folks, can I get another round of reviews on this patch? I've addressed all the comments.

michaelrj accepted this revision.Aug 2 2023, 11:02 AM

LGTM with a minor nit

libc/test/src/sched/sched_rr_get_interval_test.cpp
32–33

nit: Generally I'd recommend using either long long or uint64_t for a larger type. long is only required to be at least 32 bits, and some systems (notably windows) use that size.

This revision is now accepted and ready to land.Aug 2 2023, 11:02 AM

Hello Mikhail, it looks like https://reviews.llvm.org/D148371 broke the x86 linux build bots.
https://lab.llvm.org/buildbot/#/grid?tag=libc
https://lab.llvm.org/buildbot/#/builders/78/builds/55807/steps/4/logs/stdio

/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian/llvm-project/libc/src/sched/linux/sched_rr_get_interval.cpp:15:10: fatal error: 'linux/time_types.h' file not found
#include <linux/time_types.h> // For __kernel_timespec.

Can you revert and reland when it's fixed? Thx!