CODEWITHSUNDEEP
clinuxpthreads
lordofdou
lordofdou

Reputation: 63

semaphore operation in linux,receive SIGSEGV and segmentation fault,which part is wrong?

My thread functions are:

#include"stdio.h"
#include"sys/types.h"
#include"pthread.h"
#include"semaphore.h"

sem_t sem;
int running = 1;
int ret;
void *pf(void *arg)          //producer function
{
    int semval;
    while(running)
    {
        sleep(1);
        sem_post(&sem); 
        sem_getvalue(&sem,&semval); 
        printf("produce : %d\n",semval);
    }

}

void *cf(void *arg)         /*consumer function*/
{
    int semval;
    while(running)
    {
        sleep(1);
        sem_wait(&sem); 
        sem_getvalue(&sem,&semval);
        printf("consume : %d\n",semval);
    }
}

and main function is:

int main()
{
    pthread_t pf, cf;
    ret = sem_init(&sem,0,16);
    pthread_create(&pf,NULL,(void *)pf,NULL);   /*create producer*/
    pthread_create(&cf,NULL,(void *)cf,NULL);   /*create consumer*/
    sleep(1);
    running = 0;
    pthread_join(pf,NULL);
    pthread_join(cf,NULL);
    sem_destroy(&sem);  
    return 0;
}

When I run the executable file, it returns segmentation fault. I think the program maybe access invalid memory, but I don't know which part of my code is wrong!

Upvotes: 4

Views: 619

Answers (2)

user2371524
user2371524

Reputation: 

pthread_t pf, cf;
ret = sem_init(&sem,0,16);
pthread_create(&pf,NULL,(void *)pf,NULL);   /*create producer*/
pthread_create(&cf,NULL,(void *)cf,NULL);   /*create consumer*/

you are actually passing your thread descriptors to pthread_create() as the start routine. They have the same names as the functions you seem to mean here, but shadow them. Also note void * is a pointer to data, which is incompatible with a function pointer -> do not cast to void * here.

Correct code would e.g. look like this:

pthread_t pt, ct;
ret = sem_init(&sem,0,16);
pthread_create(&pt,NULL,pf,NULL);   /*create producer*/
pthread_create(&ct,NULL,cf,NULL);   /*create consumer*/

BTW, general hint: Enable compiler warnings: gcc -Wall -Wextra would have told you what is wrong.

edit: following the discussion on Blue Moon's answer: Just using an int to shut down your threads is indeed problematic. The data race occuring here doesn't matter in practice most of the time, because normally, you only want to tell your threads to stop and don't care, when exactly this is happening. But (and that's a big but): Your compiler sees this code:

while(running)
{
    [...]
}

where the condition for while is the only access to running in this function. Not knowing about concurrency, it could legally assume that running, once read, never changes. So, reading it once and using this read value (e.g. stored in a register) for the loop condition forever would be a valid optimization and definitely not what you want.

Oh, and adding a volatile to running is not the solution, but explanations of this tend to be lengthy, just google for it.

One possibility would be to use a semaphore for stopping the thread, too. E.g. I have this in one of my threaded projects:

/* check whether daemon shutdown was requested */
if (!sem_trywait(&forceExit))
{
    /* pass on to next thread */
    sem_post(&forceExit);
    rcout = -2;
    break;
}

The main thread just does a single sem_post(&forceExit) to shut all other threads down.

Upvotes: 2

P.P
P.P

Reputation: 121387

You have named your thread variables and functions with the same name: pf and cf. So the variables shadow the function names. It's never a good idea to have same name for variables and functions.

Change

pthread_create(&pf,NULL,(void *)pf,NULL);   /*create producer*/
pthread_create(&cf,NULL,(void *)cf,NULL);   /*create consumer*/

to

pthread_create(&pf,NULL,producer,NULL);   /*create producer*/
pthread_create(&cf,NULL,consumer,NULL);   /*create consumer*/

and rename your functions to producer and consumer respectively. Note that the casting is wrong too (and needless even if you cast is correctly) which I have removed.

You are returning any values from the thread functions. Thread functions should return void *. So you need to call pthread_exit(NULL); or return a null pointer.

Another major problem is you are accessing the variable running without any synchronization which leads to race condition. This is undefined behaviour. Depending on the thread scheduling, if main thread sets running to 0 before the threads get to execute then your threads may not execute the while loop at all.

Upvotes: 2

Related Questions