Segmentation fault without altering my program

Question

I was trying to write a program that calculates prime numbers using threads and the program was working but not giving the desired results (it was telling all numbers were prime numbers). Today I tried to run the program again and I'm getting a segmentation fault even though I didn't alter my program. I tried using gdb to find when it was happening and I think it's happening on the pthread_join function.

#include <stdlib.h>
#include <stdio.h> 
#include <unistd.h>
#include <pthread.h>

int n_threads; // number of threads
int* numbers; // array of numbers
int elements_per_thread;

int isPrime(int n){
    int i = 0;
    for(i=0; i < n; i++)
        if(n%i == 0)
            return 0;
    return 1;
}

void * threadPrime(void * arg){
    int* idxp = (int*)arg;
    int idx = *idxp;
    int start = idx* elements_per_thread;
    int finish = (idx+1)*elements_per_thread-1;
    int i;
        for(i=start; i<finish; i++)
            if(!isPrime(i))
                numbers[i]=0;
    pthread_exit(NULL);
}

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

  if (argc != 3) {
    printf("usage: %s largest_number number_threads\n", argv[0]);
    return 1;
  }

  int largest_number = atoi(argv[1]); // value of the largest number to test for primality
  int n_numbers = largest_number-1; // number of numbers to test
  n_threads = atoi(argv[2]);

  // create and fill vector of numbers to test  
  numbers = (int *)malloc(n_numbers*sizeof(int)) ;  // allocate a vector for n_numbers integers

  int i;
  for (i = 2; i <= largest_number; i++) 
    numbers[i-2] = i;

  int* id = (int *)malloc(n_threads*sizeof(int));
  // compute primes

 pthread_t* thid = (pthread_t *)malloc(n_threads*sizeof(int));
 for(i=0;i<n_threads;i++){
    id[i] = i;
    if(pthread_create(&thid[i],NULL,threadPrime,(void*)(id+i)) != 0){
       printf("Erro\n");
        exit(0);
    }
 }

 for(i=0;i<n_threads;i++){
    if(pthread_join(thid[i],NULL) != 0){
        printf("Erro\n");
        exit(0);
    }
 }

  // print result 
   printf("Primes:");
  int n_primes = 0;
  for (i = 0; i < n_numbers; i++) 
     if (numbers[i]) {
        n_primes++;
        printf (" %d", numbers[i]);
     }
  printf("\nTotal: %d primes\n", n_primes);

  return 0;         
}

Problem solved. Correct code below.

#include <stdlib.h>
#include <stdio.h> 
#include <unistd.h>
#include <pthread.h>

int n_threads; // number of threads
int* numbers; // array of numbers
int elements_per_thread;

int isPrime(int n){
    int i = 0;
    for(i=2; i < n; i++)
        if(n%i == 0)
            return 0;
    return 1;
}

void * threadPrime(void * arg){
    int* idxp = (int*) arg;
    int idx = *idxp;
    int start = idx*elements_per_thread;
    int finish = (idx+1)*elements_per_thread;
    int i;
        for(i=start; i<=finish; i++)
            if(!isPrime(numbers[i]))
                numbers[i]=0;
    pthread_exit(NULL);
}

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

 if (argc != 3) {
    printf("usage: %s largest_number number_threads\n", argv[0]);
    return 1;
  }

  int largest_number = atoi(argv[1]); // value of the largest number to test for primality
  int n_numbers = largest_number-1; // number of numbers to test
  n_threads = atoi(argv[2]);

  // create and fill vector of numbers to test  
  numbers = (int *)malloc(n_numbers*sizeof(int)) ;  // allocate a vector for n_numbers integers

  int i;
  for (i = 2; i <= largest_number; i++) 
    numbers[i-2] = i;

  int* id;
  id = (int *)malloc(n_threads*sizeof(int));

  // compute primeselements_per_thread = n_numbers/n_threads;

  elements_per_thread = (n_numbers/n_threads)+1;

 pthread_t* thid = malloc(n_threads*sizeof(*thid));

 for(i=0;i<n_threads;i++){
    id[i] = i;
    if(pthread_create(&thid[i],NULL,threadPrime,(void*)(id+i)) != 0){
        printf("Erro\n");
        exit(0);
    }
 }

 for(i=0;i<n_threads;i++){  
    if(pthread_join(thid[i],NULL) != 0){
        printf("Erro\n");
        exit(0);
    }
 }

  // print result 
  printf("Primes:");
  int n_primes = 0;
  for (i = 0; i < n_numbers; i++) 
     if (numbers[i]) {
        n_primes++;
        printf (" %d", numbers[i]);
     }
  printf("\nTotal: %d primes\n", n_primes);

  return 0;         
}

Answer 1

You didn't allocate the right amount of memory for thid. This is the main reason for your segmentation fault.

pthread_t* thid = (pthread_t *)malloc(n_threads*sizeof(int));

should be

pthread_t* thid = malloc(n_threads*sizeof(p_thread));

(you don't need to cast malloc in C)

This is why I don't usually use an explicit type as the operand of sizeof, and instead just use the variable name so that the compiler can deduce the type itself.

pthread_t* thid = malloc(n_threads*sizeof(*thid));

Answer 2

There are a number of issues:

1) The starting index for the loop should be 2. Otherwise, you are going to have divide by zero error here:

for(i=0; i < n; i++) // should be i=2
   if(n%i == 0)
        return 0;

2) elements_per_thread is not set at all. So it's going to be 0 (since it's a global variable) and the loops in thread function will never be called. Set it in main():

elements_per_thread = n_numbers/n_threads;

3) When you call isPrime() you are passing i. But you really wanted to pass numbers[i]. You also want to include the finish in the primality test. So it should be

    for(i=start; i<=finish; i++)
        if(!isPrime(numbers[i]))
            numbers[i]=0;

4) The allocation for thread array is wrong. It should be

pthread_t* thid = malloc(n_threads * sizeof *thid);

There are more efficient ways to test primes (e.g. you only need to check upto n/2 to see if it's prime). But once you fix the above issues, you'll have a working code and think about improving it later.