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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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;
>>>
>