CODEWITHSUNDEEP
cpthreadsmutex
Tom
Tom

Reputation: 332

Trying to understand POSIX Threads

I am trying to get a grasp on the use of POSIX threads and created a simple program that simply increments a global variable by 10. On sometimes it runs all the way through fine, other it seg faults in random spots. While this type of issue seems to be a regular thing with threads, I cannot understand why this happens in such a simple example. My current thought is maybe the parent is ending before due to not joining the threads, but if i try to join i seg fault on first thread... The Join statement is left in but commented out. Is Join used in correct syntax? Is not preforming the join leading to the seg fault?

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <errno.h>
#include <string.h>

#define NUMPHILO 5

thread_t threads[NUMPHILO];  //array of threads 
pthread_mutex_t mutex;        // initialize mutex

int x = 5;          //for test
int y = 10;

int philofunct(){
    pthread_mutex_lock (&mutex);
    x = x+y;
    pthread_mutex_unlock (&mutex);
    printf("Philo fucntion x = %d\n",x);
    return x;
}


int main(){

    pthread_mutex_init(&mutex, NULL);

    for(int i = 0; i < NUMPHILO; i++){  
        printf("creating thread[%i]\n",i);
        if( pthread_create(&threads[i], NULL, (void *)philofunct(),(void *) &i) != 0)
            fprintf(stderr, "%s", strerror(errno));
//      pthread_join(threads[i],NULL);  
//      printf("Threads Joined\n");
    }
    pthread_mutex_destroy(&mutex);
    printf("Threads created\n");
}

output: 

creating thread[0]
Philo fucntion x = 15
creating thread[1]
Philo fucntion x = 25
creating thread[2]
Philo fucntion x = 35
creating thread[3]
Philo fucntion x = 45
creating thread[4]
Philo fucntion x = 55
Threads created

next time ran:

creating thread[0]
Philo fucntion x = 15
creating thread[1]
Philo fucntion x = 25
creating thread[2]
Philo fucntion x = 35
Segmentation fault (core dumped)

As always, any help is greatly appreciated.

Upvotes: 1

Views: 62

Answers (3)

user3386109
user3386109

Reputation: 34829

The main thread is destroying the mutex before the child threads are finished with it. pthread_join will solve the problem, but the pthread_join needs to be in a separate loop after all of the threads have been created.

for(int i = 0; i < NUMPHILO; i++) 
   pthread_join(threads[i],NULL);

The function signature for the philofunct is wrong. In general, if you need a cast, you're doing something wrong. (Yes, there are some cases where a cast is necessary and useful. This is not one of those cases.) The correct signature is 

void *philofunct( void *arg );

and the correct call to pthread_create is 

if( pthread_create(&threads[i], NULL, philofunct, NULL) != 0)

Note that passing the address of i won't do what you want, but that's a separate question, and you haven't gotten to that point yet.

Upvotes: 1

JS1
JS1

Reputation: 4767

The main problem you have is here:

    if( pthread_create(&threads[i], NULL, (void *)philofunct(),(void *) &i) != 0)

Look carefully, you are calling philofunct() instead of passing the function pointer to it. Worse, you are passing the return value of philofunct(), which is some integer such as 15, as the function pointer to pthread_create(). Naturally, the thread will segv when it tries to execute code from an address such as 15.

Once you define philofunct() correctly like this:

void *philofunct(void *arg)

You can then call pthread_create() without a cast:

    if( pthread_create(&threads[i], NULL, philofunct,(void *) &i) != 0)

Also, if you want to join your threads, you should join your threads after they are all running. If you join the thread in the creation loop, you will be waiting for each thread to finish before creating the next one. So you would write it like this:

for (i=0;i<NUMPHILO; i++) {
    pthread_create(&threads[i], ...);
    // Other stuff...
}
for (i=0;i<NUMPHILO; i++)
    pthread_join(threads[i], NULL);

Upvotes: 4

o11c
o11c

Reputation: 16056

I see several problems with your code before even considering what you want:

  • You can't cast function pointers like that and expect a meaningful result. Instead, give the function the correct signature and cast the argument/return value at the edge of the function. But you usually don't actually need to use a return value.
  • You are accessing the variable x outside of the section locked by the mutex.
  • You are destroying the mutex without waiting for all the threads that are using it to exit. You need a second loop to pthread_join them all. Alternatively, you could pthread_exit the main thread instead of letting main terminate (and never destroy the mutex, which is not a leak since there's only one of them and its lifetime ends with the lifetime of the program), but killing the main thread sometimes makes funny kernel "bugs" happen.

Upvotes: 4

Related Questions