Syllable Forum Index Syllable
Syllable Forums
 
 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

PThreads bugs

 
Post new topic   Reply to topic    Syllable Forum Index -> Syllable Desktop
View previous topic :: View next topic  
Author Message
Vanders
The Knights of Syllable


Joined: 14 Sep 2007
Posts: 849

PostPosted: Thu Jun 07, 2012 4:38 pm    Post subject: Reply with quote

This has been bugging me, so I went back over those mutex initialisers. There are still some problems:

1) __owner is not initialised.
2) __mutex is set to 0
3) __attr is set to NULL

#1 is easy to solve. #2 and especially #3 are a real problem, because of lines like this in places like pthread_mutex_lock():

if( ( mutex->__attr == 0 ) || ( mutex->__mutex == 0 ) )
pthread_mutex_init( mutex, NULL );

Basically if you don't initialise __mutex and __attr, the mutex will just get re-initialised to default values the first time you use it (which means as it stands PTHREAD_RECURSIVE_MUTEX_INITIALIZER doesn't really work).

So #1 & #2 can be solved with something like this:

#define PTHREAD_MUTEX_INITIALIZER \
{ create_semaphore( "pthread_sem", 1, SEM_WARN_DBL_LOCK ), pthread_self(), 0, NULL, { PTHREAD_MUTEX_NORMAL, PTHREAD_PROCESS_PRIVATE } }

#define PTHREAD_RECURSIVE_MUTEX_INITIALIZER \
{ create_semaphore( "pthread_sem", 1, SEM_WARN_DBL_LOCK ), pthread_self(), 0, NULL, { PTHREAD_MUTEX_RECURSIVE, PTHREAD_PROCESS_PRIVATE } }

#3 is a real bastard. I can't think of any way to initialise __attr to point to __def_attr the way it's done in pthread_mutex_init(). So you'd have to provide some sort of function to create and return a new pthread_mutexattr_t:

pthread_mutexattr_t *__pt_alloc_mutex_attr(int kind, int shared)
{
pthread_mutexattr_t * attr = malloc(sizeof(pthread_mutexattr_t));
if ( attr )
{
attr->__mutexkind = kind;
attr->__pshared = shared;
}

return attr;
}

#define PTHREAD_MUTEX_INITIALIZER \
{ .__mutex = create_semaphore( "pthread_sem", 1, SEM_WARN_DBL_LOCK ), \
.__owner = pthread_self(), \
.__count = 0, \
.__attr = __pt_alloc_mutex_attr( PTHREAD_MUTEX_NORMAL, PTHREAD_PROCESS_PRIVATE ), \
.__def_attr = { PTHREAD_MUTEX_NORMAL, PTHREAD_PROCESS_PRIVATE } \
}

#define PTHREAD_RECURSIVE_MUTEX_INITIALIZER \
{ .__mutex = create_semaphore( "pthread_sem", 1, SEM_WARN_DBL_LOCK ), \
.__owner = pthread_self(), \
.__count = 0, \
.__attr = __pt_alloc_mutex_attr( PTHREAD_MUTEX_RECURSIVE, PTHREAD_PROCESS_PRIVATE ), \
.__def_attr = { PTHREAD_MUTEX_RECURSIVE, PTHREAD_PROCESS_PRIVATE } \
}

There is a downside that the attribute being malloc()'d is never free()'d, but a) This is a static initialiser, so may not be a problem b) You could always add a new flag to the pthread_mutexattr_t struct to indicate it was an internal malloc (similar to how the pthread_attr_t is handled) and then change pthread_mutexattr_destroy() to something like:

int pthread_mutexattr_destroy(pthread_mutexattr_t *attr)
{
if( attr == NULL )
return( EINVAL );

if ( attr->internal_malloc )
free( attr );

return( 0 );
}

The downside to this is that it would change the size of the pthread_mutexattr_t struct so you'd have to bump the version on libpthread.so
Back to top
View user's profile Send private message Send e-mail
Kaj
The Knights of Syllable


Joined: 14 Sep 2007
Posts: 2202
Location: Friesland

PostPosted: Thu Jun 07, 2012 5:13 pm    Post subject: Reply with quote

Yes, I've already fixed all these bugs and more. I've left the initialisers fully static and binary compatible, so no need to bump the library version for now. I've added full error checking for all dynamic actions and avoided extra malloc'ing.

DirectFB is starting to work and I've moved on to fixing condition variables. Along the way I'm smoothing out mutexes before checking in the overhaul.
Back to top
View user's profile Send private message Visit poster's website
Vanders
The Knights of Syllable


Joined: 14 Sep 2007
Posts: 849

PostPosted: Fri Jun 08, 2012 3:27 am    Post subject: Reply with quote

"Yes, I've already fixed all these bugs and more. I've left the initialisers fully static and binary compatible"

Cool. I checked CVS but didn't realise you had other outstanding changes. How did you resolve the __attr issue with the static initialiser?
Back to top
View user's profile Send private message Send e-mail
Kaj
The Knights of Syllable


Joined: 14 Sep 2007
Posts: 2202
Location: Friesland

PostPosted: Fri Jun 08, 2012 6:48 am    Post subject: Reply with quote

I've done away with the attr pointer. It wasn't used in a critical way anywhere. The attributes that were called default in the mutex struct are now the attributes period, so they can be statically initialised.

The initialisers must be static. The funtions that use them, or at least lock and try_lock, must check for this state and fully initialise them. This only worked for normal mutexes. I have now fixed it for recursive and error checking mutexes and for condition variables.

Also, attributes gotten from the user were cast from const and then used and written during the lifetime of the mutex, disregarding that they may be reused by the caller or even cast away. During about the first half of Syllable's life, they were copied as they should, but this was changed later in a confusion trying to fix them being free()'d illegally. They're now copied again, but without the uselessly complicated pointer construction and malloc'ing. Fixing this was a concrete thing that started to make DirectFB work better. I've fixed the similar issue in conditionals, although attrinutes aren't used there yet.

I've found many other problems.
Back to top
View user's profile Send private message Visit poster's website
Kaj
The Knights of Syllable


Joined: 14 Sep 2007
Posts: 2202
Location: Friesland

PostPosted: Wed Jun 27, 2012 6:44 pm    Post subject: Reply with quote

I've found and fixed another serious bug. The memory of all pthreads was corrupted, and any data after it.
Back to top
View user's profile Send private message Visit poster's website
Display posts from previous:   
Post new topic   Reply to topic    Syllable Forum Index -> Syllable Desktop All times are GMT - 6 Hours
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group