4
\$\begingroup\$

I have written this little thingie to fix a problem of missing pthread_barrier_t in Mac OS/X pthreads. Are there any issues with this code?

The header:

#ifndef PTHREAD_BARRIER_H
#define PTHREAD_BARRIER_H

#include <pthread.h>

#ifdef __APPLE__

#ifdef __cplusplus
extern "C" {
#endif

#if !defined(PTHREAD_BARRIER_SERIAL_THREAD)
# define PTHREAD_BARRIER_SERIAL_THREAD  (1)
#endif

#if !defined(PTHREAD_PROCESS_PRIVATE)
# define PTHREAD_PROCESS_PRIVATE    (42)
#endif
#if !defined(PTHREAD_PROCESS_SHARED)
# define PTHREAD_PROCESS_SHARED     (43)
#endif

typedef struct {
} pthread_barrierattr_t;

typedef struct {
    pthread_mutex_t mutex;
    pthread_cond_t cond;
    unsigned int limit;
    unsigned int count;
    unsigned int phase;
} pthread_barrier_t;

int pthread_barrierattr_init(pthread_barrierattr_t *attr);
int pthread_barrierattr_destroy(pthread_barrierattr_t *attr);

int pthread_barrierattr_getpshared(const pthread_barrierattr_t *restrict attr,
                   int *restrict pshared);
int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
                   int pshared);

int pthread_barrier_init(pthread_barrier_t *restrict barrier,
             const pthread_barrierattr_t *restrict attr,
             unsigned int count);
int pthread_barrier_destroy(pthread_barrier_t *barrier);

int pthread_barrier_wait(pthread_barrier_t *barrier);

#ifdef  __cplusplus
}
#endif

#endif /* __APPLE__ */

#endif /* PTHREAD_BARRIER_H */

The source file:

#include "pthread_barrier.h"

#include <errno.h>

#ifdef __APPLE__

#define __unused __attribute__((unused))

int
pthread_barrierattr_init(pthread_barrierattr_t *attr __unused)
{
    return 0;
}

int
pthread_barrierattr_destroy(pthread_barrierattr_t *attr __unused)
{
    return 0;
}

int
pthread_barrierattr_getpshared(const pthread_barrierattr_t *restrict attr __unused,
                   int *restrict pshared)
{
    *pshared = PTHREAD_PROCESS_PRIVATE;
    return 0;
}

int
pthread_barrierattr_setpshared(pthread_barrierattr_t *attr __unused,
                   int pshared)
{
    if (pshared != PTHREAD_PROCESS_PRIVATE) {
        errno = EINVAL;
        return -1;
    }
    return 0;
}

int
pthread_barrier_init(pthread_barrier_t *restrict barrier,
             const pthread_barrierattr_t *restrict attr __unused,
             unsigned count)
{
    if (count == 0) {
        errno = EINVAL;
        return -1;
    }

    if (pthread_mutex_init(&barrier->mutex, 0) < 0) {
        return -1;
    }
    if (pthread_cond_init(&barrier->cond, 0) < 0) {
        int errno_save = errno;
        pthread_mutex_destroy(&barrier->mutex);
        errno = errno_save;
        return -1;
    }

    barrier->limit = count;
    barrier->count = 0;
    barrier->phase = 0;

    return 0;
}

int
pthread_barrier_destroy(pthread_barrier_t *barrier)
{
    pthread_mutex_destroy(&barrier->mutex);
    pthread_cond_destroy(&barrier->cond);
    return 0;
}

int
pthread_barrier_wait(pthread_barrier_t *barrier)
{
    pthread_mutex_lock(&barrier->mutex);
    barrier->count++;
    if (barrier->count >= barrier->limit) {
        barrier->phase++;
        barrier->count = 0;
        pthread_cond_broadcast(&barrier->cond);
        pthread_mutex_unlock(&barrier->mutex);
        return PTHREAD_BARRIER_SERIAL_THREAD;
    } else {
        unsigned phase = barrier->phase;
        do
            pthread_cond_wait(&barrier->cond, &barrier->mutex);
        while (phase == barrier->phase);
        pthread_mutex_unlock(&barrier->mutex);
        return 0;
    }
}

#endif /* __APPLE__ */
\$\endgroup\$
3
  • \$\begingroup\$ Instead of __unused, why not just comment out the parameter name? \$\endgroup\$
    – glampert
    Apr 29, 2015 at 3:06
  • \$\begingroup\$ Because it's C, not C++. stackoverflow.com/questions/8776810/… \$\endgroup\$ Apr 29, 2015 at 6:44
  • \$\begingroup\$ Interesting, I work with both languages yet hadn't noticed this detail... Thanks for the heads-up! \$\endgroup\$
    – glampert
    Apr 29, 2015 at 18:39

2 Answers 2

3
\$\begingroup\$

No problems found

As they say, "no news is good news". I read your code all the way through with an eye towards finding any possible errors, but I didn't find anything to criticize.

Instead I'll point out the things I liked about your code.

Use of restrict keyword

I must admit, I don't use restrict but I should learn to, as it can only help the compiler. I already advocate using const when appropriate, and this is similar.

Proper error checking

You check for errors and set errno appropriately.

Correct barrier operation

I tried to find any concurrency flaw in your barrier wait function but it looked correct.

The one weird thing

The one thing I thought was strange was that you used a do {} while loop without the curly braces:

    do
        pthread_cond_wait(&barrier->cond, &barrier->mutex);
    while (phase == barrier->phase);

I've never seen that before but I guess if you like to do that it's fine. I think people throw in the curlies because it doesn't waste any lines, whereas with a while loop or an if block it would add an extra line:

    while (condition) {
        foo();
    }                       // <-- Wasted line

    do {
        foo();
    } while (condition);    // <-- No waste
\$\endgroup\$
0
\$\begingroup\$

I believe this implementation suffers from the bug described here: https://sourceware.org/bugzilla/show_bug.cgi?id=13065

IOW, it's possible that a thread is accessing the barrier's memory while others have exit pthread_barrier_wait. And the threads having exited pthread_barrier_wait are free to call pthread_barrier_destroy, leading to undefined behavior.

The fix in glibc referred to in the ticket can be found here: https://github.com/Distrotech/glibc/commit/b02840bacdefde318d2ad2f920e50785b9b25d69

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.