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

[Chris Metcalf <cmetcalf@ezchip.com>]

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