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

[Evan Green <evan@rivosinc.com>]

On Sat, Feb 24, 2024 at 3:40 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Evan Green:
>
> > 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;                                      \
> >   })
>
> Indentation is off for me, for clarification:

Yeah sorry I just pasted it into the gmail client, which mangles everything.

>
> #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;                                                             \
>   })
>
> ?: does not work because the comparison with -ENOSYS discards the
> return value.
>
> Does this look correct?
>
> #define INTERNAL_VSYSCALL(name, nr, args...)                            \
>   ({                                                                    \
>     long int sc_ret = -ENOSYS;                                          \
>     __typeof (GLRO(dl_vdso_##name)) vdsop = GLRO(dl_vdso_##name);       \
>     if (vdsop != NULL)                                                  \
>       sc_ret = INTERNAL_VSYSCALL_CALL (vdsop, nr, ##args);              \
>     if (sc_ret == -ENOSYS)                                              \
>       sc_ret = INTERNAL_SYSCALL_CALL (name, ##args);                    \
>     sc_ret;                                                             \
>   })
>
> I expect GCC to generate decent code for it.

I like it! I'm guessing I should continue to use the
INTERNAL_SYSCALL_ERRNO_P() and INTERNAL_SYSCALL_ERRNO(), and you
omitted it for example's sake. Will plan to spin with this.
-Evan