Remove __PTHREAD_MUTEX_HAVE_ELISION undefined warning
Commit Message
On 26-03-2014 13:36, Roland McGrath wrote:
>> +#if !defined _PTHREAD_H
> This is OK but there is no reason not to use #ifndef.
>
>> +# error "Never include this file directly. Use <pthread.h> instead"
> Two spaces between the sentences, and a period at the end of each.
>
> (Those two also apply to the sysdeps variant files you're adding, of course.)
>
>> +#define __PTHREAD_MUTEX_HAVE_ELISION 0
> As Joseph also requested, add an explanatory comment describing the
> protocol for this macro and how other sysdeps variant files might define
> it.
>
>> +#endif
>> diff --git a/nptl/sysdeps/pthread/pthread.h b/nptl/sysdeps/pthread/pthread.h
>> index 1e0c5dc..39d6b91 100644
>> --- a/nptl/sysdeps/pthread/pthread.h
>> +++ b/nptl/sysdeps/pthread/pthread.h
>> @@ -26,6 +26,7 @@
>> #include <bits/pthreadtypes.h>
>> #include <bits/setjmp.h>
>> #include <bits/wordsize.h>
>> +#include <bits/pthread-elision.h>
> I'd put it right after bits/pthreadtypes.h.
>
>> --- a/posix/Makefile
>> +++ b/posix/Makefile
>> @@ -29,7 +29,7 @@ headers := sys/utsname.h sys/times.h sys/wait.h sys/types.h unistd.h \
>> bits/local_lim.h tar.h bits/utsname.h bits/confname.h \
>> bits/waitflags.h bits/waitstatus.h sys/unistd.h sched.h \
>> bits/sched.h re_comp.h wait.h bits/environments.h cpio.h \
>> - sys/sysmacros.h spawn.h bits/unistd.h
>> + sys/sysmacros.h spawn.h bits/unistd.h bits/pthread-elision.h
> That belongs in nptl/Makefile.
>
>
> Thanks,
> Roland
>
Thanks for the review, what about this version (fixed all the comments):
--
* nptl/sysdeps/pthread/bits/pthread-elision.h: New header: define
default lock elision support and defines.
* nptl/sysdeps/pthread/pthread.h: Include pthread-elision.h.
* nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
(__PTHREAD_MUTEX_HAVE_ELISION): Undefine.
* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h: New
header: x86 specific lock elision support.
* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h:
(__PTHREAD_MUTEX_HAVE_ELISION): Definition moved to specific lock
elision header.
* nptl/Makefile: Add bits/pthread-elision.h to install.
---
Comments
> * nptl/sysdeps/pthread/bits/pthread-elision.h: New header: define
> default lock elision support and defines.
> * nptl/sysdeps/pthread/pthread.h: Include pthread-elision.h.
Say bits/pthread-elision.h, or since it follows the addition, just "it".
> * nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
> (__PTHREAD_MUTEX_HAVE_ELISION): Undefine.
Say "Macro removed." Saying "Undefine" implies #undef.
> * nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h: New
> header: x86 specific lock elision support.
> * nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h:
No colon at the end of the line when there are (...) sections in the entry.
> (__PTHREAD_MUTEX_HAVE_ELISION): Definition moved to specific lock
> elision header.
That's fine, but canonical style is:
* nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
(__PTHREAD_MUTEX_HAVE_ELISION): Macro moved to ...
* nptl/sysdeps/unix/sysv/linux/x86/bits/pthread-elision.h:
... this new file.
> * nptl/Makefile: Add bits/pthread-elision.h to install.
Should be:
* nptl/Makefile (headers): Add bits/pthread-elision.h.
But I'd put this right after the pthread.h log line, and just say, "Add it."
> +#ifndef _PTHREAD_H
> +# error "Never include this file directly. Use <pthread.h> instead"
> +#endif
Needs a trailing period.
> +/* Define it if the architecture supports lock elision using transactional
> + memory or a similar facility. It changes the mutex initializers to add
> + the required elision fields. Currently, three value are possible:
> + * 0: No elision support, default value.
> + * 1: Elision support for 64 bits.
> + * 2: Elision support for 32 bits. */
> +#define __PTHREAD_MUTEX_HAVE_ELISION 0
s/value/values/
I don't understand what "support for 64 bits" or "support for 32 bits"
means. OK, I've looked at bits/pthreadtypes.h so I do understand. But it
seems pretty wrong to pretend this is a generic 32/64 sort of thing when it
is really just about the x86-private layout of pthread_mutex_t. It seems
more proper to have bits/pthreadtypes.h just define __PTHREAD_SPINS.
That can be a separate cleanup if you want, and others may want to kibitz.
But that might involve dropping the header you're adding here, so maybe you
want to just resolve it now.
If you want to go ahead with this change, then it's OK with the other nits
above and this comment rewritten to describe concretely what the macro
means. In actual usage so far, it doesn't actually mean anything about
elision support per se. It just means something about how the fields of
pthread_mutex_t are structured and hence what the initializer must look like.
If that's all it's for, it should be made clear.
Thanks,
Roland
@@ -22,7 +22,7 @@ subdir := nptl
include ../Makeconfig
-headers := pthread.h semaphore.h bits/semaphore.h
+headers := pthread.h semaphore.h bits/semaphore.h bits/pthread-elision.h
extra-libs := libpthread
extra-libs-others := $(extra-libs)
new file mode 100644
@@ -0,0 +1,34 @@
+/* Lock elision default definitions.
+ Copyright (C) 2014 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _PTHREAD_ELISION_H
+#define _PTHREAD_ELISION_H 1
+
+#ifndef _PTHREAD_H
+# error "Never include this file directly. Use <pthread.h> instead"
+#endif
+
+/* Define it if the architecture supports lock elision using transactional
+ memory or a similar facility. It changes the mutex initializers to add
+ the required elision fields. Currently, three value are possible:
+ * 0: No elision support, default value.
+ * 1: Elision support for 64 bits.
+ * 2: Elision support for 32 bits. */
+#define __PTHREAD_MUTEX_HAVE_ELISION 0
+
+#endif
@@ -24,6 +24,7 @@
#include <time.h>
#include <bits/pthreadtypes.h>
+#include <bits/pthread-elision.h>
#include <bits/setjmp.h>
#include <bits/wordsize.h>
@@ -20,8 +20,6 @@
#include <bits/wordsize.h>
-# define __PTHREAD_MUTEX_HAVE_ELISION 0
-
#if __WORDSIZE == 64
# define __SIZEOF_PTHREAD_ATTR_T 56
# define __SIZEOF_PTHREAD_MUTEX_T 40
new file mode 100644
@@ -0,0 +1,32 @@
+/* x86 lock elision definitions.
+ Copyright (C) 2014 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _PTHREAD_ELISION_H
+#define _PTHREAD_ELISION_H 1
+
+#ifndef _PTHREAD_H
+# error "Never include this file directly. Use <pthread.h> instead"
+#endif
+
+#ifdef __x86_64__
+# define __PTHREAD_MUTEX_HAVE_ELISION 1
+#else
+# define __PTHREAD_MUTEX_HAVE_ELISION 2
+#endif
+
+#endif
@@ -105,7 +105,6 @@ typedef union
short __elision;
__pthread_list_t __list;
# define __PTHREAD_MUTEX_HAVE_PREV 1
-# define __PTHREAD_MUTEX_HAVE_ELISION 1
#else
unsigned int __nusers;
__extension__ union
@@ -116,7 +115,6 @@ typedef union
short __elision;
# define __spins d.__espins
# define __elision d.__elision
-# define __PTHREAD_MUTEX_HAVE_ELISION 2
} d;
__pthread_slist_t __list;
};