[PATCH] Use inline syscalls for non-cancellable versions

[PATCH] Use inline syscalls for non-cancellable versions

[Adhemerval Zanella]

Hi

This patch is one of the required adjustments for the fix for bz12683
(Race conditions in pthread cancellation) and the idea is to not rely
on the non-cancelable entry points for cancelable syscalls (since the
upcoming fill will remove them).
 
This patch uses inline calls (through INLINE_SYSCALL macro) to define
the non-cancellable functions macros to avoid use of the
syscall_nocancel entrypoint.

Tested on i386, x86_64, x32, powerpc32, powerpc64, arm, and aarch64.

---

	* sysdeps/unix/sysv/linux/not-cancel.h (open_not_cancel): Rewrite to
	be an inline implementation regardless of library is built within.
	(open_not_cancel_2): Likewise.
	(__read_nocancel): Likewise.
	(__write_nocancel): Likewise.
	(openat_not_cancel): Likewise.
	(openat_not_cancel_3): Likewise.
	(openat64_not_cancel): Likewise.
	(openat64_not_cancel_3): Likewise.
	(__close_nocancel): Likewise.
	(pause_not_cancel): Likewise.
	(nanosleep_not_cancel): Likewise.
	(sigsuspend_not_cancel): Likewise.

--

diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
index 62d487f..8a358fd 100644
--- a/sysdeps/unix/sysv/linux/not-cancel.h
+++ b/sysdeps/unix/sysv/linux/not-cancel.h
@@ -17,48 +17,48 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#ifndef NOT_CANCEL_H
+# define NOT_CANCEL_H
+
 #include <sysdep.h>
+#include <errno.h>
+#include <unistd.h>
+#include <sys/syscall.h>
 
-#if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
-extern int __open_nocancel (const char *, int, ...) attribute_hidden;
-extern int __close_nocancel (int) attribute_hidden;
-extern int __read_nocancel (int, void *, size_t) attribute_hidden;
-extern int __write_nocancel (int, const void *, size_t) attribute_hidden;
-extern pid_t __waitpid_nocancel (pid_t, int *, int) attribute_hidden;
-extern int __openat_nocancel (int fd, const char *fname, int oflag,
-				mode_t mode) attribute_hidden;
-extern int __openat64_nocancel (int fd, const char *fname, int oflag,
-				  mode_t mode) attribute_hidden;
+/* Uncancelable open.  */
+#ifdef __NR_open
+# define open_not_cancel(name, flags, mode) \
+   INLINE_SYSCALL (open, 3, name, flags, mode)
+# define open_not_cancel_2(name, flags) \
+   INLINE_SYSCALL (open, 2, name, flags)
 #else
-# define __open_nocancel(name, ...) __open (name, __VA_ARGS__)
-# define __close_nocancel(fd) __close (fd)
-# define __read_nocancel(fd, buf, len) __read (fd, buf, len)
-# define __write_nocancel(fd, buf, len) __write (fd, buf, len)
-# define __waitpid_nocancel(pid, stat_loc, options) \
-  __waitpid (pid, stat_loc, options)
-# define __openat_nocancel(fd, fname, oflag, mode) \
-  openat (fd, fname, oflag, mode)
-# define __openat64_nocancel(fd, fname, oflag, mode) \
-  openat64 (fd, fname, oflag, mode)
+# define open_not_cancel(name, flags, mode) \
+   INLINE_SYSCALL (openat, 4, AT_FDCWD, name, flags, mode)
+# define open_not_cancel_2(name, flags) \
+   INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags)
 #endif
 
-/* Uncancelable open.  */
-#define open_not_cancel(name, flags, mode) \
-   __open_nocancel (name, flags, mode)
-#define open_not_cancel_2(name, flags) \
-   __open_nocancel (name, flags)
+/* Uncancelable read.  */
+#define __read_nocancel(fd, buf, len) \
+  INLINE_SYSCALL (read, 3, fd, buf, len)
+
+/* Uncancelable write.  */
+#define __write_nocancel(fd, buf, len) \
+  INLINE_SYSCALL (write, 3, fd, buf, len)
 
 /* Uncancelable openat.  */
 #define openat_not_cancel(fd, fname, oflag, mode) \
-  __openat_nocancel (fd, fname, oflag, mode)
+  INLINE_SYSCALL (openat, 4, fd, fname, oflag, mode)
 #define openat_not_cancel_3(fd, fname, oflag) \
-  __openat_nocancel (fd, fname, oflag, 0)
+  INLINE_SYSCALL (openat, 3, fd, fname, oflag)
 #define openat64_not_cancel(fd, fname, oflag, mode) \
-  __openat64_nocancel (fd, fname, oflag, mode)
+  INLINE_SYSCALL (openat, 4, fd, fname, oflag | O_LARGEFILE, mode)
 #define openat64_not_cancel_3(fd, fname, oflag) \
-  __openat64_nocancel (fd, fname, oflag, 0)
+  INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE)
 
 /* Uncancelable close.  */
+#define __close_nocancel(fd) \
+  INLINE_SYSCALL (close, 1, fd)
 #define close_not_cancel(fd) \
   __close_nocancel (fd)
 #define close_not_cancel_no_status(fd) \
