Re: [PATCH v12 2/7] linux: Introduce INTERNAL_VSYSCALL

[Evan Green <evan@rivosinc.com>]

On Thu, Feb 15, 2024 at 11:44 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Evan Green:
>
> > Add an INTERNAL_VSYSCALL() macro that makes a vDSO call, falling back to
> > a regular syscall, but without setting errno. Instead, the return value
> > is plumbed straight out of the macro.
> >
> > Signed-off-by: Evan Green <evan@rivosinc.com>
> > ---
> >
> > (no changes since v10)
> >
> > Changes in v10:
> >  - Introduced INTERNAL_VSYSCALL patch
> >
> >  sysdeps/unix/sysv/linux/sysdep-vdso.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/sysdeps/unix/sysv/linux/sysdep-vdso.h b/sysdeps/unix/sysv/linux/sysdep-vdso.h
> > index 189319ad98..f7d4b29b25 100644
> > --- a/sysdeps/unix/sysv/linux/sysdep-vdso.h
> > +++ b/sysdeps/unix/sysv/linux/sysdep-vdso.h
> > @@ -53,4 +53,23 @@
> >      sc_ret;                                                                \
> >    })
> >
> > +#define INTERNAL_VSYSCALL(name, nr, args...)                               \
> > +  ({                                                                       \
> > +    __label__ out;                                                         \
> > +    long int sc_ret;                                                       \
> > +                                                                           \
> > +    __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name);          \
> > +    if (vdsop != NULL)                                                             \
> > +      {                                                                            \
> > +     sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args);                  \
> > +     if ((!INTERNAL_SYSCALL_ERROR_P (sc_ret)) ||                           \
> > +         (INTERNAL_SYSCALL_ERRNO (sc_ret) != ENOSYS))                      \
> > +       goto out;                                                           \
> > +      }                                                                            \
> > +                                                                           \
> > +    sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);                         \
> > +  out:                                                                             \
> > +    sc_ret;                                                                \
> > +  })
> > +
> >  #endif /* SYSDEP_VDSO_LINUX_H  */
>
> I wonder if this is clearer without the label and with a conditional
> expression instead?

It's kinda awkward without the label because the regular syscall is
made either if vdsop is NULL or returns ENOSYS (which is two checks on
the return value, so doesn't lend itself to inlining in the if
statement). The best I could come up with without the label is to
duplicate the syscall_call:

#define INTERNAL_VSYSCALL(name, nr, args...)                      \
  ({                                          \
    long int sc_ret;                                  \
                                          \
    __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name);          \
    if (vdsop != NULL)                                  \
      {                                          \
    sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args);              \
    if ((INTERNAL_SYSCALL_ERROR_P (sc_ret)) &&                  \
        (INTERNAL_SYSCALL_ERRNO (sc_ret) == ENOSYS))              \
      sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);              \
      }                                          \
    else                                      \
      {                                          \
    sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);                  \
      }                                          \
    sc_ret;                                      \
  })

I think I prefer the version with the label, but I'm not attuned to
the glibc style, so let me know if you like this and I'll send out a
new version.
-Evan