From patchwork Tue Nov 14 18:05:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Carroll, Paul" X-Patchwork-Id: 24242 Received: (qmail 4359 invoked by alias); 14 Nov 2017 18:05:41 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 4350 invoked by uid 89); 14 Nov 2017 18:05:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KB_WAM_FROM_NAME_SINGLEWORD, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=engineers, H*MI:be61, H*M:be61, urn X-HELO: relay1.mentorg.com From: Paul Carroll To: Subject: [PATCH] pthread_cleanup_push macro generates warning when -Wclobbered is set Message-ID: <3255e05c-196f-be61-799d-ad64828a721a@mentorg.com> Date: Tue, 14 Nov 2017 13:05:27 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 X-ClientProxiedBy: svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) To SVR-ORW-MBX-04.mgc.mentorg.com (147.34.90.204) I originally submitted this patch back in May as part of Bugzilla #21513. That issue was never pursued to completion. There is an open issue filed against the Gcc Middle-End - Bugzilla #61118. In that issue, it is noted that the use of the pthread_cleanup_push macro from pthread.h will generate 2 warnings when -Wclobbered is set. The warnings are for the '__cancel_routine' and '__cancel_arg' variables that are created as part of the macro definition. The warning occurs because of the presence of a sigsetjmp() call after those 2 variables are defined. For our customer, who compiles with -Werror, the presence of these warnings is unacceptable. In the absence of a GCC fix, a solution is to modify the macro definitions in pthread.h so as to mark those variables as 'volatile'. The changes would be made to both pthread_cleanup_push and pthread_cleanup_push_defer_np macro definitions. The changes would make the macros look something like this: # define pthread_cleanup_push(routine, arg) \ do { \ __pthread_unwind_buf_t __cancel_buf; \ void (* volatile __cancel_routine) (void *) = (routine); \ void * volatile __cancel_arg = (arg); \ : Since those variables are now reloaded whenever they are used and thus cannot not be clobbered by the setjmp, the compilation is quiet. Attempts were made to use '#pragma GCC diagnostic' to turn off the warning specifically for this macro expansion, but that had no effect. And, as has been noted in the GCC Bugzilla issue, this issue only occurs when optimization is used and when files are preprocessed (i.e., not .i files). The analysis behind this patch was noted by one of our engineers: (a) The warning is a false positive. The _cancel_routine and cancel_arg variables will not in fact be modified between the _sigsetjmp call and the corresponding longjmp (if any) with the same jmp_buf. (b) The compiler has no way of knowing that it is a false positive. The calls to the macros are within a loop and the compiler cannot tell that a longjmp does not occur that uses the jmp_buf from a previous iteration. (c) As these warnings occur quite late and are based on when RTL pseudos for particular variables are live, they are very sensitive to details of source code and code generation. (d) Occurring that late also means the macro expansion contexts that are available earlier in compilation are not available here, only the expansion-point location. This probably explains why diagnostic pragmas inside the cleanup macros do not serve to disable the warning. (e) Going via preprocessed source simplifies the location information to the form that can be represented in .i files. By forcing every expanded token to be either a system header token or not, as marked in the .i file, it may well perturb details of when warnings occur for code coming partly from expansions of macros in system headers. The warning state in normal compilation matters more than the warning state when information has been discarded by going through a .i file. (f) On that basis, although it is a workaround for a compiler limitation regarding diagnostic pragmas, adding volatile in pthread.h seems a reasonable approach to avoiding the warning. diff -urN a/ChangeLog b/ChangeLog --- a/ChangeLog 2017-08-02 07:57:16.000000000 -0500 +++ b/ChangeLog 2017-11-14 10:44:33.367788400 -0500 @@ -1,3 +1,13 @@ +2017-11-14 Paul Carroll + + [BZ #21513] + * sysdeps/nptl/pthread.h (pthread_cleanup_push): Added 'volatile' + to __cancel_routine and __cancel_arg definitions. + * sysdeps/unix/sysv/linux/hppa/pthread.h (pthread_cleanup_push): Added + 'volatile' to __cancel_routine and __cancel_arg definitions. + * nptl/Makefile (tests): Add tst-clobbered.c. + * nptl/tst-clobbered.c: New file. + 2017-08-02 Siddhesh Poyarekar * version.h (RELEASE): Set to "stable" diff -urN a/nptl/Makefile b/nptl/Makefile --- a/nptl/Makefile 2017-08-02 07:57:16.000000000 -0500 +++ b/nptl/Makefile 2017-11-14 10:44:12.211757900 -0500 @@ -302,7 +302,8 @@ c89 gnu89 c99 gnu99 c11 gnu11) \ tst-bad-schedattr \ tst-thread_local1 tst-mutex-errorcheck tst-robust10 \ - tst-robust-fork tst-create-detached tst-memstream + tst-robust-fork tst-create-detached tst-memstream \ + tst-clobbered tests-internal := tst-typesizes \ tst-rwlock19 tst-rwlock20 \ diff -urN a/nptl/tst-clobbered.c b/nptl/tst-clobbered.c --- a/nptl/tst-clobbered.c 1969-12-31 19:00:00.000000000 -0500 +++ b/nptl/tst-clobbered.c 2017-11-14 10:45:50.376896700 -0500 @@ -0,0 +1,39 @@ +#include +#pragma GCC diagnostic error "-Wclobbered" + +#include + +void cleanup_fn(void *mutex) +{ +} + +typedef struct { + size_t progress; + size_t total; + pthread_mutex_t mutex; + pthread_cond_t cond; + double min_wait; +} dmnsn_future; + +void +dmnsn_future_wait(dmnsn_future *future, double progress) +{ + pthread_mutex_lock(&future->mutex); + while ((double)future->progress/future->total < progress) { + /* Warning goes away without this block */ + if (progress < future->min_wait) { + future->min_wait = progress; + } + + pthread_cleanup_push(cleanup_fn, &future->mutex); + pthread_cond_wait(&future->cond, &future->mutex); + pthread_cleanup_pop(0); + } + pthread_mutex_unlock(&future->mutex); +} + +int +main(int argc, char *argv[]) +{ + exit(0); +} diff -urN a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h --- a/sysdeps/nptl/pthread.h 2017-08-02 07:57:16.000000000 -0500 +++ b/sysdeps/nptl/pthread.h 2017-11-14 10:48:12.943097200 -0500 @@ -665,8 +665,8 @@ # define pthread_cleanup_push(routine, arg) \ do { \ __pthread_unwind_buf_t __cancel_buf; \ - void (*__cancel_routine) (void *) = (routine); \ - void *__cancel_arg = (arg); \ + void (* volatile __cancel_routine) (void *) = (routine); \ + void * volatile __cancel_arg = (arg); \ int __not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) (void *) \ __cancel_buf.__cancel_jmp_buf, 0); \ if (__glibc_unlikely (__not_first_call)) \ @@ -700,8 +700,8 @@ # define pthread_cleanup_push_defer_np(routine, arg) \ do { \ __pthread_unwind_buf_t __cancel_buf; \ - void (*__cancel_routine) (void *) = (routine); \ - void *__cancel_arg = (arg); \ + void (* volatile __cancel_routine) (void *) = (routine); \ + void * volatile __cancel_arg = (arg); \ int __not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) (void *) \ __cancel_buf.__cancel_jmp_buf, 0); \ if (__glibc_unlikely (__not_first_call)) \ diff -urN a/sysdeps/unix/sysv/linux/hppa/pthread.h b/sysdeps/unix/sysv/linux/hppa/pthread.h --- a/sysdeps/unix/sysv/linux/hppa/pthread.h 2017-08-02 07:57:16.000000000 -0500 +++ b/sysdeps/unix/sysv/linux/hppa/pthread.h 2017-11-14 10:50:03.305252400 -0500 @@ -641,8 +641,8 @@ # define pthread_cleanup_push(routine, arg) \ do { \ __pthread_unwind_buf_t __cancel_buf; \ - void (*__cancel_routine) (void *) = (routine); \ - void *__cancel_arg = (arg); \ + void (* volatile __cancel_routine) (void *) = (routine); \ + void * volatile __cancel_arg = (arg); \ int __not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) (void *) \ __cancel_buf.__cancel_jmp_buf, 0); \ if (__glibc_unlikely (__not_first_call)) \ @@ -676,8 +676,8 @@ # define pthread_cleanup_push_defer_np(routine, arg) \ do { \ __pthread_unwind_buf_t __cancel_buf; \ - void (*__cancel_routine) (void *) = (routine); \ - void *__cancel_arg = (arg); \ + void (* volatile __cancel_routine) (void *) = (routine); \ + void * volatile __cancel_arg = (arg); \ int __not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) (void *) \ __cancel_buf.__cancel_jmp_buf, 0); \ if (__glibc_unlikely (__not_first_call)) \