@@ -83,17 +83,27 @@ extern int __openat64_nocancel (int fd, const char *fname, int oflag,
   __fcntl_nocancel (fd, cmd, val)
 
 /* Uncancelable waitpid.  */
-#define waitpid_not_cancel(pid, stat_loc, options) \
+#define __waitpid_nocancel(pid, stat_loc, options) \
   INLINE_SYSCALL (wait4, 4, pid, stat_loc, options, NULL)
+#define waitpid_not_cancel(pid, stat_loc, options) \
+  __waitpid_nocancel(pid, stat_loc, options)
 
 /* Uncancelable pause.  */
 #define pause_not_cancel() \
-  __pause_nocancel ()
+  ({ sigset_t set; 							     \
+     int __rc = INLINE_SYSCALL (rt_sigprocmask, 4, SIG_BLOCK, NULL, &set,    \
+				_NSIG / 8);				     \
+     if (__rc == 0)							     \
+       __rc = INLINE_SYSCALL (rt_sigsuspend, 2, &set, _NSIG / 8);	     \
+     __rc;								     \
+  })
 
 /* Uncancelable nanosleep.  */
 #define nanosleep_not_cancel(requested_time, remaining) \
-  __nanosleep_nocancel (requested_time, remaining)
+  INLINE_SYSCALL (nanosleep, 2, requested_time, remaining)
 
 /* Uncancelable sigsuspend.  */
 #define sigsuspend_not_cancel(set) \
-  __sigsuspend_nocancel (set)
+  INLINE_SYSCALL (rt_sigsuspend, 2, set, _NSIG / 8)
+
+#endif /* NOT_CANCEL_H  */
diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
index 5b6bb51..3578ea8 100644
--- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
+++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
@@ -41,7 +41,7 @@ __get_clockfreq (void)
      contains at least one line like:
      timebase        : 33333333
      We search for this line and convert the number into an integer.  */
-  int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
+  int fd = open_not_cancel_2 ("/proc/cpuinfo", O_RDONLY);
   if (__glibc_likely (fd != -1))
     return result;

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Adhemerval Zanella]

On issue I found for this patchset is it fails for tile (and it is the
only architecture I found that this warning shows) with:

--

malloc.c: In function ‘_int_free’:
../sysdeps/unix/sysv/linux/malloc-sysdep.h:51:35: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
    may_shrink_heap = n > 0 && val == '2';
                                   ^
../sysdeps/unix/sysv/linux/malloc-sysdep.h:49:9: note: ‘val’ was declared here
    char val;

--

It is due the fact 'read_not_cancel' now calls the INLINE_SYSCALL for the
architecture and so 'tile' version is triggering this issue for some reason
(I did not check exactly why, any tile expert could help me out here?).

The straightforward fix would just initialize this variable, but this is
just clobbering tile potential issue.  Any recommendations? 

