This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] tile: use better variable naming in INLINE_SYSCALL
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Chris Metcalf <cmetcalf at ezchip dot com>, Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Tue, 26 May 2015 16:44:25 -0400
- Subject: Re: [PATCH] tile: use better variable naming in INLINE_SYSCALL
- Authentication-results: sourceware.org; auth=none
- References: <1432664355-10269-1-git-send-email-cmetcalf at ezchip dot com>
On 05/26/2015 02:19 PM, Chris Metcalf wrote:
> At issue for INLINE_SYSCALL was that it used "err" and "val"
> as variable names in a #define, so that if it was used in a context
> where the "caller" was also using "err" or "val", and those
> variables were passed in to INLINE_SYSCALL, we would end up
> referencing the internal shadowed variables instead.
>
> For example, "char val" in check_may_shrink_heap() in
> sysdeps/unix/sysv/linux/malloc-sysdep.h was being shadowed by
> the syscall return "val" in INLINE_SYSCALL, causing the "char val"
> not to get updated at all, and may_shrink_heap ended up always false.
>
> A similar fix was made to INTERNAL_VSYSCALL_CALL.
Established practice appears to be to use `sc_err`.
A quick look shows that other sysdep.h also suffer this problem,
but have been lucky that nobody uses `sc_ret` or similarly named
variables in the outer scope.
Doesn't this also impact MIPS, Microblaze, SPARC and NIOS2?
sysdeps/unix/sysv/linux/mips/mips64/n32/sysdep.h: ({ INTERNAL_SYSCALL_DECL(err); \
sysdeps/unix/sysv/linux/mips/mips64/n32/sysdep.h:#undef INTERNAL_SYSCALL_DECL
sysdeps/unix/sysv/linux/mips/mips64/n64/sysdep.h: ({ INTERNAL_SYSCALL_DECL(err); \
sysdeps/unix/sysv/linux/mips/mips64/n64/sysdep.h:#undef INTERNAL_SYSCALL_DECL
sysdeps/unix/sysv/linux/mips/mips32/sysdep.h: ({ INTERNAL_SYSCALL_DECL(err); \
sysdeps/unix/sysv/linux/mips/mips32/sysdep.h:#undef INTERNAL_SYSCALL_DECL
sysdeps/unix/sysv/linux/microblaze/sysdep.h:({ INTERNAL_SYSCALL_DECL(err); \
sysdeps/unix/sysv/linux/microblaze/sysdep.h:# undef INTERNAL_SYSCALL_DECL
sysdeps/unix/sysv/linux/sparc/sysdep.h:({ INTERNAL_SYSCALL_DECL(err); \
sysdeps/unix/sysv/linux/sparc/sysdep.h:#undef INTERNAL_SYSCALL_DECL
sysdeps/unix/sysv/linux/nios2/sysdep.h: ({ INTERNAL_SYSCALL_DECL(err); \
sysdeps/unix/sysv/linux/nios2/sysdep.h:#undef INTERNAL_SYSCALL_DECL
I dislike the use of `sc_err`, and I'd rather blanket fix
*every* port to use `_sc_err` instead, but that's just me.
Fixing tile is more than good enough.
> ---
> I will commit this shortly unless someone raises a concern.
>
> ChangeLog | 6 ++++++
> sysdeps/unix/sysv/linux/tile/sysdep.h | 29 +++++++++++++++--------------
> 2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index f06cb9487d8a..2b11c7fc64e7 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2015-05-26 Chris Metcalf <cmetcalf@ezchip.com>
> +
> + * sysdeps/unix/sysv/linux/tile/sysdep.h (INLINE_SYSCALL):
> + Avoid using variables in #defines that might cause shadowing.
> + (INTERNAL_VSYSCALL_CALL): Likewise.
> +
> 2015-05-26 Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> * sysdeps/unix/sysv/linux/aarch64/gettimeofday.c (HAVE_VSYSCALL):
> diff --git a/sysdeps/unix/sysv/linux/tile/sysdep.h b/sysdeps/unix/sysv/linux/tile/sysdep.h
> index 30d52e32c9a7..6568dc803485 100644
> --- a/sysdeps/unix/sysv/linux/tile/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/tile/sysdep.h
> @@ -78,16 +78,17 @@
> /* Define a macro which expands inline into the wrapper code for a system
> call. */
> # undef INLINE_SYSCALL
> -# define INLINE_SYSCALL(name, nr, args...) \
> +# define INLINE_SYSCALL(name, nr, args...) \
> ({ \
> - INTERNAL_SYSCALL_DECL (err); \
> - unsigned long val = INTERNAL_SYSCALL (name, err, nr, args); \
> - if (__builtin_expect (INTERNAL_SYSCALL_ERROR_P (val, err), 0)) \
> - { \
> - __set_errno (INTERNAL_SYSCALL_ERRNO (val, err)); \
> - val = -1; \
> - } \
> - (long) val; })
> + INTERNAL_SYSCALL_DECL (_sys_err); \
> + unsigned long _sys_val = INTERNAL_SYSCALL (name, _sys_err, nr, args); \
> + if (__builtin_expect (INTERNAL_SYSCALL_ERROR_P (_sys_val, _sys_err), 0)) \
> + { \
> + __set_errno (INTERNAL_SYSCALL_ERRNO (_sys_val, _sys_err)); \
> + _sys_val = -1; \
> + } \
> + (long) _sys_val; \
> + })
>
> #undef INTERNAL_SYSCALL
> #define INTERNAL_SYSCALL(name, err, nr, args...) \
> @@ -203,11 +204,11 @@
> "=R05" (_clobber_r5), "=R10" (_clobber_r10)
>
>
> -#define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...) \
> - ({ \
> - struct syscall_return_value rv = funcptr (args); \
> - err = rv.error; \
> - rv.value; \
> +#define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...) \
> + ({ \
> + struct syscall_return_value _sys_rv = funcptr (args); \
> + err = _sys_rv.error; \
> + _sys_rv.value; \
> })
>
> /* List of system calls which are supported as vsyscalls. */
>