Need some help with Threads

[dearnightmare]

Hello, this is my first time here, so if I've done something incorrectly please tell me what I've done and I'll change up the post to make it look better.

Ok, so basically when we call the program ./4p n m, we're supposed to get the m prime numbers that follow number N.

isPrimeLong(), the function in the thread, is pulled from a library we built for class, and works fine, all it does is confirm whether the number is prime or not. returns 1 if it has a factor 0 if prime

Here is my problem. When I run it, sometimes I get segfaults. Not always, and they seem more common, the more primes I need to find. Sometimes I get a memory dump from the command line too, I really dont get it.

I guess it means a leak somewhere?? If someone that has used threads more than me could look at this and tell me If I've done something stupidly, I'd really appreciate it

I'm attaching the code and then the outputs I get

part4.c

#include "primelib.h"
#include <pthread.h>


long long *GBUFFER;
int BUFFSIZE=0;
long long CURNUM;
int LIMIT;
static pthread_mutex_t cs_mutex = PTHREAD_MUTEX_INITIALIZER;




void *testThread(void *arg){
        int prime;
        long long thisNum;


        for(;;){
                //Critical Section for getting the number we're working with
                //if(LOCKNUM == 0){
                pthread_mutex_lock( &cs_mutex );
                thisNum=CURNUM;
                CURNUM++;
                pthread_mutex_unlock( &cs_mutex );


                prime = isPrimeLongLong(thisNum); 
                BUFFSIZE++;
                GBUFFER[BUFFSIZE-1]=thisNum;
                
                if(BUFFSIZE == LIMIT){
                        break;
                }
        }
}


int main(int argc, char *argv[]){
        if(argc != 3){
                fprintf(stderr, "Wrong number of CMD line arguments\n");
                exit(-1);
        }


        CURNUM = atoll(argv[1]);
        LIMIT = atoi(argv[2]);
        GBUFFER = malloc((sizeof(long long))* 100);


        /*TEST*/


        int fd;
        int error;
        int i;
        pthread_t  id[LIMIT];


        for(i=0; i<LIMIT; i++){
                pthread_create(&id[0], NULL, testThread, NULL);
        }


        for(i=0; i<LIMIT; i++){
                pthread_join(id[0], NULL);
        }


        printf("%s\n", argv[1]);
        for(i=0; i<LIMIT; i++){
                printf("%llu\n", GBUFFER[i]);
        }


        free(GBUFFER);


        return 0;
}

output:

elk04:~/Desktop/hw4> ./4p 911 6
911
911
919
929
937
941
947
elk04:~/Desktop/hw4> ./4p 911 6
Segmentation fault

at around 6, there is about a 50-50 segfault change, but by the time I crank it up to 10, it segfaults Pmuch every time.

Thank you very much in advance for your help!

[grumpy]

All sorts of things wrong with that code

1) When calling pthread_create() and, later, pthread_join(), the first argument provided is &id[0]. This means that your code only keeps track of the last thread created, and only joins that thread multiple times. (IIRC, the behaviour that results from joining the same thread multiple times is undefined as well).

2) You're using a fixed size for GBUFFER, but not checking if that size is exceeded when writing to its elements.

3) Your code is calling isPrimeLongLong() but not doing anything different based on the result.

5) GBUFFER and BUFFSIZE are both being written to by multiple threads, but are not being protected by a critical section.

[dearnightmare]

thanks a lot for your quick response grumpy, and that was SUPER helpful. Ugh lots of small embarrassing mistakes. I went ahead and checked all the things you mentioned,

Code v2.0

#include "primelib.h"
#include <pthread.h>


long long *GBUFFER;
int BUFFSIZE=0;
long long CURNUM;
int LIMIT;
static pthread_mutex_t cs_mutex = PTHREAD_MUTEX_INITIALIZER;




void *testThread(void *arg){
        int prime;
        long long thisNum;


        for(;;){
                //Critical Section for getting the number we're working with
                pthread_mutex_lock( &cs_mutex );
                thisNum=CURNUM;
                CURNUM++;
                pthread_mutex_unlock( &cs_mutex );


                prime = isPrimeLongLong(thisNum);//calculate if its prime or not
 
                if(prime == 0){
                        pthread_mutex_lock( &cs_mutex );
                                if(BUFFSIZE < LIMIT){//if not full, add it to the buffer list
                                        BUFFSIZE++;
                                        GBUFFER[BUFFSIZE-1]=thisNum;
                                }else{//if full, add it last if its smaller than the last number there
                                        if(thisNum< GBUFFER[BUFFSIZE]){
                                                GBUFFER[BUFFSIZE]=thisNum;
                                        }
                                }
                        pthread_mutex_unlock( &cs_mutex );
                }
                //if BUFFSIZE == LIMIT, then we're done
                if(BUFFSIZE == LIMIT){
                        break;
                }
        }
}


int main(int argc, char *argv[]){
        if(argc != 3){
                fprintf(stderr, "Wrong number of CMD line arguments\n");
                exit(-1);
        }


        CURNUM = atoll(argv[1]);//number we start from
        LIMIT = atoi(argv[2]);//No of Primes being gotten
        GBUFFER = malloc((sizeof(long long))* LIMIT);


        int fd;
        int i;
        int NoThreads=2;//no Of threads we're making
        pthread_t  id[NoThreads];


        //create thread
        for(i=0; i<NoThreads; i++){
                pthread_create(&id[i], NULL, testThread, NULL);
        }


        //join all threads
        for(i=0; i<NoThreads; i++){
                pthread_join(id[i], NULL);
        }


        //print all the primes
        printf("%s\n", argv[1]);
        for(i=0; i<LIMIT; i++){
                printf("%llu\n", GBUFFER[i]);
        }


        free(GBUFFER); //free the GBUFFER we mallocked


        return 0;
}