On 20-05-2015 11:19, Adhemerval Zanella wrote:
> Hi
> 
> This patch is one of the required adjustments for the fix for bz12683
> (Race conditions in pthread cancellation) and the idea is to not rely
> on the non-cancelable entry points for cancelable syscalls (since the
> upcoming fill will remove them).
>  
> This patch uses inline calls (through INLINE_SYSCALL macro) to define
> the non-cancellable functions macros to avoid use of the
> syscall_nocancel entrypoint.
> 
> Tested on i386, x86_64, x32, powerpc32, powerpc64, arm, and aarch64.
> 
> ---
> 
> 	* sysdeps/unix/sysv/linux/not-cancel.h (open_not_cancel): Rewrite to
> 	be an inline implementation regardless of library is built within.
> 	(open_not_cancel_2): Likewise.
> 	(__read_nocancel): Likewise.
> 	(__write_nocancel): Likewise.
> 	(openat_not_cancel): Likewise.
> 	(openat_not_cancel_3): Likewise.
> 	(openat64_not_cancel): Likewise.
> 	(openat64_not_cancel_3): Likewise.
> 	(__close_nocancel): Likewise.
> 	(pause_not_cancel): Likewise.
> 	(nanosleep_not_cancel): Likewise.
> 	(sigsuspend_not_cancel): Likewise.
> 
> --
> 
> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
> index 62d487f..8a358fd 100644
> --- a/sysdeps/unix/sysv/linux/not-cancel.h
> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
> @@ -17,48 +17,48 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#ifndef NOT_CANCEL_H
> +# define NOT_CANCEL_H
> +
>  #include <sysdep.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
>  
> -#if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
> -extern int __open_nocancel (const char *, int, ...) attribute_hidden;
> -extern int __close_nocancel (int) attribute_hidden;
> -extern int __read_nocancel (int, void *, size_t) attribute_hidden;
> -extern int __write_nocancel (int, const void *, size_t) attribute_hidden;
> -extern pid_t __waitpid_nocancel (pid_t, int *, int) attribute_hidden;
> -extern int __openat_nocancel (int fd, const char *fname, int oflag,
> -				mode_t mode) attribute_hidden;
> -extern int __openat64_nocancel (int fd, const char *fname, int oflag,
> -				  mode_t mode) attribute_hidden;
> +/* Uncancelable open.  */
> +#ifdef __NR_open
> +# define open_not_cancel(name, flags, mode) \
> +   INLINE_SYSCALL (open, 3, name, flags, mode)
> +# define open_not_cancel_2(name, flags) \
> +   INLINE_SYSCALL (open, 2, name, flags)
>  #else
> -# define __open_nocancel(name, ...) __open (name, __VA_ARGS__)
> -# define __close_nocancel(fd) __close (fd)
> -# define __read_nocancel(fd, buf, len) __read (fd, buf, len)
> -# define __write_nocancel(fd, buf, len) __write (fd, buf, len)
> -# define __waitpid_nocancel(pid, stat_loc, options) \
> -  __waitpid (pid, stat_loc, options)
> -# define __openat_nocancel(fd, fname, oflag, mode) \
> -  openat (fd, fname, oflag, mode)
> -# define __openat64_nocancel(fd, fname, oflag, mode) \
> -  openat64 (fd, fname, oflag, mode)
> +# define open_not_cancel(name, flags, mode) \
> +   INLINE_SYSCALL (openat, 4, AT_FDCWD, name, flags, mode)
> +# define open_not_cancel_2(name, flags) \
> +   INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags)
>  #endif
>  
> -/* Uncancelable open.  */
> -#define open_not_cancel(name, flags, mode) \
> -   __open_nocancel (name, flags, mode)
> -#define open_not_cancel_2(name, flags) \
> -   __open_nocancel (name, flags)
> +/* Uncancelable read.  */
> +#define __read_nocancel(fd, buf, len) \
> +  INLINE_SYSCALL (read, 3, fd, buf, len)
> +
> +/* Uncancelable write.  */
> +#define __write_nocancel(fd, buf, len) \
> +  INLINE_SYSCALL (write, 3, fd, buf, len)
>  
>  /* Uncancelable openat.  */
>  #define openat_not_cancel(fd, fname, oflag, mode) \
> -  __openat_nocancel (fd, fname, oflag, mode)
> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag, mode)
>  #define openat_not_cancel_3(fd, fname, oflag) \
> -  __openat_nocancel (fd, fname, oflag, 0)
> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag)
>  #define openat64_not_cancel(fd, fname, oflag, mode) \
> -  __openat64_nocancel (fd, fname, oflag, mode)
> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag | O_LARGEFILE, mode)
>  #define openat64_not_cancel_3(fd, fname, oflag) \
> -  __openat64_nocancel (fd, fname, oflag, 0)
> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE)
>  
>  /* Uncancelable close.  */
> +#define __close_nocancel(fd) \
> +  INLINE_SYSCALL (close, 1, fd)
>  #define close_not_cancel(fd) \
>    __close_nocancel (fd)
>  #define close_not_cancel_no_status(fd) \
> @@ -83,17 +83,27 @@ extern int __openat64_nocancel (int fd, const char *fname, int oflag,
>    __fcntl_nocancel (fd, cmd, val)
>  
>  /* Uncancelable waitpid.  */
> -#define waitpid_not_cancel(pid, stat_loc, options) \
> +#define __waitpid_nocancel(pid, stat_loc, options) \
>    INLINE_SYSCALL (wait4, 4, pid, stat_loc, options, NULL)
> +#define waitpid_not_cancel(pid, stat_loc, options) \
> +  __waitpid_nocancel(pid, stat_loc, options)
>  
>  /* Uncancelable pause.  */
>  #define pause_not_cancel() \
> -  __pause_nocancel ()
> +  ({ sigset_t set; 							     \
> +     int __rc = INLINE_SYSCALL (rt_sigprocmask, 4, SIG_BLOCK, NULL, &set,    \
> +				_NSIG / 8);				     \
> +     if (__rc == 0)							     \
> +       __rc = INLINE_SYSCALL (rt_sigsuspend, 2, &set, _NSIG / 8);	     \
> +     __rc;								     \
> +  })
>  
>  /* Uncancelable nanosleep.  */
>  #define nanosleep_not_cancel(requested_time, remaining) \
> -  __nanosleep_nocancel (requested_time, remaining)
> +  INLINE_SYSCALL (nanosleep, 2, requested_time, remaining)
>  
>  /* Uncancelable sigsuspend.  */
>  #define sigsuspend_not_cancel(set) \
> -  __sigsuspend_nocancel (set)
> +  INLINE_SYSCALL (rt_sigsuspend, 2, set, _NSIG / 8)
> +
> +#endif /* NOT_CANCEL_H  */
> diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> index 5b6bb51..3578ea8 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> @@ -41,7 +41,7 @@ __get_clockfreq (void)
>       contains at least one line like:
>       timebase        : 33333333
>       We search for this line and convert the number into an integer.  */
> -  int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
> +  int fd = open_not_cancel_2 ("/proc/cpuinfo", O_RDONLY);
>    if (__glibc_likely (fd != -1))
>      return result;
> 

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Carlos O'Donell]

On 05/21/2015 05:17 PM, Adhemerval Zanella wrote:
> On issue I found for this patchset is it fails for tile (and it is the
> only architecture I found that this warning shows) with:
> 
> --
> 
> malloc.c: In function ‘_int_free’:
> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:51:35: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     may_shrink_heap = n > 0 && val == '2';
>                                    ^
> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:49:9: note: ‘val’ was declared here
>     char val;
> 
> --
> 
> It is due the fact 'read_not_cancel' now calls the INLINE_SYSCALL for the
> architecture and so 'tile' version is triggering this issue for some reason
> (I did not check exactly why, any tile expert could help me out here?).
> 
> The straightforward fix would just initialize this variable, but this is
> just clobbering tile potential issue.  Any recommendations? 

Ask Chris Metcalf for help? :-)

Cheers,
Carlos.

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Chris Metcalf]

On 5/23/2015 10:21 PM, Carlos O'Donell wrote:
> On 05/21/2015 05:17 PM, Adhemerval Zanella wrote:
>> On issue I found for this patchset is it fails for tile (and it is the
>> only architecture I found that this warning shows) with:
>>
>> --
>>
>> malloc.c: In function ‘_int_free’:
>> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:51:35: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>      may_shrink_heap = n > 0 && val == '2';
>>                                     ^
>> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:49:9: note: ‘val’ was declared here
>>      char val;
>>
>> --
>>
>> It is due the fact 'read_not_cancel' now calls the INLINE_SYSCALL for the
>> architecture and so 'tile' version is triggering this issue for some reason
>> (I did not check exactly why, any tile expert could help me out here?).
>>
>> The straightforward fix would just initialize this variable, but this is
>> just clobbering tile potential issue.  Any recommendations?
> Ask Chris Metcalf for help? :-)

Sorry, it's been a crazy week.  I should be back and following libc-alpha again by early
next week.  I'll look at this over the long weekend if I get a chance!

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Chris Metcalf]

On 05/21/2015 05:17 PM, Adhemerval Zanella wrote:
> On issue I found for this patchset is it fails for tile (and it is the
> only architecture I found that this warning shows) with:
>
> --
>
> malloc.c: In function ‘_int_free’:
> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:51:35: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      may_shrink_heap = n > 0 && val == '2';
>                                     ^
> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:49:9: note: ‘val’ was declared here
>      char val;
>
> --
>
> It is due the fact 'read_not_cancel' now calls the INLINE_SYSCALL for the
> architecture and so 'tile' version is triggering this issue for some reason
> (I did not check exactly why, any tile expert could help me out here?).
>
> The straightforward fix would just initialize this variable, but this is
> just clobbering tile potential issue.  Any recommendations?

