Both system calls are exactly the same, the only difference is the
(optional) timespec conversion. Consolidate the two, and check which
syscall being emulated in the timespec conversion place.

This is just a PoC. But this brings at least 2 questions.

Let's take pselect6 as an example. There are 2 possible pselects
in there (actually more, but let's focus on just the two):
TARGET_NR_pselect6 and TARGET_NR_pselect6_time64.  Both are implemented
in a single do_pselect6() function, which, depending on its "time64"
argument, will call either target_to_host_timespec64() or
target_to_host_timespec().

But these functions, in turn, are guarded by a lot of #if
defined(foo). In particular, in context of pselect6,
target_to_host_timespec() is guarded by
 if defined(TARGET_NR_pselect6),
while target_to_host_timespec64() is guarded by
 if defined(TARGET_NR_pselect6_time64).

So basically, if just one of the two TARGET_NR_pselect6*
is defined, one of target_to_host_timespec*() will not
be defined, but do_pselect6() calls *both* anyway.  In
other words, both functions must be provided if any of
the select6 are to be implemented.

But the good news is that these functions
(target_to_host_timespec*()) are always defined because
they're needed for some syscalls anyway, like, eg,
TARGET_NR_semop or TARGET_NR_utimensat or CONFIG_TIMERFD.

It looks like whole gigantic ifdeffery around these two
functions should be dropped, and a common function provided,
with an extra argument like time64, to be used in many
places where this construct is already used. Like in
this PoC too, which again calls one of the two depending
on the actual syscall being used.  Or maybe we can even
combine the two into one with an extra arg like "time64".

This should simplify quite some code greatly.

What do you think?

And another question: this PoC should work in either of
4 cases, including when just one (any of) rt_sigtimedwait
and rt_sigtimedwait_time64 is defined. It uses small
preprocessor trick, which - to my taste anyway - isn't
exactly great. Maybe there's some other, more elegant,
way to do that exists?

Thanks,

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 46 ++++++++------------------------------------
 1 file changed, 8 insertions(+), 38 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 24b25759be..c175e03207 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9700,48 +9700,16 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             finish_sigsuspend_mask(ret);
         }
         return ret;
+#if defined(TARGET_NR_rt_sigtimedwait) || defined(TARGET_NR_rt_sigtimedwait_time64)
 #ifdef TARGET_NR_rt_sigtimedwait
     case TARGET_NR_rt_sigtimedwait:
-        {
-            sigset_t set;
-            struct timespec uts, *puts;
-            siginfo_t uinfo;
-
-            if (arg4 != sizeof(target_sigset_t)) {
-                return -TARGET_EINVAL;
-            }
-
-            if (!(p = lock_user(VERIFY_READ, arg1, sizeof(target_sigset_t), 1)))
-                return -TARGET_EFAULT;
-            target_to_host_sigset(&set, p);
-            unlock_user(p, arg1, 0);
-            if (arg3) {
-                puts = &uts;
-                if (target_to_host_timespec(puts, arg3)) {
-                    return -TARGET_EFAULT;
-                }
-            } else {
-                puts = NULL;
-            }
-            ret = get_errno(safe_rt_sigtimedwait(&set, &uinfo, puts,
-                                                 SIGSET_T_SIZE));
-            if (!is_error(ret)) {
-                if (arg2) {
-                    p = lock_user(VERIFY_WRITE, arg2, sizeof(target_siginfo_t),
-                                  0);
-                    if (!p) {
-                        return -TARGET_EFAULT;
-                    }
-                    host_to_target_siginfo(p, &uinfo);
-                    unlock_user(p, arg2, sizeof(target_siginfo_t));
-                }
-                ret = host_to_target_signal(ret);
-            }
-        }
-        return ret;
 #endif
 #ifdef TARGET_NR_rt_sigtimedwait_time64
     case TARGET_NR_rt_sigtimedwait_time64:
+#define rt_sigtimedwait_istime64() (num == TARGET_NR_rt_sigtimedwait_time64)
+#else
+#define rt_sigtimedwait_istime64() 0
+#endif
         {
             sigset_t set;
             struct timespec uts, *puts;
@@ -9759,7 +9727,9 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             unlock_user(p, arg1, 0);
             if (arg3) {
                 puts = &uts;
-                if (target_to_host_timespec64(puts, arg3)) {
+                if (rt_sigtimedwait_istime64()
+                    ? target_to_host_timespec64(puts, arg3)
+                    : target_to_host_timespec(puts, arg3)) {
                     return -TARGET_EFAULT;
                 }
             } else {
-- 
2.30.2

On Fri, Dec 16, 2022, at 19:08, Michael Tokarev wrote:
> Both system calls are exactly the same, the only difference is the
> (optional) timespec conversion. Consolidate the two, and check which
> syscall being emulated in the timespec conversion place.
>
> This is just a PoC. But this brings at least 2 questions.
>
> Let's take pselect6 as an example. There are 2 possible pselects
> in there (actually more, but let's focus on just the two):
> TARGET_NR_pselect6 and TARGET_NR_pselect6_time64.  Both are implemented
> in a single do_pselect6() function, which, depending on its "time64"
> argument, will call either target_to_host_timespec64() or
> target_to_host_timespec().
>
> But these functions, in turn, are guarded by a lot of #if
> defined(foo). In particular, in context of pselect6,
> target_to_host_timespec() is guarded by
>  if defined(TARGET_NR_pselect6),
> while target_to_host_timespec64() is guarded by
>  if defined(TARGET_NR_pselect6_time64).
>
> So basically, if just one of the two TARGET_NR_pselect6*
> is defined, one of target_to_host_timespec*() will not
> be defined, but do_pselect6() calls *both* anyway.  In
> other words, both functions must be provided if any of
> the select6 are to be implemented.
>
> But the good news is that these functions
> (target_to_host_timespec*()) are always defined because
> they're needed for some syscalls anyway, like, eg,
> TARGET_NR_semop or TARGET_NR_utimensat or CONFIG_TIMERFD.
>
> It looks like whole gigantic ifdeffery around these two
> functions should be dropped, and a common function provided,
> with an extra argument like time64, to be used in many
> places where this construct is already used. Like in
> this PoC too, which again calls one of the two depending
> on the actual syscall being used.  Or maybe we can even
> combine the two into one with an extra arg like "time64".

The cleanup you suggest here looks correct to me, but as I
mentioned on IRC, I think this should only be done if the
time64 host side bug is also fixed. At the moment, both
the time32 and time64 target syscalls are always translated
into the time32 variant on 32-bit hosts, which is a mistake
for multiple reasons:

- the libc-defined 'timespec' argument that is passed into
  the kernel-defined syscall has the wrong layout, resulting
  in incorrect data. This could be avoided by using
  the kernel-defined __kernel_old_timespec instead, but then

- time64 target arguments needlessly get truncated to
  the time32 range. This happens e.g. when running 64-bit
  target on 32-bit host, but could be easily avoided
  by converting into the time64 types (__kernel_timespec)
  and calling the time64 syscalls whenever those are
  available.

- On modern hosts, only the time64 syscalls are available, so
  e.g. on riscv32 hosts, the current implementation does not
  even build.

The best solution for all of the above I think would be to
have a wrapper for each of the affected syscalls that calls
the time64 interface first and falls back to the time32
version only if that fails, using __kernel_timespec
on the interface instead of libc timespec.

A simpler but less flexible method would be to detect
sizeof(time_t) at compile time and then use either the
time32 or time64 host syscall numbers, whichever goes
with the libc time_t/timespec.


      Arnd