This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 13/15] linux: Consolidate INLINE_SYSCALL
On 11/02/2020 09:03, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
>> index c7f3e54d37..389c94cfda 100644
>> --- a/sysdeps/unix/sysv/linux/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/sysdep.h
>> @@ -15,8 +15,44 @@
>> License along with the GNU C Library; if not, see
>> <https://www.gnu.org/licenses/>. */
>>
>> +#ifndef _SYSDEP_LINUX_H
>> +#define _SYSDEP_LINUX_H
>> +
>> #include <bits/wordsize.h>
>> #include <kernel-features.h>
>> +#include <errno.h>
>> +
>> +#ifndef __ASSEMBLER__
>> +
>> +#undef INTERNAL_SYSCALL_DECL
>> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0)
>
> I think these preprocessor directives should be indented (including most
> of the rest of the file).
I usually avoid such changes since it generated changes unrelated to the
patch itself. The __ASSEMBLER__ is not really required, so I removed
it. Also, my idea is to refactor this code to use inline function, so
I add the indentation change once __ASSEMBLER__ closure is really
required.
>
>> +#undef INTERNAL_SYSCALL_ERROR_P
>> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
>> + ((unsigned long) (val) > -4096UL)
>> +
>> +#ifndef SYSCALL_ERROR_LABEL
>> +# define SYSCALL_ERROR_LABEL(sc_err) \
>> + ({ \
>> + __set_errno (sc_err); \
>> + -1L; \
>> + })
>> +#endif
>> +
>> +/* This version is for kernels that implement system calls that
>> + behave like function calls as far as register saving. */
>> +#undef INLINE_SYSCALL
>> +#define INLINE_SYSCALL(name, nr, args...) \
>> + ({ \
>> + INTERNAL_SYSCALL_DECL (sc_err); \
>> + long int sc_ret = INTERNAL_SYSCALL (name, sc_err, nr, args); \
>> + __glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (sc_ret, sc_err)) \
>> + ? SYSCALL_ERROR_LABEL (INTERNAL_SYSCALL_ERRNO (sc_ret, sc_err)) \
>> + : sc_ret; \
>> + })
>
> The comment seems misleading to me. Does “register saving” really
> matter here? I think it's about the -errno behavior. I think the
> comment should explain how this macro is to be used (i.e., it sets errno
> on failure).
Indeed, it is an artefact from powerpc version. What about:
/* Define a macro which expands into the inline wrapper code for a system
call. It sets the errno and returns -1 on a failure, or the syscall
return value otherwise. */
>
> Thanks,
> Florian
>