Thanks for catching this!  It turns out to be a bug where the tile
definition of INLINE_SYSCALL() was shadowing the "val" variable
from the surrounding context.  Using better namespace rules for
the variables defined in INLINE_SYSCALL() fixes it; patch forthcoming.
It shouldn't block the work you're doing in the meanwhile.

Perhaps an argument for -Wshadow?  I don't know how terrible
the fallout of another -W option would be, though :-)  Adding
-Wshadow to my reduced testcase shows the bug nice and clearly,
though of course I only thought to check this after I had already
finished tracking down the bug:

test-bug.c: In function ‘read_check_2’:
test-bug.c:39:19: warning: declaration of ‘val’ shadows a previous local [-Wshadow]
      unsigned long val = INTERNAL_SYSCALL (name, err, nr, args);         \
                    ^
test-bug.c:50:3: note: in expansion of macro ‘INLINE_SYSCALL’
    INLINE_SYSCALL (read, 3, fd, buf, len)
    ^
test-bug.c:52:3: note: in expansion of macro ‘__read_nocancel’
    __read_nocancel (fd, buf, n)
    ^
test-bug.c:58:3: note: in expansion of macro ‘read_not_cancel’
    read_not_cancel (fd, &val, 1);
    ^
test-bug.c:57:8: warning: shadowed declaration is here [-Wshadow]
    char val;
         ^


>
> On 20-05-2015 11:19, Adhemerval Zanella wrote:
>> Hi
>>
>> This patch is one of the required adjustments for the fix for bz12683
>> (Race conditions in pthread cancellation) and the idea is to not rely
>> on the non-cancelable entry points for cancelable syscalls (since the
>> upcoming fill will remove them).
>>   
>> This patch uses inline calls (through INLINE_SYSCALL macro) to define
>> the non-cancellable functions macros to avoid use of the
>> syscall_nocancel entrypoint.
>>
>> Tested on i386, x86_64, x32, powerpc32, powerpc64, arm, and aarch64.
>>
>> ---
>>
>> 	* sysdeps/unix/sysv/linux/not-cancel.h (open_not_cancel): Rewrite to
>> 	be an inline implementation regardless of library is built within.
>> 	(open_not_cancel_2): Likewise.
>> 	(__read_nocancel): Likewise.
>> 	(__write_nocancel): Likewise.
>> 	(openat_not_cancel): Likewise.
>> 	(openat_not_cancel_3): Likewise.
>> 	(openat64_not_cancel): Likewise.
>> 	(openat64_not_cancel_3): Likewise.
>> 	(__close_nocancel): Likewise.
>> 	(pause_not_cancel): Likewise.
>> 	(nanosleep_not_cancel): Likewise.
>> 	(sigsuspend_not_cancel): Likewise.
>>
>> --
>>
>> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
>> index 62d487f..8a358fd 100644
>> --- a/sysdeps/unix/sysv/linux/not-cancel.h
>> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
>> @@ -17,48 +17,48 @@
>>      License along with the GNU C Library; if not, see
>>      <http://www.gnu.org/licenses/>.  */
>>   
>> +#ifndef NOT_CANCEL_H
>> +# define NOT_CANCEL_H
>> +
>>   #include <sysdep.h>
>> +#include <errno.h>
>> +#include <unistd.h>
>> +#include <sys/syscall.h>
>>   
>> -#if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
>> -extern int __open_nocancel (const char *, int, ...) attribute_hidden;
>> -extern int __close_nocancel (int) attribute_hidden;
>> -extern int __read_nocancel (int, void *, size_t) attribute_hidden;
>> -extern int __write_nocancel (int, const void *, size_t) attribute_hidden;
>> -extern pid_t __waitpid_nocancel (pid_t, int *, int) attribute_hidden;
>> -extern int __openat_nocancel (int fd, const char *fname, int oflag,
>> -				mode_t mode) attribute_hidden;
>> -extern int __openat64_nocancel (int fd, const char *fname, int oflag,
>> -				  mode_t mode) attribute_hidden;
>> +/* Uncancelable open.  */
>> +#ifdef __NR_open
>> +# define open_not_cancel(name, flags, mode) \
>> +   INLINE_SYSCALL (open, 3, name, flags, mode)
>> +# define open_not_cancel_2(name, flags) \
>> +   INLINE_SYSCALL (open, 2, name, flags)
>>   #else
>> -# define __open_nocancel(name, ...) __open (name, __VA_ARGS__)
>> -# define __close_nocancel(fd) __close (fd)
>> -# define __read_nocancel(fd, buf, len) __read (fd, buf, len)
>> -# define __write_nocancel(fd, buf, len) __write (fd, buf, len)
>> -# define __waitpid_nocancel(pid, stat_loc, options) \
>> -  __waitpid (pid, stat_loc, options)
>> -# define __openat_nocancel(fd, fname, oflag, mode) \
>> -  openat (fd, fname, oflag, mode)
>> -# define __openat64_nocancel(fd, fname, oflag, mode) \
>> -  openat64 (fd, fname, oflag, mode)
>> +# define open_not_cancel(name, flags, mode) \
>> +   INLINE_SYSCALL (openat, 4, AT_FDCWD, name, flags, mode)
>> +# define open_not_cancel_2(name, flags) \
>> +   INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags)
>>   #endif
>>   
>> -/* Uncancelable open.  */
>> -#define open_not_cancel(name, flags, mode) \
>> -   __open_nocancel (name, flags, mode)
>> -#define open_not_cancel_2(name, flags) \
>> -   __open_nocancel (name, flags)
>> +/* Uncancelable read.  */
>> +#define __read_nocancel(fd, buf, len) \
>> +  INLINE_SYSCALL (read, 3, fd, buf, len)
>> +
>> +/* Uncancelable write.  */
>> +#define __write_nocancel(fd, buf, len) \
>> +  INLINE_SYSCALL (write, 3, fd, buf, len)
>>   
>>   /* Uncancelable openat.  */
>>   #define openat_not_cancel(fd, fname, oflag, mode) \
>> -  __openat_nocancel (fd, fname, oflag, mode)
>> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag, mode)
>>   #define openat_not_cancel_3(fd, fname, oflag) \
>> -  __openat_nocancel (fd, fname, oflag, 0)
>> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag)
>>   #define openat64_not_cancel(fd, fname, oflag, mode) \
>> -  __openat64_nocancel (fd, fname, oflag, mode)
>> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag | O_LARGEFILE, mode)
>>   #define openat64_not_cancel_3(fd, fname, oflag) \
>> -  __openat64_nocancel (fd, fname, oflag, 0)
>> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE)
>>   
>>   /* Uncancelable close.  */
>> +#define __close_nocancel(fd) \
>> +  INLINE_SYSCALL (close, 1, fd)
>>   #define close_not_cancel(fd) \
>>     __close_nocancel (fd)
>>   #define close_not_cancel_no_status(fd) \
>> @@ -83,17 +83,27 @@ extern int __openat64_nocancel (int fd, const char *fname, int oflag,
>>     __fcntl_nocancel (fd, cmd, val)
>>   
>>   /* Uncancelable waitpid.  */
>> -#define waitpid_not_cancel(pid, stat_loc, options) \
>> +#define __waitpid_nocancel(pid, stat_loc, options) \
>>     INLINE_SYSCALL (wait4, 4, pid, stat_loc, options, NULL)
>> +#define waitpid_not_cancel(pid, stat_loc, options) \
>> +  __waitpid_nocancel(pid, stat_loc, options)
>>   
>>   /* Uncancelable pause.  */
>>   #define pause_not_cancel() \
>> -  __pause_nocancel ()
>> +  ({ sigset_t set; 							     \
>> +     int __rc = INLINE_SYSCALL (rt_sigprocmask, 4, SIG_BLOCK, NULL, &set,    \
>> +				_NSIG / 8);				     \
>> +     if (__rc == 0)							     \
>> +       __rc = INLINE_SYSCALL (rt_sigsuspend, 2, &set, _NSIG / 8);	     \
>> +     __rc;								     \
>> +  })
>>   
>>   /* Uncancelable nanosleep.  */
>>   #define nanosleep_not_cancel(requested_time, remaining) \
>> -  __nanosleep_nocancel (requested_time, remaining)
>> +  INLINE_SYSCALL (nanosleep, 2, requested_time, remaining)
>>   
>>   /* Uncancelable sigsuspend.  */
>>   #define sigsuspend_not_cancel(set) \
>> -  __sigsuspend_nocancel (set)
>> +  INLINE_SYSCALL (rt_sigsuspend, 2, set, _NSIG / 8)
>> +
>> +#endif /* NOT_CANCEL_H  */
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> index 5b6bb51..3578ea8 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> +++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>> @@ -41,7 +41,7 @@ __get_clockfreq (void)
>>        contains at least one line like:
>>        timebase        : 33333333
>>        We search for this line and convert the number into an integer.  */
>> -  int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
>> +  int fd = open_not_cancel_2 ("/proc/cpuinfo", O_RDONLY);
>>     if (__glibc_likely (fd != -1))
>>       return result;
>>

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

[PATCH] tile: use better variable naming in INLINE_SYSCALL

[Chris Metcalf]

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(-)

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.  */
-- 
2.1.2

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Adhemerval Zanella]

