multithreading program to perform word count frequency- Segmentation fault

Viewed 1032
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <pthread.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <unistd.h>
#include <time.h>

pthread_mutex_t lock;

//typedef struct for a word
typedef struct {
    char word[101];
    int frequency;
}Word;

//struct for thread
struct ft{
    char* fileName;
    int start;
    int stop;
};

//compare frequency of 2 words 
int compareWords(const void *f1, const void *f2){
    Word *a = (Word *)f1;
    Word *b = (Word *)f2;
    return (b->frequency - a->frequency);
}

//count frequency of a word
void countFrequency(void *arg){

    pthread_mutex_lock(&lock);

    int i, c;
    struct ft* fi = (struct ft*)arg;
    FILE *file = fopen(fi->fileName,"r");
    fseek(file,fi->start,SEEK_SET);
    for(i = 0; i < fi->stop - fi->start; i++){
        c = getc(file);
        //printf("%d\n", c);
        //frequency count
    }

    fclose(file);

    pthread_mutex_unlock(&lock);
}

int main (int argc, char **argv){

    //variabies for <time.h>
    struct timespec startTime;
    struct timespec endTime;
    clock_gettime(CLOCK_REALTIME, &startTime);

    /*------------main------------------*/

    //variables
    int nthreads;   //number of threads
    int chunkSize;  //each threas processing size

    //if user input is not correct, inform
    if(argc < 3){ 
       printf("./a.out text_file #ofthreads \n");
       exit(-1);
    }


    nthreads = atoi(argv[2]);
    chunkSize = sizeof(argv[1])/nthreads;

    //declare threads and default attributes
    pthread_t threads[nthreads];
    pthread_attr_t attr;
    pthread_attr_init(&attr);

    //run threads in parallel
    int i; 
    for (i = 0; i < nthreads; i++){
       struct ft data[nthreads];
       data[i].start = i*chunkSize;
       data[i].stop = data[i].start+chunkSize;
       data[i].fileName = argv[1];


       // Create a new thread for every segment, and count word  frequency for each
       pthread_create(&threads[i], &attr, (void*) countFrequency, (void*) &data[i]);

    }

    //wait for results (all threads)
    for (i = 0; i < nthreads; i++){
       pthread_join(threads[i], NULL);
    } 

       //func of <time.h>
       clock_gettime(CLOCK_REALTIME, &endTime);
       time_t sec = endTime.tv_sec - startTime.tv_sec;
       long n_sec = endTime.tv_nsec - startTime.tv_nsec;
       if (endTime.tv_nsec < startTime.tv_nsec)
        {
        --sec;
        n_sec = n_sec + 1000000000L;
        }
       printf("Total Time was %ld.%09ld seconds\n", sec, n_sec);
}

I'm working on this program to use multiple threads to read and process a large text file and perform a word count frequency of the top 10 most frequent words in the text that are longer than 6 characters long. But I keep getting the segmentation fault error im not sure why, does anybody have any idea.?

1 Answers

This code:

   for (i = 0; i < nthreads; i++){
       struct ft data[nthreads];

declares data that is live (legal to use) for the duration of this for loop. This code:

      pthread_create(&threads[i], &attr, (void*) countFrequency, (void*) &data[i]);
   }

passes the address of data into the threads, and then exits the loop. Once the loop is done, data is no longer live, and all access to it leads to undefined behavior.

The compiler is free to write anything else to the memory where data used to be.

The immediate cause of the crash is that if one of the threads doesn't execute fopen before data is overwritten, then fopen may fail, and you don't check for the failure in fopen.

P.S.

As Eraklon noted, this code: chunkSize = sizeof(argv[1])/nthreads; will divide sizeof(char*) (either 4 or 8 depending on whether you build for 32-bit or for 64-bit) by number of threads. That is unlikely to be what you want, and will yield chinkSize==0 for nthreads > 4 on 32-bit and nthreads > 8 on 64-bit machines.

P.P.S.

There is a concurrency bug in your program as well: since each of the countFrequency invocations locks the same lock for the entire duration, they will all run in sequence (one after another), never in parallel. Thus your program will be slower than if you just did all the work in your main thread.

Related