After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659866 - pthread_rwlock_t requires defined __USE_UNIX98 || defined __USE_XOPEN2K
pthread_rwlock_t requires defined __USE_UNIX98 || defined __USE_XOPEN2K
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 660561 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2011-09-22 19:58 UTC by Colin Walters
Modified: 2011-10-03 02:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GThread posix: switch to Windows ABI (11.13 KB, patch)
2011-10-03 01:32 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Re-enable 'include' testcase (829 bytes, patch)
2011-10-03 01:32 UTC, Allison Karlitskaya (desrt)
committed Details | Review
locks: drop _INIT macros (18.28 KB, patch)
2011-10-03 01:32 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Colin Walters 2011-09-22 19:58:37 UTC
On glibc 2.14 at least, we have in pthreadtypes.h:

#if defined __USE_UNIX98 || defined __USE_XOPEN2K
/* Data structure for read-write lock variable handling.  The
   structure of the attribute type is not exposed on purpose.  */
typedef union
{
...
} pthread_rwlock_t;
#endif

This means that anyone using G_RW_LOCK in their program needs a #define that will enable this, like _XOPEN_SOURCE 500 or GNU_SOURCE.  We can temporarily make one of these #defines before we include pthread.h, but this will fail if anyone happens to include pthread.h before they include glib.h.

But maybe we can punt on that.
Comment 1 Colin Walters 2011-09-22 19:59:26 UTC
This is a regression introduced by 3d4102776e59e748ee8b6e4d456a06a33593f308
Comment 2 Matthias Clasen 2011-09-22 23:00:27 UTC
I don't think it can be a regression, since GRWLock didn't even exist before that commit.

One easy way out here might be to simply document the requirements on include order of glib.h vs pthread.h...

But we certainly need a configure check for pthread_rwlock_t if we want to rely on it in this fashion.
Comment 3 Matthias Clasen 2011-09-23 14:44:59 UTC
One way forward that was explored on irc is to bring back the sizeof configure checks and avoid exposing the pthread_rwlock_t type in the headers
Comment 4 Colin Walters 2011-09-23 14:48:08 UTC
(In reply to comment #2)
> I don't think it can be a regression, since GRWLock didn't even exist before
> that commit.

The regression is that other components and apps fail to compile against glib master, whereas they did before.  All they're doing is including <gthread.h>.

Having things compile is good.
Comment 5 Matthias Clasen 2011-09-24 21:06:43 UTC
I've now added a (disabled) testcase that shows the problem: glib/tests/include.c
Comment 6 Allison Karlitskaya (desrt) 2011-10-03 01:02:57 UTC
*** Bug 660561 has been marked as a duplicate of this bug. ***
Comment 7 Allison Karlitskaya (desrt) 2011-10-03 01:32:45 UTC
Created attachment 198053 [details] [review]
GThread posix: switch to Windows ABI

Modify the POSIX implementation of the synchronisation primatives to use
the same ABI as Windows: one pointer for each type.

This frees us from having to #include <pthread.h> and avoids the problem
with pthread_rwlock_t not being defined under certain compiler defines.

A few more changes are expected to the ABI -- they will be committed
separately.
Comment 8 Allison Karlitskaya (desrt) 2011-10-03 01:32:49 UTC
Created attachment 198054 [details] [review]
Re-enable 'include' testcase

The bug is fixed now.
Comment 9 Allison Karlitskaya (desrt) 2011-10-03 01:32:52 UTC
Created attachment 198055 [details] [review]
locks: drop _INIT macros

All locks are now zero-initialised, so we can drop the G_*_INIT macros
for them.

Adjust various users around GLib accordingly and change the docs.
Comment 10 Matthias Clasen 2011-10-03 01:39:52 UTC
Review of attachment 198055 [details] [review]:

Looks generally good. I would like to see the zero-initialization mentioned explicitly in the docs, somewhere.

::: glib/gthread.c
@@ +331,3 @@
  *
  * A GRecMutex should only be accessed with the
+ * <function>g_rec_mutex_</function> functions.

I see that you ditched the static initializers, but the init() functions are still around, yes ? Should they not still be mentioned here ? And maybe it would be good to state explicitly that static locks/mutexes/whatever are usable as is, due to implicit zero-initialization by the compiler ?
Comment 11 Matthias Clasen 2011-10-03 01:41:55 UTC
Review of attachment 198053 [details] [review]:

Looks good.

::: glib/gthread-posix.c
@@ +84,3 @@
+  gint status;
+
+  mutex = malloc (sizeof (pthread_mutex_t));

This is system malloc because we can't use g_malloc() here ?
Comment 12 Matthias Clasen 2011-10-03 01:42:18 UTC
Review of attachment 198054 [details] [review]:

no problem
Comment 13 Matthias Clasen 2011-10-03 01:42:28 UTC
Review of attachment 198055 [details] [review]:

Looks generally good. I would like to see the zero-initialization mentioned explicitly in the docs, somewhere.

::: glib/gthread.c
@@ +331,3 @@
  *
  * A GRecMutex should only be accessed with the
+ * <function>g_rec_mutex_</function> functions.

I see that you ditched the static initializers, but the init() functions are still around, yes ? Should they not still be mentioned here ? And maybe it would be good to state explicitly that static locks/mutexes/whatever are usable as is, due to implicit zero-initialization by the compiler ?
Comment 14 Allison Karlitskaya (desrt) 2011-10-03 01:43:19 UTC
Review of attachment 198055 [details] [review]:

::: glib/gthread-posix.c
@@ +316,3 @@
  * It is not necessary to initialize a recursive mutex that has
+ * been created with g_rec_mutex_new(). It is not necessary to
+ * initialise a recursive mutex that has been statically allocated.

i mention that here, one for each type.

wouldn't hurt to add it back to the docs for the structure type itself, of course.
Comment 15 Allison Karlitskaya (desrt) 2011-10-03 02:11:05 UTC
Review of attachment 198053 [details] [review]:

::: glib/gthread-posix.c
@@ +84,3 @@
+  gint status;
+
+  mutex = malloc (sizeof (pthread_mutex_t));

Yes -- g_malloc() uses locks, so locks can't use g_malloc().
Comment 16 Allison Karlitskaya (desrt) 2011-10-03 02:16:49 UTC
fwiw, this malloc() business probably looks sort of silly, but in my defence:

  - we will replace GMutex and GCond with native implementations on Linux
    that use only the space already in the structure itself (no malloc)

  - Windows uses SRWLock and CONDITION_VARIABLE (no malloc)

  - FreeBSD and OpenBSD have pointer-sized pthread_mutex_t et al. that
    we can store directly into the structure (no malloc).

  - people used to use g_mutex_new() which involved a malloc anyway, and now
    they have g_mutex_init() to avoid that, so even in the worst case they
    are no worse off.

  - GRWLock and GRecMutex are vastly faster than the deprecated 'Static' ones
Comment 17 Allison Karlitskaya (desrt) 2011-10-03 02:33:29 UTC
Attachment 198053 [details] pushed as e081ead - GThread posix: switch to Windows ABI
Attachment 198054 [details] pushed as 3315aee - Re-enable 'include' testcase
Attachment 198055 [details] pushed as 2a677d1 - locks: drop _INIT macros