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

[Gabriel Ravier <gabravier@gmail.com>]

On 2/23/24 23:12, Evan Green wrote:
> 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

Is GCC able to de-duplicate the duplicated syscall call ? The label 
version already seems readable enough in any case and if GCC is not able 
to de-duplicate the syscall it seems preferable from a performance/code 
size standpoint as well.