Define __PTHREAD_MUTEX_HAVE_PREV only if undefined [BZ #22298]
Commit Message
On 15/10/2017 13:11, H.J. Lu wrote:
> It is incorrect to define __PTHREAD_MUTEX_HAVE_PREV to 1 only when
> __WORDSIZE == 64. For x32, __PTHREAD_MUTEX_HAVE_PREV should be 1, but
> it has __WORDSIZE == 32. This patch defines __PTHREAD_MUTEX_HAVE_PREV
> based on __WORDSIZE only if it is undefined. __PTHREAD_MUTEX_HAVE_PREV
> check is changed from "#ifdef" to "#if" to support values of 0 or 1.
>
> OK for master and 2.26 branch?
Thanks for checking and I think changing '#ifdef' to '#if' is a good
way forward, however instead of check if __PTHREAD_MUTEX_HAVE_PREV is
define to redefine is based on __WORDSIZE, I think it would be better
if we define it by arch-bases as other __PTHREAD_* defines.
Based on your patch, I rechecked all the architectures and the expected
__PTHREAD_MUTEX_HAVE_PREV and move its definition to pthreadtypes-arch.h.
What do you think about the below:
---
Comments
On Okt 16 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
> index 68b82b6..5dfe01e 100644
> --- a/sysdeps/nptl/bits/thread-shared-types.h
> +++ b/sysdeps/nptl/bits/thread-shared-types.h
> @@ -58,8 +58,7 @@
> #include <bits/pthreadtypes-arch.h>
>
> /* Common definition of pthread_mutex_t. */
> -
> -#if __WORDSIZE == 64
> +#if __PTHREAD_MUTEX_HAVE_PREV
> typedef struct __pthread_internal_list
> {
> struct __pthread_internal_list *__prev;
> @@ -74,7 +73,7 @@ typedef struct __pthread_internal_slist
>
> /* Lock elision support. */
> #if __PTHREAD_MUTEX_LOCK_ELISION
> -# if __WORDSIZE == 64
> +# if __PTHREAD_MUTEX_HAVE_PREV
> # define __PTHREAD_SPINS_DATA \
> short __spins; \
> short __elision
> @@ -101,17 +100,16 @@ struct __pthread_mutex_s
> int __lock __LOCK_ALIGNMENT;
> unsigned int __count;
> int __owner;
> -#if __WORDSIZE == 64
> +#if __PTHREAD_MUTEX_HAVE_PREV
> unsigned int __nusers;
> #endif
The name doesn't really fit here. There is nothing matching prev in
__nusers or __PTHREAD_SPINS_DATA.
Andreas.
On Mon, Oct 16, 2017 at 7:31 AM, Andreas Schwab <schwab@suse.de> wrote:
> On Okt 16 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
>> index 68b82b6..5dfe01e 100644
>> --- a/sysdeps/nptl/bits/thread-shared-types.h
>> +++ b/sysdeps/nptl/bits/thread-shared-types.h
>> @@ -58,8 +58,7 @@
>> #include <bits/pthreadtypes-arch.h>
>>
>> /* Common definition of pthread_mutex_t. */
>> -
>> -#if __WORDSIZE == 64
>> +#if __PTHREAD_MUTEX_HAVE_PREV
>> typedef struct __pthread_internal_list
>> {
>> struct __pthread_internal_list *__prev;
>> @@ -74,7 +73,7 @@ typedef struct __pthread_internal_slist
>>
>> /* Lock elision support. */
>> #if __PTHREAD_MUTEX_LOCK_ELISION
>> -# if __WORDSIZE == 64
>> +# if __PTHREAD_MUTEX_HAVE_PREV
>> # define __PTHREAD_SPINS_DATA \
>> short __spins; \
>> short __elision
>> @@ -101,17 +100,16 @@ struct __pthread_mutex_s
>> int __lock __LOCK_ALIGNMENT;
>> unsigned int __count;
>> int __owner;
>> -#if __WORDSIZE == 64
>> +#if __PTHREAD_MUTEX_HAVE_PREV
>> unsigned int __nusers;
>> #endif
>
> The name doesn't really fit here. There is nothing matching prev in
> __nusers or __PTHREAD_SPINS_DATA.
>
True. But they are tied together.
On Mon, Oct 16, 2017 at 6:50 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> On 15/10/2017 13:11, H.J. Lu wrote:
>> It is incorrect to define __PTHREAD_MUTEX_HAVE_PREV to 1 only when
>> __WORDSIZE == 64. For x32, __PTHREAD_MUTEX_HAVE_PREV should be 1, but
>> it has __WORDSIZE == 32. This patch defines __PTHREAD_MUTEX_HAVE_PREV
>> based on __WORDSIZE only if it is undefined. __PTHREAD_MUTEX_HAVE_PREV
>> check is changed from "#ifdef" to "#if" to support values of 0 or 1.
>>
>> OK for master and 2.26 branch?
>
> Thanks for checking and I think changing '#ifdef' to '#if' is a good
> way forward, however instead of check if __PTHREAD_MUTEX_HAVE_PREV is
> define to redefine is based on __WORDSIZE, I think it would be better
> if we define it by arch-bases as other __PTHREAD_* defines.
>
> Based on your patch, I rechecked all the architectures and the expected
> __PTHREAD_MUTEX_HAVE_PREV and move its definition to pthreadtypes-arch.h.
> What do you think about the below:
See my comments below.
> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
> index 68b82b6..5dfe01e 100644
> --- a/sysdeps/nptl/bits/thread-shared-types.h
> +++ b/sysdeps/nptl/bits/thread-shared-types.h
> @@ -58,8 +58,7 @@
> #include <bits/pthreadtypes-arch.h>
>
> /* Common definition of pthread_mutex_t. */
> -
> -#if __WORDSIZE == 64
> +#if __PTHREAD_MUTEX_HAVE_PREV
Do we need to issue an error if __PTHREAD_MUTEX_HAVE_PREV
isn't defined?
> diff --git a/sysdeps/x86/nptl/bits/pthreadtypes-arch.h b/sysdeps/x86/nptl/bits/pthreadtypes-arch.h
> index fd86806..3592667 100644
> --- a/sysdeps/x86/nptl/bits/pthreadtypes-arch.h
> +++ b/sysdeps/x86/nptl/bits/pthreadtypes-arch.h
> @@ -35,6 +35,7 @@
> # define __SIZEOF_PTHREAD_BARRIER_T 20
> # endif
> #else
> +# define __PTHREAD_MUTEX_HAVE_PREV 0
^^^^^^^^^^^^^^^^^^^^^^^ No need for it.
> # define __SIZEOF_PTHREAD_MUTEX_T 24
> # define __SIZEOF_PTHREAD_ATTR_T 36
> # define __SIZEOF_PTHREAD_MUTEX_T 24
> @@ -51,6 +52,11 @@
> #define __PTHREAD_COMPAT_PADDING_MID
> #define __PTHREAD_COMPAT_PADDING_END
> #define __PTHREAD_MUTEX_LOCK_ELISION 1
> +#ifdef __x86_64__
> +# define __PTHREAD_MUTEX_HAVE_PREV 1
> +#else
> +# define __PTHREAD_MUTEX_HAVE_PREV 0
> +#endif
>
> #define __LOCK_ALIGNMENT
> #define __ONCE_ALIGNMENT
On 16/10/2017 13:00, H.J. Lu wrote:
> On Mon, Oct 16, 2017 at 6:50 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>> On 15/10/2017 13:11, H.J. Lu wrote:
>>> It is incorrect to define __PTHREAD_MUTEX_HAVE_PREV to 1 only when
>>> __WORDSIZE == 64. For x32, __PTHREAD_MUTEX_HAVE_PREV should be 1, but
>>> it has __WORDSIZE == 32. This patch defines __PTHREAD_MUTEX_HAVE_PREV
>>> based on __WORDSIZE only if it is undefined. __PTHREAD_MUTEX_HAVE_PREV
>>> check is changed from "#ifdef" to "#if" to support values of 0 or 1.
>>>
>>> OK for master and 2.26 branch?
>>
>> Thanks for checking and I think changing '#ifdef' to '#if' is a good
>> way forward, however instead of check if __PTHREAD_MUTEX_HAVE_PREV is
>> define to redefine is based on __WORDSIZE, I think it would be better
>> if we define it by arch-bases as other __PTHREAD_* defines.
>>
>> Based on your patch, I rechecked all the architectures and the expected
>> __PTHREAD_MUTEX_HAVE_PREV and move its definition to pthreadtypes-arch.h.
>> What do you think about the below:
>
> See my comments below.
>
>
>> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
>> index 68b82b6..5dfe01e 100644
>> --- a/sysdeps/nptl/bits/thread-shared-types.h
>> +++ b/sysdeps/nptl/bits/thread-shared-types.h
>> @@ -58,8 +58,7 @@
>> #include <bits/pthreadtypes-arch.h>
>>
>> /* Common definition of pthread_mutex_t. */
>> -
>> -#if __WORDSIZE == 64
>> +#if __PTHREAD_MUTEX_HAVE_PREV
>
> Do we need to issue an error if __PTHREAD_MUTEX_HAVE_PREV
> isn't defined?
>
I think the build error is enough to indicate the architecture should
define it.
>
>> diff --git a/sysdeps/x86/nptl/bits/pthreadtypes-arch.h b/sysdeps/x86/nptl/bits/pthreadtypes-arch.h
>> index fd86806..3592667 100644
>> --- a/sysdeps/x86/nptl/bits/pthreadtypes-arch.h
>> +++ b/sysdeps/x86/nptl/bits/pthreadtypes-arch.h
>> @@ -35,6 +35,7 @@
>> # define __SIZEOF_PTHREAD_BARRIER_T 20
>> # endif
>> #else
>> +# define __PTHREAD_MUTEX_HAVE_PREV 0
> ^^^^^^^^^^^^^^^^^^^^^^^ No need for it.
>
>> # define __SIZEOF_PTHREAD_MUTEX_T 24
>> # define __SIZEOF_PTHREAD_ATTR_T 36
>> # define __SIZEOF_PTHREAD_MUTEX_T 24
>> @@ -51,6 +52,11 @@
>> #define __PTHREAD_COMPAT_PADDING_MID
>> #define __PTHREAD_COMPAT_PADDING_END
>> #define __PTHREAD_MUTEX_LOCK_ELISION 1
>> +#ifdef __x86_64__
>> +# define __PTHREAD_MUTEX_HAVE_PREV 1
>> +#else
>> +# define __PTHREAD_MUTEX_HAVE_PREV 0
>> +#endif
>>
>> #define __LOCK_ALIGNMENT
>> #define __ONCE_ALIGNMENT
Ack.
On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> On Mon, Oct 16, 2017 at 7:31 AM, Andreas Schwab <schwab@suse.de> wrote:
>> On Okt 16 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>
>>> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
>>> index 68b82b6..5dfe01e 100644
>>> --- a/sysdeps/nptl/bits/thread-shared-types.h
>>> +++ b/sysdeps/nptl/bits/thread-shared-types.h
>>> @@ -58,8 +58,7 @@
>>> #include <bits/pthreadtypes-arch.h>
>>>
>>> /* Common definition of pthread_mutex_t. */
>>> -
>>> -#if __WORDSIZE == 64
>>> +#if __PTHREAD_MUTEX_HAVE_PREV
>>> typedef struct __pthread_internal_list
>>> {
>>> struct __pthread_internal_list *__prev;
>>> @@ -74,7 +73,7 @@ typedef struct __pthread_internal_slist
>>>
>>> /* Lock elision support. */
>>> #if __PTHREAD_MUTEX_LOCK_ELISION
>>> -# if __WORDSIZE == 64
>>> +# if __PTHREAD_MUTEX_HAVE_PREV
>>> # define __PTHREAD_SPINS_DATA \
>>> short __spins; \
>>> short __elision
>>> @@ -101,17 +100,16 @@ struct __pthread_mutex_s
>>> int __lock __LOCK_ALIGNMENT;
>>> unsigned int __count;
>>> int __owner;
>>> -#if __WORDSIZE == 64
>>> +#if __PTHREAD_MUTEX_HAVE_PREV
>>> unsigned int __nusers;
>>> #endif
>>
>> The name doesn't really fit here. There is nothing matching prev in
>> __nusers or __PTHREAD_SPINS_DATA.
>>
>
> True. But they are tied together.
Are they? They look rather unrelated, or only related by chance.
Andreas.
On Mon, Oct 16, 2017 at 8:25 AM, Andreas Schwab <schwab@suse.de> wrote:
> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>> On Mon, Oct 16, 2017 at 7:31 AM, Andreas Schwab <schwab@suse.de> wrote:
>>> On Okt 16 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>>
>>>> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
>>>> index 68b82b6..5dfe01e 100644
>>>> --- a/sysdeps/nptl/bits/thread-shared-types.h
>>>> +++ b/sysdeps/nptl/bits/thread-shared-types.h
>>>> @@ -58,8 +58,7 @@
>>>> #include <bits/pthreadtypes-arch.h>
>>>>
>>>> /* Common definition of pthread_mutex_t. */
>>>> -
>>>> -#if __WORDSIZE == 64
>>>> +#if __PTHREAD_MUTEX_HAVE_PREV
>>>> typedef struct __pthread_internal_list
>>>> {
>>>> struct __pthread_internal_list *__prev;
>>>> @@ -74,7 +73,7 @@ typedef struct __pthread_internal_slist
>>>>
>>>> /* Lock elision support. */
>>>> #if __PTHREAD_MUTEX_LOCK_ELISION
>>>> -# if __WORDSIZE == 64
>>>> +# if __PTHREAD_MUTEX_HAVE_PREV
>>>> # define __PTHREAD_SPINS_DATA \
>>>> short __spins; \
>>>> short __elision
>>>> @@ -101,17 +100,16 @@ struct __pthread_mutex_s
>>>> int __lock __LOCK_ALIGNMENT;
>>>> unsigned int __count;
>>>> int __owner;
>>>> -#if __WORDSIZE == 64
>>>> +#if __PTHREAD_MUTEX_HAVE_PREV
>>>> unsigned int __nusers;
>>>> #endif
>>>
>>> The name doesn't really fit here. There is nothing matching prev in
>>> __nusers or __PTHREAD_SPINS_DATA.
>>>
>>
>> True. But they are tied together.
>
> Are they? They look rather unrelated, or only related by chance.
>
> Andreas.
>
Glibc 2.25 has
#ifdef __x86_64__
typedef struct __pthread_internal_list
{
struct __pthread_internal_list *__prev;
struct __pthread_internal_list *__next;
} __pthread_list_t;
#else
typedef struct __pthread_internal_slist
{
struct __pthread_internal_slist *__next;
} __pthread_slist_t;
#endif
/* Data structures for mutex handling. The structure of the attribute
type is not exposed on purpose. */
typedef union
{
struct __pthread_mutex_s
{
int __lock;
unsigned int __count;
int __owner;
#ifdef __x86_64__
unsigned int __nusers;
#endif
/* KIND must stay at this position in the structure to maintain
binary compatibility with static initializers. */
int __kind;
#ifdef __x86_64__
short __spins;
short __elision;
__pthread_list_t __list;
# define __PTHREAD_MUTEX_HAVE_PREV 1
/* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */
# define __PTHREAD_SPINS 0, 0
#else
unsigned int __nusers;
__extension__ union
{
struct
{
short __espins;
short __elision;
# define __spins __elision_data.__espins
# define __elision __elision_data.__elision
# define __PTHREAD_SPINS { 0, 0 }
} __elision_data;
__pthread_slist_t __list;
};
#endif
} __data;
char __size[__SIZEOF_PTHREAD_MUTEX_T];
long int __align;
} pthread_mutex_t;
Here __x86_64__ == __PTHREAD_MUTEX_HAVE_PREV, which ties
__PTHREAD_MUTEX_HAVE_PREV with __nusers and __PTHREAD_SPINS.
On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> Here __x86_64__ == __PTHREAD_MUTEX_HAVE_PREV, which ties
> __PTHREAD_MUTEX_HAVE_PREV with __nusers and __PTHREAD_SPINS.
That's what I would call related by chance. The position of __nusers
differs only because __kind must not be moved, and that difference
existed already before the introduction of __prev and __next.
Andreas.
On Mon, Oct 16, 2017 at 9:05 AM, Andreas Schwab <schwab@suse.de> wrote:
> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>> Here __x86_64__ == __PTHREAD_MUTEX_HAVE_PREV, which ties
>> __PTHREAD_MUTEX_HAVE_PREV with __nusers and __PTHREAD_SPINS.
>
> That's what I would call related by chance. The position of __nusers
> differs only because __kind must not be moved, and that difference
> existed already before the introduction of __prev and __next.
>
By chance or not, they can't be changed on x86.
On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> On Mon, Oct 16, 2017 at 9:05 AM, Andreas Schwab <schwab@suse.de> wrote:
>> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>
>>> Here __x86_64__ == __PTHREAD_MUTEX_HAVE_PREV, which ties
>>> __PTHREAD_MUTEX_HAVE_PREV with __nusers and __PTHREAD_SPINS.
>>
>> That's what I would call related by chance. The position of __nusers
>> differs only because __kind must not be moved, and that difference
>> existed already before the introduction of __prev and __next.
>>
>
> By chance or not, they can't be changed on x86.
This file is used by all architectures.
Andreas.
On Mon, Oct 16, 2017 at 10:39 AM, Andreas Schwab <schwab@suse.de> wrote:
> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>> On Mon, Oct 16, 2017 at 9:05 AM, Andreas Schwab <schwab@suse.de> wrote:
>>> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>
>>>> Here __x86_64__ == __PTHREAD_MUTEX_HAVE_PREV, which ties
>>>> __PTHREAD_MUTEX_HAVE_PREV with __nusers and __PTHREAD_SPINS.
>>>
>>> That's what I would call related by chance. The position of __nusers
>>> differs only because __kind must not be moved, and that difference
>>> existed already before the introduction of __prev and __next.
>>>
>>
>> By chance or not, they can't be changed on x86.
>
> This file is used by all architectures.
>
> Andreas.
>
The current code has
/* Common definition of pthread_mutex_t. */
#if __WORDSIZE == 64
typedef struct __pthread_internal_list
{
struct __pthread_internal_list *__prev;
struct __pthread_internal_list *__next;
} __pthread_list_t;
#else
typedef struct __pthread_internal_slist
{
struct __pthread_internal_slist *__next;
} __pthread_slist_t;
#endif
/* Lock elision support. */
#if __PTHREAD_MUTEX_LOCK_ELISION
# if __WORDSIZE == 64
# define __PTHREAD_SPINS_DATA \
short __spins; \
short __elision
# define __PTHREAD_SPINS 0, 0
# else
# define __PTHREAD_SPINS_DATA \
struct \
{ \
short __espins; \
short __eelision; \
} __elision_data
# define __PTHREAD_SPINS { 0, 0 }
# define __spins __elision_data.__espins
# define __elision __elision_data.__eelision
# endif
#else
# define __PTHREAD_SPINS_DATA int __spins
/* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */
# define __PTHREAD_SPINS 0
#endif
struct __pthread_mutex_s
{
int __lock __LOCK_ALIGNMENT;
unsigned int __count;
int __owner;
#if __WORDSIZE == 64
unsigned int __nusers;
#endif
/* KIND must stay at this position in the structure to maintain
binary compatibility with static initializers. */
int __kind;
__PTHREAD_COMPAT_PADDING_MID
#if __WORDSIZE == 64
__PTHREAD_SPINS_DATA;
__pthread_list_t __list;
# define __PTHREAD_MUTEX_HAVE_PREV 1
#else
unsigned int __nusers;
__extension__ union
{
__PTHREAD_SPINS_DATA;
__pthread_slist_t __list;
};
#endif
__PTHREAD_COMPAT_PADDING_END
};
__PTHREAD_MUTEX_HAVE_PREV is the same as __WORDSIZE == 64,
which ties it together with __nusers and __PTHREAD_SPINS_DATA. We
can't change it for existing targets.
On 16/10/2017 16:00, H.J. Lu wrote:
> On Mon, Oct 16, 2017 at 10:39 AM, Andreas Schwab <schwab@suse.de> wrote:
>> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>
>>> On Mon, Oct 16, 2017 at 9:05 AM, Andreas Schwab <schwab@suse.de> wrote:
>>>> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>>
>>>>> Here __x86_64__ == __PTHREAD_MUTEX_HAVE_PREV, which ties
>>>>> __PTHREAD_MUTEX_HAVE_PREV with __nusers and __PTHREAD_SPINS.
>>>>
>>>> That's what I would call related by chance. The position of __nusers
>>>> differs only because __kind must not be moved, and that difference
>>>> existed already before the introduction of __prev and __next.
>>>>
>>>
>>> By chance or not, they can't be changed on x86.
>>
>> This file is used by all architectures.
>>
>> Andreas.
>>
>
> The current code has
>
> /* Common definition of pthread_mutex_t. */
>
> #if __WORDSIZE == 64
> typedef struct __pthread_internal_list
> {
> struct __pthread_internal_list *__prev;
> struct __pthread_internal_list *__next;
> } __pthread_list_t;
> #else
> typedef struct __pthread_internal_slist
> {
> struct __pthread_internal_slist *__next;
> } __pthread_slist_t;
> #endif
>
> /* Lock elision support. */
> #if __PTHREAD_MUTEX_LOCK_ELISION
> # if __WORDSIZE == 64
> # define __PTHREAD_SPINS_DATA \
> short __spins; \
> short __elision
> # define __PTHREAD_SPINS 0, 0
> # else
> # define __PTHREAD_SPINS_DATA \
> struct \
> { \
> short __espins; \
> short __eelision; \
> } __elision_data
> # define __PTHREAD_SPINS { 0, 0 }
> # define __spins __elision_data.__espins
> # define __elision __elision_data.__eelision
> # endif
> #else
> # define __PTHREAD_SPINS_DATA int __spins
> /* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */
> # define __PTHREAD_SPINS 0
> #endif
>
> struct __pthread_mutex_s
> {
> int __lock __LOCK_ALIGNMENT;
> unsigned int __count;
> int __owner;
> #if __WORDSIZE == 64
> unsigned int __nusers;
> #endif
> /* KIND must stay at this position in the structure to maintain
> binary compatibility with static initializers. */
> int __kind;
> __PTHREAD_COMPAT_PADDING_MID
> #if __WORDSIZE == 64
> __PTHREAD_SPINS_DATA;
> __pthread_list_t __list;
> # define __PTHREAD_MUTEX_HAVE_PREV 1
> #else
> unsigned int __nusers;
> __extension__ union
> {
> __PTHREAD_SPINS_DATA;
> __pthread_slist_t __list;
> };
> #endif
> __PTHREAD_COMPAT_PADDING_END
> };
>
> __PTHREAD_MUTEX_HAVE_PREV is the same as __WORDSIZE == 64,
> which ties it together with __nusers and __PTHREAD_SPINS_DATA. We
> can't change it for existing targets.
I agree, in the end __PTHREAD_MUTEX_HAVE_PREV will define which internal
layout __pthread_mutex_s will use to place __nusers and __list. We can
add an extra architecture define to set the __nusers/__list layout along
with __PTHREAD_MUTEX_HAVE_PREV, however this will only add complexity.
I plan to send an update version of this patch and I think it worth add
a comment about __PTHREAD_MUTEX_HAVE_PREV and the internal layout coupling.
On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> __PTHREAD_MUTEX_HAVE_PREV is the same as __WORDSIZE == 64,
Just because it happens to match now does not mean that all new
architectures want to follow the idiosyncrasies of linuxthreads
compatibility.
Andreas.
On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> By chance or not, they can't be changed on x86.
Who said that x86 must change??
Andreas.
On Tue, Oct 17, 2017 at 12:20 AM, Andreas Schwab <schwab@suse.de> wrote:
> On Okt 16 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>> __PTHREAD_MUTEX_HAVE_PREV is the same as __WORDSIZE == 64,
>
> Just because it happens to match now does not mean that all new
> architectures want to follow the idiosyncrasies of linuxthreads
> compatibility.
>
1. We first add a test to check offsets of all fields in struct
__pthread_mutex_s.
2. Define __PTHREAD_SPINS_DATA_IN_STRUCT to control
__PTHREAD_SPINS_DATA
3. __PTHREAD_MUTEX_NUSERS_BEFORE_KIND to control where to
put __nusers.
On Okt 17 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> 2. Define __PTHREAD_SPINS_DATA_IN_STRUCT to control
> __PTHREAD_SPINS_DATA
> 3. __PTHREAD_MUTEX_NUSERS_BEFORE_KIND to control where to
> put __nusers.
Yes, this looks like the right approach. Though I would define the
macros in a way so that defining them to 0 results in the preferred
layout for future architectures (thus __PTHREAD_MUTEX_NUSERS_AFTER_KIND
instead of __PTHREAD_MUTEX_NUSERS_BEFORE_KIND).
__PTHREAD_SPINS_DATA_IN_STRUCT could then imply
__PTHREAD_MUTEX_HAVE_PREV as before, since both are only needed due to
space constraints for linuxthreads compatibility (perhaps rename it to
__PTHREAD_MUTEX_USE_UNION?).
Andreas.
On 18/10/2017 06:21, Andreas Schwab wrote:
> On Okt 17 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>> 2. Define __PTHREAD_SPINS_DATA_IN_STRUCT to control
>> __PTHREAD_SPINS_DATA
>> 3. __PTHREAD_MUTEX_NUSERS_BEFORE_KIND to control where to
>> put __nusers.
>
> Yes, this looks like the right approach. Though I would define the
> macros in a way so that defining them to 0 results in the preferred
> layout for future architectures (thus __PTHREAD_MUTEX_NUSERS_AFTER_KIND
> instead of __PTHREAD_MUTEX_NUSERS_BEFORE_KIND).
> __PTHREAD_SPINS_DATA_IN_STRUCT could then imply
> __PTHREAD_MUTEX_HAVE_PREV as before, since both are only needed due to
> space constraints for linuxthreads compatibility (perhaps rename it to
> __PTHREAD_MUTEX_USE_UNION?).
>
> Andreas.
>
Alright, I will adapt my first submission to follow this.
On Wed, Oct 18, 2017 at 4:06 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> On 18/10/2017 06:21, Andreas Schwab wrote:
>> On Okt 17 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>
>>> 2. Define __PTHREAD_SPINS_DATA_IN_STRUCT to control
>>> __PTHREAD_SPINS_DATA
>>> 3. __PTHREAD_MUTEX_NUSERS_BEFORE_KIND to control where to
>>> put __nusers.
>>
>> Yes, this looks like the right approach. Though I would define the
>> macros in a way so that defining them to 0 results in the preferred
>> layout for future architectures (thus __PTHREAD_MUTEX_NUSERS_AFTER_KIND
>> instead of __PTHREAD_MUTEX_NUSERS_BEFORE_KIND).
>> __PTHREAD_SPINS_DATA_IN_STRUCT could then imply
>> __PTHREAD_MUTEX_HAVE_PREV as before, since both are only needed due to
>> space constraints for linuxthreads compatibility (perhaps rename it to
>> __PTHREAD_MUTEX_USE_UNION?).
>>
>> Andreas.
>>
>
> Alright, I will adapt my first submission to follow this.
Please use a separate patch to check offsets which should be backported
to 2.25 branch.
On 18/10/2017 10:35, H.J. Lu wrote:
> On Wed, Oct 18, 2017 at 4:06 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>> On 18/10/2017 06:21, Andreas Schwab wrote:
>>> On Okt 17 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>
>>>> 2. Define __PTHREAD_SPINS_DATA_IN_STRUCT to control
>>>> __PTHREAD_SPINS_DATA
>>>> 3. __PTHREAD_MUTEX_NUSERS_BEFORE_KIND to control where to
>>>> put __nusers.
>>>
>>> Yes, this looks like the right approach. Though I would define the
>>> macros in a way so that defining them to 0 results in the preferred
>>> layout for future architectures (thus __PTHREAD_MUTEX_NUSERS_AFTER_KIND
>>> instead of __PTHREAD_MUTEX_NUSERS_BEFORE_KIND).
>>> __PTHREAD_SPINS_DATA_IN_STRUCT could then imply
>>> __PTHREAD_MUTEX_HAVE_PREV as before, since both are only needed due to
>>> space constraints for linuxthreads compatibility (perhaps rename it to
>>> __PTHREAD_MUTEX_USE_UNION?).
>>>
>>> Andreas.
>>>
>>
>> Alright, I will adapt my first submission to follow this.
>
> Please use a separate patch to check offsets which should be backported
> to 2.25 branch.
>
Yes, My idea is to follow your suggestion:
1. One patch to check offsets of all relevant fields in struct
__pthread_mutex_s.
2. Another patch to define __PTHREAD_SPINS_DATA_IN_STRUCT to control
__PTHREAD_SPINS_DATA
3. Another to define __PTHREAD_MUTEX_NUSERS_BEFORE_KIND to control where to
put __nusers (not sure if it worth to bind 2. and 3. together).
@@ -753,7 +753,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
- offsetof (pthread_mutex_t,
__data.__list.__next));
pd->robust_head.list_op_pending = NULL;
-#ifdef __PTHREAD_MUTEX_HAVE_PREV
+#if __PTHREAD_MUTEX_HAVE_PREV
pd->robust_prev = &pd->robust_head;
#endif
pd->robust_head.list = &pd->robust_head;
@@ -169,7 +169,7 @@ struct pthread
pid_t pid_ununsed;
/* List of robust mutexes the thread is holding. */
-#ifdef __PTHREAD_MUTEX_HAVE_PREV
+#if __PTHREAD_MUTEX_HAVE_PREV
void *robust_prev;
struct robust_list_head robust_head;
@@ -297,7 +297,7 @@ __pthread_initialize_minimal_internal (void)
/* Initialize the robust mutex data. */
{
-#ifdef __PTHREAD_MUTEX_HAVE_PREV
+#if __PTHREAD_MUTEX_HAVE_PREV
pd->robust_prev = &pd->robust_head;
#endif
pd->robust_head.list = &pd->robust_head;
@@ -518,7 +518,7 @@ START_THREAD_DEFN
#ifndef __ASSUME_SET_ROBUST_LIST
/* If this thread has any robust mutexes locked, handle them now. */
-# ifdef __PTHREAD_MUTEX_HAVE_PREV
+# if __PTHREAD_MUTEX_HAVE_PREV
void *robust = pd->robust_head.list;
# else
__pthread_slist_t *robust = pd->robust_list.__next;
@@ -536,7 +536,7 @@ START_THREAD_DEFN
__list.__next));
robust = *((void **) robust);
-# ifdef __PTHREAD_MUTEX_HAVE_PREV
+# if __PTHREAD_MUTEX_HAVE_PREV
this->__list.__prev = NULL;
# endif
this->__list.__next = NULL;
@@ -45,6 +45,11 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#ifdef __ILP32__
+#define __PTHREAD_MUTEX_HAVE_PREV 0
+#else
+#define __PTHREAD_MUTEX_HAVE_PREV 1
+#endif
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -33,6 +33,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 1
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -34,6 +34,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 0
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -48,6 +48,7 @@
pthread_mutex_t is larger than Linuxthreads. */
#define __PTHREAD_COMPAT_PADDING_END int __reserved[2];
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 0
#define __LOCK_ALIGNMENT __attribute__ ((__aligned__(16)))
#define __ONCE_ALIGNMENT
@@ -33,6 +33,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 1
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -35,6 +35,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 0
#define __LOCK_ALIGNMENT __attribute__ ((__aligned__ (4)))
#define __ONCE_ALIGNMENT __attribute__ ((__aligned__ (4)))
@@ -35,6 +35,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 0
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -42,6 +42,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV (_MIPS_SIM == _ABI64)
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -35,6 +35,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 0
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -58,8 +58,7 @@
#include <bits/pthreadtypes-arch.h>
/* Common definition of pthread_mutex_t. */
-
-#if __WORDSIZE == 64
+#if __PTHREAD_MUTEX_HAVE_PREV
typedef struct __pthread_internal_list
{
struct __pthread_internal_list *__prev;
@@ -74,7 +73,7 @@ typedef struct __pthread_internal_slist
/* Lock elision support. */
#if __PTHREAD_MUTEX_LOCK_ELISION
-# if __WORDSIZE == 64
+# if __PTHREAD_MUTEX_HAVE_PREV
# define __PTHREAD_SPINS_DATA \
short __spins; \
short __elision
@@ -101,17 +100,16 @@ struct __pthread_mutex_s
int __lock __LOCK_ALIGNMENT;
unsigned int __count;
int __owner;
-#if __WORDSIZE == 64
+#if __PTHREAD_MUTEX_HAVE_PREV
unsigned int __nusers;
#endif
/* KIND must stay at this position in the structure to maintain
binary compatibility with static initializers. */
int __kind;
__PTHREAD_COMPAT_PADDING_MID
-#if __WORDSIZE == 64
+#if __PTHREAD_MUTEX_HAVE_PREV
__PTHREAD_SPINS_DATA;
__pthread_list_t __list;
-# define __PTHREAD_MUTEX_HAVE_PREV 1
#else
unsigned int __nusers;
__extension__ union
@@ -166,7 +166,7 @@ __libc_fork (void)
inherit the correct value from the parent. We do not need to clear
the pending operation because it must have been zero when fork was
called. */
-# ifdef __PTHREAD_MUTEX_HAVE_PREV
+# if __PTHREAD_MUTEX_HAVE_PREV
self->robust_prev = &self->robust_head;
# endif
self->robust_head.list = &self->robust_head;
@@ -83,7 +83,7 @@ enum
#endif
-#ifdef __PTHREAD_MUTEX_HAVE_PREV
+#if __PTHREAD_MUTEX_HAVE_PREV
# define PTHREAD_MUTEX_INITIALIZER \
{ { 0, 0, 0, 0, 0, __PTHREAD_SPINS, { 0, 0 } } }
# ifdef __USE_GNU
@@ -42,6 +42,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 1
+#define __PTHREAD_MUTEX_HAVE_PREV (__WORDSIZE == 64)
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -45,6 +45,7 @@
#else
#define __PTHREAD_MUTEX_LOCK_ELISION 0
#endif
+#define __PTHREAD_MUTEX_HAVE_PREV (__WORDSIZE == 64)
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -34,6 +34,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV 0
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -43,6 +43,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV (__WORDSIZE == 64)
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -43,6 +43,7 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 0
+#define __PTHREAD_MUTEX_HAVE_PREV (__WORDSIZE == 64)
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT
@@ -35,6 +35,7 @@
# define __SIZEOF_PTHREAD_BARRIER_T 20
# endif
#else
+# define __PTHREAD_MUTEX_HAVE_PREV 0
# define __SIZEOF_PTHREAD_MUTEX_T 24
# define __SIZEOF_PTHREAD_ATTR_T 36
# define __SIZEOF_PTHREAD_MUTEX_T 24
@@ -51,6 +52,11 @@
#define __PTHREAD_COMPAT_PADDING_MID
#define __PTHREAD_COMPAT_PADDING_END
#define __PTHREAD_MUTEX_LOCK_ELISION 1
+#ifdef __x86_64__
+# define __PTHREAD_MUTEX_HAVE_PREV 1
+#else
+# define __PTHREAD_MUTEX_HAVE_PREV 0
+#endif
#define __LOCK_ALIGNMENT
#define __ONCE_ALIGNMENT