Message ID | 1432664355-10269-1-git-send-email-cmetcalf@ezchip.com |
---|---|
State | New |
Headers | show |
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.
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. */