[PATCH] pthread_cleanup_push macro generates warning when -Wclobbered is set
Paul Carroll
Paul_Carroll@mentor.com
Tue Nov 14 18:05:00 GMT 2017
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.
-------------- next part --------------
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 <pcarroll@codesourcery.com>
+
+ [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 <siddhesh@sourceware.org>
* 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 <pthread.h>
+#pragma GCC diagnostic error "-Wclobbered"
+
+#include <stdlib.h>
+
+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)) \
More information about the Libc-alpha
mailing list