On 26-05-2015 14:06, Chris Metcalf wrote:
> On 05/21/2015 05:17 PM, Adhemerval Zanella wrote:
>> On issue I found for this patchset is it fails for tile (and it is the
>> only architecture I found that this warning shows) with:
>>
>> -- 
>>
>> malloc.c: In function ‘_int_free’:
>> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:51:35: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>      may_shrink_heap = n > 0 && val == '2';
>>                                     ^
>> ../sysdeps/unix/sysv/linux/malloc-sysdep.h:49:9: note: ‘val’ was declared here
>>      char val;
>>
>> -- 
>>
>> It is due the fact 'read_not_cancel' now calls the INLINE_SYSCALL for the
>> architecture and so 'tile' version is triggering this issue for some reason
>> (I did not check exactly why, any tile expert could help me out here?).
>>
>> The straightforward fix would just initialize this variable, but this is
>> just clobbering tile potential issue.  Any recommendations?
> 
> Thanks for catching this!  It turns out to be a bug where the tile
> definition of INLINE_SYSCALL() was shadowing the "val" variable
> from the surrounding context.  Using better namespace rules for
> the variables defined in INLINE_SYSCALL() fixes it; patch forthcoming.
> It shouldn't block the work you're doing in the meanwhile.
> 
> Perhaps an argument for -Wshadow?  I don't know how terrible
> the fallout of another -W option would be, though :-)  Adding
> -Wshadow to my reduced testcase shows the bug nice and clearly,
> though of course I only thought to check this after I had already
> finished tracking down the bug:

It seemed a tile issue in fact, thanks for reviewing it.  For -Wshadow,
I am not sure because for some GCC version it is seems too strict [1]
and GLIBC still nominally supports GCC 4.6.

[1] http://stackoverflow.com/questions/2958457/gcc-wshadow-is-too-strict

