[cfe-commits] patch: libcxxabi cxa_*_virtual and cxa_guard_* methods

[Nick Lewycky]

On 22 May 2011 12:23, Howard Hinnant <hhinnant at apple.com> wrote:

> On May 20, 2011, at 2:11 AM, Nick Lewycky wrote:
>
> > I've started dipping my toes in the libcxxabi project by implementing a
> few simple methods. Patch attached, please review!
> >
> > This isn't heavily tested, I'm mostly just trying to make sure I have the
> style down. Please let me know if we should be using different file names
> (hard to follow a pattern where there isn't one), of if the private methods
> in cxa_guard actually belong in an anonymous namespace, etc.
> >
> > Nick
> >
> > <libcxxabi-virtual-and-guard.patch>
>
> I disagree with John that your implementation doesn't support the recursive
> calls for *different* static objects.  I believe it does.


Thanks. I was just about to write up an email to the same effect. It does
support recursive calls for different static objects because the lock it
uses is the second byte of the guard variable, not a global (or per-thread)
lock.

It's a great test to add to test_guard.cpp though!

And I like that your design allows for parallel initialization of different
> static objects.
>

Thanks! I'm very motivated to preserve this property.

What I'm nervous about though is the use of the spin lock.  An arbitrary
> amount of time can go by while one thread is initializing and the other
> thread is spinning, waiting for that initialization to complete.
>

Sure, but what are the alternatives? sched_yield()?

I translated your code into using pthread_mutex_t and pthread_cond_t, and
> came up with the following.  I tried to keep your notation so as to make the
> two implementations more easily comparable:
>
> #include <pthread.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> namespace __cxxabiv1
> {
>
> namespace __libcxxabi
> {
>
> void abort(const char* s) {printf("%s\n", s); ::abort();}
>
> }  // __libcxxabi
>
> static pthread_mutex_t guard_mut = PTHREAD_MUTEX_INITIALIZER;
> static pthread_cond_t  guard_cv  = PTHREAD_COND_INITIALIZER;
>
> extern "C"
> {
>
> int __cxa_guard_acquire(int64_t* guard_object)
> {
>    uint8_t* initialized = (uint8_t*)guard_object;
>    uint8_t* lock = (uint8_t*)guard_object + 1;
>    int r = pthread_mutex_lock(&guard_mut);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_acquire failed to acquire mutex");
>    if (*initialized == 1)
>    {
>        r = pthread_mutex_unlock(&guard_mut);
>        if (r != 0)
>            __libcxxabi::abort("__cxa_guard_acquire failed to release
> mutex");
>        return 0;
>    }
>    while (*lock == 1)
>        pthread_cond_wait(&guard_cv, &guard_mut);
>    int result = *initialized;
>    if (result == 0)
>        *lock = 1;
>    r = pthread_mutex_unlock(&guard_mut);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_acquire failed to release mutex");
>    return result == 0;
> }
>
> void __cxa_guard_release(int64_t* guard_object)
> {
>    uint8_t* initialized = (uint8_t*)guard_object;
>    uint8_t* lock = (uint8_t*)guard_object + 1;
>    int r = pthread_mutex_lock(&guard_mut);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_release failed to acquire mutex");
>    *initialized = 1;
>    *lock = 0;
>    r = pthread_mutex_unlock(&guard_mut);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_release failed to release mutex");
>    r = pthread_cond_broadcast(&guard_cv);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_release failed to broadcast
> condition variable");
> }
>
> void __cxa_guard_abort(int64_t* guard_object)
> {
>    uint8_t* lock = (uint8_t*)guard_object + 1;
>    int r = pthread_mutex_lock(&guard_mut);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_abort failed to acquire mutex");
>    *lock = 0;
>    r = pthread_mutex_unlock(&guard_mut);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_abort failed to release mutex");
>    r = pthread_cond_broadcast(&guard_cv);
>    if (r != 0)
>        __libcxxabi::abort("__cxa_guard_abort failed to broadcast condition
> variable");
> }
>
> }  // extern "C"
>
> }  // __cxxabiv1
>
> The structure of our implementations is nearly identical.
>

If I understand correctly, yours does detect recursive initializers.
pthread_mutex_lock will return EDEADLK, and your code will abort.

I'll need to spend more time reviewing your implementation before I make
further comments about it. I'm not sufficiently familiar with the pthreads
API.

One of the things I don't like about my implementation (or yours) is that it
> does not detect recursive entry of the same static object.  I'm thinking of
> something that makes use of thread_local data get that part, but I'm not
> there yet.  But I thought I'd share what I have so far.
>

Back when I thought that we needed to support self-recursive initialization,
I came up with a way to do this. The fundamental problem with pthread_self()
is that it returns an 8-byte opaque object (ie., they aren't guaranteed to
be dense), and C++11's thread library is worse. (Linux supports "gettid()"
which returns a 4-byte ID which would fit neatly in the guard variable, but
let's leave that aside for the moment.) We can construct our own thread ID
by creating a thread_local variable, and we can make it use only 7 bytes by
giving it 256 byte alignment. The recursion depth counter would then be kept
on the initialization byte (supporting 254 levels of recursion depth with
values 0 and 1 reserved for an unlocked guard variable).

The nasty part is that we don't have a 7-byte CAS operation, but I'm pretty
sure we don't need it. On entry, do CAS of all-zeros to (&thread_id | 0x2)
and if you get it, you're the first to lock the variable. Otherwise, you can
do an unordered memory read to check the thread id. If it matches, you're
the thread which already holds the lock so you can conclude that your
previous memory read was safe since no other thread is touching that memory.
You can then increment the recursion count (no atomicity needed, for the
same reason). If the thread_id doesn't match, spin until you see an
initialized value in the guard variable, or successfully lock it (implying
that the previous attempt ended in __cxa_guard_abort).

Thanks for helping us with this library!
>

Thanks for releasing it!

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110522/5a38850a/attachment.html>