Its now a lot more stable, I still get the memory fault in there sometimes

output:

elk04:~/Desktop/hw4> ./4p 7898942 10
7898942
7898981
7898983
7898993
7899011
7899029
7899037
7899053
7899071
7899083
7899113
*** glibc detected *** ./4p: double free or corruption (!prev): 0x0804c008 ***
======= Backtrace: =========
/lib/tls/i686/cmov/libc.so.6[0xb7eab604]
/lib/tls/i686/cmov/libc.so.6(cfree+0x96)[0xb7ead5b6]
./4p[0x8048b19]
/lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe5)[0xb7e52775]
./4p[0x8048741]
======= Memory map: ========
08048000-0804a000 r-xp 00000000 00:18 123667951  /home/jparra/Desktop/hw4/4p
0804a000-0804b000 r--p 00001000 00:18 123667951  /home/jparra/Desktop/hw4/4p
0804b000-0804c000 rw-p 00002000 00:18 123667951  /home/jparra/Desktop/hw4/4p
0804c000-0806d000 rw-p 0804c000 00:00 0          [heap]
b6d00000-b6d21000 rw-p b6d00000 00:00 0 
b6d21000-b6e00000 ---p b6d21000 00:00 0 
b6e29000-b6e36000 r-xp 00000000 08:05 33784      /lib/libgcc_s.so.1
b6e36000-b6e37000 r--p 0000c000 08:05 33784      /lib/libgcc_s.so.1
b6e37000-b6e38000 rw-p 0000d000 08:05 33784      /lib/libgcc_s.so.1
b6e38000-b6e39000 rw-p b6e38000 00:00 0 
b6e39000-b6e3a000 ---p b6e39000 00:00 0 
b6e3a000-b763a000 rw-p b6e3a000 00:00 0 
b763a000-b763b000 ---p b763a000 00:00 0 
b763b000-b7e3c000 rw-p b763b000 00:00 0 
b7e3c000-b7f98000 r-xp 00000000 08:05 32899      /lib/tls/i686/cmov/libc-2.9.so
b7f98000-b7f99000 ---p 0015c000 08:05 32899      /lib/tls/i686/cmov/libc-2.9.so
b7f99000-b7f9b000 r--p 0015c000 08:05 32899      /lib/tls/i686/cmov/libc-2.9.so
b7f9b000-b7f9c000 rw-p 0015e000 08:05 32899      /lib/tls/i686/cmov/libc-2.9.so
b7f9c000-b7f9f000 rw-p b7f9c000 00:00 0 
b7f9f000-b7fb4000 r-xp 00000000 08:05 32902      /lib/tls/i686/cmov/libpthread-2.9.so
b7fb4000-b7fb5000 r--p 00014000 08:05 32902      /lib/tls/i686/cmov/libpthread-2.9.so
b7fb5000-b7fb6000 rw-p 00015000 08:05 32902      /lib/tls/i686/cmov/libpthread-2.9.so
b7fb6000-b7fb9000 rw-p b7fb6000 00:00 0 
b7fb9000-b7fdd000 r-xp 00000000 08:05 32915      /lib/tls/i686/cmov/libm-2.9.so
b7fdd000-b7fde000 r--p 00023000 08:05 32915      /lib/tls/i686/cmov/libm-2.9.so
b7fde000-b7fdf000 rw-p 00024000 08:05 32915      /lib/tls/i686/cmov/libm-2.9.so
b7fdf000-b7fe1000 rw-p b7fdf000 00:00 0 
b7fe1000-b7fe2000 r-xp b7fe1000 00:00 0          [vdso]
b7fe2000-b7ffe000 r-xp 00000000 08:05 33783      /lib/ld-2.9.so
b7ffe000-b7fff000 r--p 0001b000 08:05 33783      /lib/ld-2.9.so
b7fff000-b8000000 rw-p 0001c000 08:05 33783      /lib/ld-2.9.so
bffeb000-c0000000 rw-p bffeb000 00:00 0          [stack]
Abort

but it still seems to be giving me the right code most of the time, so I'm kinda confused. At this point I have what I need for my assignment I still want to figure out what IS actually wrong though. Thanks a lot again for your help! and Thanks in advance to anyone that helps in the future.

[grumpy]

When any variable is able to be modified by multiple threads, ANY access of its value (read the value or modify it) needs to be synchronised. You're not doing that.

[dearnightmare]

Can you be a bit more specific? what am I not protecting? on the testThread function pretty much the only thing that is not protected is the isPrimeLongLong fucntion.
The main function, besides allocating space, all it does is join the threads and then read the GBUFFER array we made.

[grumpy]

BUFFSIZE is being changed by multiple threads. Its value is being accessed outside the critical section.

I don't know if that is the only problem, but it is also potentially a contributor.

One rule of multi-threaded programming is paranoia. You must assume that all threads are out to tromp your global data. If one thread is potentially writing a value, and another is potentially reading it, both operations need to be synchronised.

The other thing to remember is that basic operations (retrieving a value, incrementing, comparing with another value) are not atomic (those operations can often be preempted by another thread before the operation is complete).