> 
> test-bug.c: In function ‘read_check_2’:
> test-bug.c:39:19: warning: declaration of ‘val’ shadows a previous local [-Wshadow]
>      unsigned long val = INTERNAL_SYSCALL (name, err, nr, args);         \
>                    ^
> test-bug.c:50:3: note: in expansion of macro ‘INLINE_SYSCALL’
>    INLINE_SYSCALL (read, 3, fd, buf, len)
>    ^
> test-bug.c:52:3: note: in expansion of macro ‘__read_nocancel’
>    __read_nocancel (fd, buf, n)
>    ^
> test-bug.c:58:3: note: in expansion of macro ‘read_not_cancel’
>    read_not_cancel (fd, &val, 1);
>    ^
> test-bug.c:57:8: warning: shadowed declaration is here [-Wshadow]
>    char val;
>         ^
> 
> 
>>
>> On 20-05-2015 11:19, Adhemerval Zanella wrote:
>>> Hi
>>>
>>> This patch is one of the required adjustments for the fix for bz12683
>>> (Race conditions in pthread cancellation) and the idea is to not rely
>>> on the non-cancelable entry points for cancelable syscalls (since the
>>> upcoming fill will remove them).
>>>   This patch uses inline calls (through INLINE_SYSCALL macro) to define
>>> the non-cancellable functions macros to avoid use of the
>>> syscall_nocancel entrypoint.
>>>
>>> Tested on i386, x86_64, x32, powerpc32, powerpc64, arm, and aarch64.
>>>
>>> ---
>>>
>>>     * sysdeps/unix/sysv/linux/not-cancel.h (open_not_cancel): Rewrite to
>>>     be an inline implementation regardless of library is built within.
>>>     (open_not_cancel_2): Likewise.
>>>     (__read_nocancel): Likewise.
>>>     (__write_nocancel): Likewise.
>>>     (openat_not_cancel): Likewise.
>>>     (openat_not_cancel_3): Likewise.
>>>     (openat64_not_cancel): Likewise.
>>>     (openat64_not_cancel_3): Likewise.
>>>     (__close_nocancel): Likewise.
>>>     (pause_not_cancel): Likewise.
>>>     (nanosleep_not_cancel): Likewise.
>>>     (sigsuspend_not_cancel): Likewise.
>>>
>>> -- 
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
>>> index 62d487f..8a358fd 100644
>>> --- a/sysdeps/unix/sysv/linux/not-cancel.h
>>> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
>>> @@ -17,48 +17,48 @@
>>>      License along with the GNU C Library; if not, see
>>>      <http://www.gnu.org/licenses/>.  */
>>>   +#ifndef NOT_CANCEL_H
>>> +# define NOT_CANCEL_H
>>> +
>>>   #include <sysdep.h>
>>> +#include <errno.h>
>>> +#include <unistd.h>
>>> +#include <sys/syscall.h>
>>>   -#if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
>>> -extern int __open_nocancel (const char *, int, ...) attribute_hidden;
>>> -extern int __close_nocancel (int) attribute_hidden;
>>> -extern int __read_nocancel (int, void *, size_t) attribute_hidden;
>>> -extern int __write_nocancel (int, const void *, size_t) attribute_hidden;
>>> -extern pid_t __waitpid_nocancel (pid_t, int *, int) attribute_hidden;
>>> -extern int __openat_nocancel (int fd, const char *fname, int oflag,
>>> -                mode_t mode) attribute_hidden;
>>> -extern int __openat64_nocancel (int fd, const char *fname, int oflag,
>>> -                  mode_t mode) attribute_hidden;
>>> +/* Uncancelable open.  */
>>> +#ifdef __NR_open
>>> +# define open_not_cancel(name, flags, mode) \
>>> +   INLINE_SYSCALL (open, 3, name, flags, mode)
>>> +# define open_not_cancel_2(name, flags) \
>>> +   INLINE_SYSCALL (open, 2, name, flags)
>>>   #else
>>> -# define __open_nocancel(name, ...) __open (name, __VA_ARGS__)
>>> -# define __close_nocancel(fd) __close (fd)
>>> -# define __read_nocancel(fd, buf, len) __read (fd, buf, len)
>>> -# define __write_nocancel(fd, buf, len) __write (fd, buf, len)
>>> -# define __waitpid_nocancel(pid, stat_loc, options) \
>>> -  __waitpid (pid, stat_loc, options)
>>> -# define __openat_nocancel(fd, fname, oflag, mode) \
>>> -  openat (fd, fname, oflag, mode)
>>> -# define __openat64_nocancel(fd, fname, oflag, mode) \
>>> -  openat64 (fd, fname, oflag, mode)
>>> +# define open_not_cancel(name, flags, mode) \
>>> +   INLINE_SYSCALL (openat, 4, AT_FDCWD, name, flags, mode)
>>> +# define open_not_cancel_2(name, flags) \
>>> +   INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags)
>>>   #endif
>>>   -/* Uncancelable open.  */
>>> -#define open_not_cancel(name, flags, mode) \
>>> -   __open_nocancel (name, flags, mode)
>>> -#define open_not_cancel_2(name, flags) \
>>> -   __open_nocancel (name, flags)
>>> +/* Uncancelable read.  */
>>> +#define __read_nocancel(fd, buf, len) \
>>> +  INLINE_SYSCALL (read, 3, fd, buf, len)
>>> +
>>> +/* Uncancelable write.  */
>>> +#define __write_nocancel(fd, buf, len) \
>>> +  INLINE_SYSCALL (write, 3, fd, buf, len)
>>>     /* Uncancelable openat.  */
>>>   #define openat_not_cancel(fd, fname, oflag, mode) \
>>> -  __openat_nocancel (fd, fname, oflag, mode)
>>> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag, mode)
>>>   #define openat_not_cancel_3(fd, fname, oflag) \
>>> -  __openat_nocancel (fd, fname, oflag, 0)
>>> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag)
>>>   #define openat64_not_cancel(fd, fname, oflag, mode) \
>>> -  __openat64_nocancel (fd, fname, oflag, mode)
>>> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag | O_LARGEFILE, mode)
>>>   #define openat64_not_cancel_3(fd, fname, oflag) \
>>> -  __openat64_nocancel (fd, fname, oflag, 0)
>>> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE)
>>>     /* Uncancelable close.  */
>>> +#define __close_nocancel(fd) \
>>> +  INLINE_SYSCALL (close, 1, fd)
>>>   #define close_not_cancel(fd) \
>>>     __close_nocancel (fd)
>>>   #define close_not_cancel_no_status(fd) \
>>> @@ -83,17 +83,27 @@ extern int __openat64_nocancel (int fd, const char *fname, int oflag,
>>>     __fcntl_nocancel (fd, cmd, val)
>>>     /* Uncancelable waitpid.  */
>>> -#define waitpid_not_cancel(pid, stat_loc, options) \
>>> +#define __waitpid_nocancel(pid, stat_loc, options) \
>>>     INLINE_SYSCALL (wait4, 4, pid, stat_loc, options, NULL)
>>> +#define waitpid_not_cancel(pid, stat_loc, options) \
>>> +  __waitpid_nocancel(pid, stat_loc, options)
>>>     /* Uncancelable pause.  */
>>>   #define pause_not_cancel() \
>>> -  __pause_nocancel ()
>>> +  ({ sigset_t set;                                  \
>>> +     int __rc = INLINE_SYSCALL (rt_sigprocmask, 4, SIG_BLOCK, NULL, &set,    \
>>> +                _NSIG / 8);                     \
>>> +     if (__rc == 0)                                 \
>>> +       __rc = INLINE_SYSCALL (rt_sigsuspend, 2, &set, _NSIG / 8);         \
>>> +     __rc;                                     \
>>> +  })
>>>     /* Uncancelable nanosleep.  */
>>>   #define nanosleep_not_cancel(requested_time, remaining) \
>>> -  __nanosleep_nocancel (requested_time, remaining)
>>> +  INLINE_SYSCALL (nanosleep, 2, requested_time, remaining)
>>>     /* Uncancelable sigsuspend.  */
>>>   #define sigsuspend_not_cancel(set) \
>>> -  __sigsuspend_nocancel (set)
>>> +  INLINE_SYSCALL (rt_sigsuspend, 2, set, _NSIG / 8)
>>> +
>>> +#endif /* NOT_CANCEL_H  */
>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>>> index 5b6bb51..3578ea8 100644
>>> --- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>>> +++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
>>> @@ -41,7 +41,7 @@ __get_clockfreq (void)
>>>        contains at least one line like:
>>>        timebase        : 33333333
>>>        We search for this line and convert the number into an integer.  */
>>> -  int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
>>> +  int fd = open_not_cancel_2 ("/proc/cpuinfo", O_RDONLY);
>>>     if (__glibc_likely (fd != -1))
>>>       return result;
>>>
> 

Re: [PATCH] tile: use better variable naming in INLINE_SYSCALL

[Carlos O'Donell]

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.  */
> 

Re: [PATCH] tile: use better variable naming in INLINE_SYSCALL

[Chris Metcalf]

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.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Joseph Myers]

On Tue, 26 May 2015, Chris Metcalf wrote:

> Perhaps an argument for -Wshadow?  I don't know how terrible
> the fallout of another -W option would be, though :-)  Adding
> -Wshadow to my reduced testcase shows the bug nice and clearly,
> though of course I only thought to check this after I had already
> finished tracking down the bug:

