tile: use better variable naming in INLINE_SYSCALL
Commit Message
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.
---
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(-)
Comments
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. */
>
On 5/26/2015 4:44 PM, Carlos O'Donell wrote:
> 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.
Thanks for looking at this, and you're right that probably fixing everything is
a better approach, but one that I was too lazy to undertake this minute. :-)
I updated my change to use _sc_err instead of _sys_err (likewise for _sc_val) and
committed.
This is the convention already used in sysdeps/unix/alpha/sysdep.h, by the way.
On Tue, 26 May 2015, Carlos O'Donell wrote:
> 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?
Have you filed a bug for this (preferably one per architecture,
architecture maintainers CC:ed), or do you intend to fix it yourself (per
standard glibc practice, if you find a bug you aren't fixing, you should
file it in Bugzilla)?
@@ -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):
@@ -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. */