This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Don't use pthread initializers in constexpr constructors
ClosedPublic

Authored by elram on Jun 23 2016, 1:00 AM.

Details

Summary

Some pthread implementations use volatile types in their structs.
C++11 does not allow initializing volatile types in constexpr constructors.

This fixes building libcxx on musl-libc and NetBSD.

Diff Detail

Event Timeline

elram updated this revision to Diff 61645.Jun 23 2016, 1:00 AM
elram retitled this revision from to [libcxx] Don't use pthread initializers in constexpr constructors.
elram updated this object.
elram added a reviewer: EricWF.
elram added a subscriber: cfe-commits.

@joerg. I think this has been fixed in newer versions of NetBSD.

Sure would be nice if someone could fix this on the MUSL side of things.

See https://bugs.musicpd.org/view.php?id=4110 for fun conversations about mutex, PTHREAD_MUTEX_INIT and constexpr.

Also, the first revision of this review:
http://reviews.llvm.org/D19344

EricWF requested changes to this revision.Jun 29 2016, 11:37 PM
EricWF edited edge metadata.

C++03 does not support default member initializers but libc++ provides both of these classes an extensions in C++03.
C++03 also does not support defaulted special members.

I think the current patch looks good for C++11, but a fallback implementation is needed for C++03.

This revision now requires changes to proceed.Jun 29 2016, 11:37 PM
elram updated this revision to Diff 62339.Jun 30 2016, 12:57 AM
elram edited edge metadata.
EricWF accepted this revision.Jun 30 2016, 2:42 AM
EricWF edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jun 30 2016, 2:42 AM

Small side question: so how are __m_ and __cv_ initialized in this setup? is it that pthread_mutex_lock() and the like would be able to handle an un-initialized blob of memory (in a thread-safe way)? Because I don't see a call to pthread_mutex_init() anywhere in the code-path.

Small side question: so how are __m_ and __cv_ initialized in this setup? is it that pthread_mutex_lock() and the like would be able to handle an un-initialized blob of memory (in a thread-safe way)? Because I don't see a call to pthread_mutex_init() anywhere in the code-path.

__m_ and __cv_ are initialized during non-static data member initialization. They didn't previously call pthread_mutex_lock(), instead they directly initialized 'm_' and 'cv_' in the constructors initializer list.

And according to the current spec we are allowed to use PTHREAD_MUTEX_INITIALIZER to initialize non-static mutexes.
http://pubs.opengroup.org/onlinepubs/9699919799/

In cases where default mutex attributes are appropriate, the macro PTHREAD_MUTEX_INITIALIZER can be used to initialize mutexes.
The effect shall be equivalent to dynamic initialization by a call to pthread_mutex_init() with parameter attr specified as NULL, except that no error checks are performed.

Small side question: so how are __m_ and __cv_ initialized in this setup? is it that pthread_mutex_lock() and the like would be able to handle an un-initialized blob of memory (in a thread-safe way)? Because I don't see a call to pthread_mutex_init() anywhere in the code-path.

__m_ and __cv_ are initialized during non-static data member initialization. They didn't previously call pthread_mutex_lock(), instead they directly initialized 'm_' and 'cv_' in the constructors initializer list.

And according to the current spec we are allowed to use PTHREAD_MUTEX_INITIALIZER to initialize non-static mutexes.
http://pubs.opengroup.org/onlinepubs/9699919799/

In cases where default mutex attributes are appropriate, the macro PTHREAD_MUTEX_INITIALIZER can be used to initialize mutexes.
The effect shall be equivalent to dynamic initialization by a call to pthread_mutex_init() with parameter attr specified as NULL, except that no error checks are performed.

Ah! I've misread the patch - missed the assignment in the constructor (for the _LIBCPP_HAS_NO_CONSTEXPR case). Sorted now.

Thanks.

/ Asiri

I don't have commit access.
Can someone do it for me please?

mclow.lists closed this revision.Jul 18 2016, 10:35 AM
mclow.lists edited edge metadata.

Committed as revision 275819.