See <https://sourceware.org/ml/libc-alpha/2014-10/msg00336.html>.  I think 
we should try to find a subset of Roland's changes that don't affect 
stripped installed shared libraries at all, and so are trivially safe to 
go in - then changes that only affect assertion line numbers, then look at 
those with more substantial code generation effects to understand why such 
changes occur.  (Then of course there would be further work to do before 
-Wshadow could be added by default without -Wno-error=shadow.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] tile: use better variable naming in INLINE_SYSCALL

[Joseph Myers]

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)?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Use inline syscalls for non-cancellable versions

[Adhemerval Zanella]

HI tile issue now fixed, I will commit it soon unless someone opposes it.

On 20-05-2015 11:19, Adhemerval Zanella wrote:
> Hi
> 
> This patch is one of the required adjustments for the fix for bz12683
> (Race conditions in pthread cancellation) and the idea is to not rely
> on the non-cancelable entry points for cancelable syscalls (since the
> upcoming fill will remove them).
>  
> This patch uses inline calls (through INLINE_SYSCALL macro) to define
> the non-cancellable functions macros to avoid use of the
> syscall_nocancel entrypoint.
> 
> Tested on i386, x86_64, x32, powerpc32, powerpc64, arm, and aarch64.
> 
> ---
> 
> 	* sysdeps/unix/sysv/linux/not-cancel.h (open_not_cancel): Rewrite to
> 	be an inline implementation regardless of library is built within.
> 	(open_not_cancel_2): Likewise.
> 	(__read_nocancel): Likewise.
> 	(__write_nocancel): Likewise.
> 	(openat_not_cancel): Likewise.
> 	(openat_not_cancel_3): Likewise.
> 	(openat64_not_cancel): Likewise.
> 	(openat64_not_cancel_3): Likewise.
> 	(__close_nocancel): Likewise.
> 	(pause_not_cancel): Likewise.
> 	(nanosleep_not_cancel): Likewise.
> 	(sigsuspend_not_cancel): Likewise.
> 
> --
> 
> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
> index 62d487f..8a358fd 100644
> --- a/sysdeps/unix/sysv/linux/not-cancel.h
> +++ b/sysdeps/unix/sysv/linux/not-cancel.h
> @@ -17,48 +17,48 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#ifndef NOT_CANCEL_H
> +# define NOT_CANCEL_H
> +
>  #include <sysdep.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
>  
> -#if IS_IN (libc) || IS_IN (libpthread) || IS_IN (librt)
> -extern int __open_nocancel (const char *, int, ...) attribute_hidden;
> -extern int __close_nocancel (int) attribute_hidden;
> -extern int __read_nocancel (int, void *, size_t) attribute_hidden;
> -extern int __write_nocancel (int, const void *, size_t) attribute_hidden;
> -extern pid_t __waitpid_nocancel (pid_t, int *, int) attribute_hidden;
> -extern int __openat_nocancel (int fd, const char *fname, int oflag,
> -				mode_t mode) attribute_hidden;
> -extern int __openat64_nocancel (int fd, const char *fname, int oflag,
> -				  mode_t mode) attribute_hidden;
> +/* Uncancelable open.  */
> +#ifdef __NR_open
> +# define open_not_cancel(name, flags, mode) \
> +   INLINE_SYSCALL (open, 3, name, flags, mode)
> +# define open_not_cancel_2(name, flags) \
> +   INLINE_SYSCALL (open, 2, name, flags)
>  #else
> -# define __open_nocancel(name, ...) __open (name, __VA_ARGS__)
> -# define __close_nocancel(fd) __close (fd)
> -# define __read_nocancel(fd, buf, len) __read (fd, buf, len)
> -# define __write_nocancel(fd, buf, len) __write (fd, buf, len)
> -# define __waitpid_nocancel(pid, stat_loc, options) \
> -  __waitpid (pid, stat_loc, options)
> -# define __openat_nocancel(fd, fname, oflag, mode) \
> -  openat (fd, fname, oflag, mode)
> -# define __openat64_nocancel(fd, fname, oflag, mode) \
> -  openat64 (fd, fname, oflag, mode)
> +# define open_not_cancel(name, flags, mode) \
> +   INLINE_SYSCALL (openat, 4, AT_FDCWD, name, flags, mode)
> +# define open_not_cancel_2(name, flags) \
> +   INLINE_SYSCALL (openat, 3, AT_FDCWD, name, flags)
>  #endif
>  
> -/* Uncancelable open.  */
> -#define open_not_cancel(name, flags, mode) \
> -   __open_nocancel (name, flags, mode)
> -#define open_not_cancel_2(name, flags) \
> -   __open_nocancel (name, flags)
> +/* Uncancelable read.  */
> +#define __read_nocancel(fd, buf, len) \
> +  INLINE_SYSCALL (read, 3, fd, buf, len)
> +
> +/* Uncancelable write.  */
> +#define __write_nocancel(fd, buf, len) \
> +  INLINE_SYSCALL (write, 3, fd, buf, len)
>  
>  /* Uncancelable openat.  */
>  #define openat_not_cancel(fd, fname, oflag, mode) \
> -  __openat_nocancel (fd, fname, oflag, mode)
> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag, mode)
>  #define openat_not_cancel_3(fd, fname, oflag) \
> -  __openat_nocancel (fd, fname, oflag, 0)
> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag)
>  #define openat64_not_cancel(fd, fname, oflag, mode) \
> -  __openat64_nocancel (fd, fname, oflag, mode)
> +  INLINE_SYSCALL (openat, 4, fd, fname, oflag | O_LARGEFILE, mode)
>  #define openat64_not_cancel_3(fd, fname, oflag) \
> -  __openat64_nocancel (fd, fname, oflag, 0)
> +  INLINE_SYSCALL (openat, 3, fd, fname, oflag | O_LARGEFILE)
>  
>  /* Uncancelable close.  */
> +#define __close_nocancel(fd) \
> +  INLINE_SYSCALL (close, 1, fd)
>  #define close_not_cancel(fd) \
>    __close_nocancel (fd)
>  #define close_not_cancel_no_status(fd) \
> @@ -83,17 +83,27 @@ extern int __openat64_nocancel (int fd, const char *fname, int oflag,
>    __fcntl_nocancel (fd, cmd, val)
>  
>  /* Uncancelable waitpid.  */
> -#define waitpid_not_cancel(pid, stat_loc, options) \
> +#define __waitpid_nocancel(pid, stat_loc, options) \
>    INLINE_SYSCALL (wait4, 4, pid, stat_loc, options, NULL)
> +#define waitpid_not_cancel(pid, stat_loc, options) \
> +  __waitpid_nocancel(pid, stat_loc, options)
>  
>  /* Uncancelable pause.  */
>  #define pause_not_cancel() \
> -  __pause_nocancel ()
> +  ({ sigset_t set; 							     \
> +     int __rc = INLINE_SYSCALL (rt_sigprocmask, 4, SIG_BLOCK, NULL, &set,    \
> +				_NSIG / 8);				     \
> +     if (__rc == 0)							     \
> +       __rc = INLINE_SYSCALL (rt_sigsuspend, 2, &set, _NSIG / 8);	     \
> +     __rc;								     \
> +  })
>  
>  /* Uncancelable nanosleep.  */
>  #define nanosleep_not_cancel(requested_time, remaining) \
> -  __nanosleep_nocancel (requested_time, remaining)
> +  INLINE_SYSCALL (nanosleep, 2, requested_time, remaining)
>  
>  /* Uncancelable sigsuspend.  */
>  #define sigsuspend_not_cancel(set) \
> -  __sigsuspend_nocancel (set)
> +  INLINE_SYSCALL (rt_sigsuspend, 2, set, _NSIG / 8)
> +
> +#endif /* NOT_CANCEL_H  */
> diff --git a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> index 5b6bb51..3578ea8 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
> @@ -41,7 +41,7 @@ __get_clockfreq (void)
>       contains at least one line like:
>       timebase        : 33333333
>       We search for this line and convert the number into an integer.  */
> -  int fd = __open_nocancel ("/proc/cpuinfo", O_RDONLY);
> +  int fd = open_not_cancel_2 ("/proc/cpuinfo", O_RDONLY);
>    if (__glibc_likely (fd != -1))
>      return result;
>