Worker function of the thread is not taking value 0 as arg and pthread_join is giving me segmentation fault

Question

I am trying to do matrix multiplication by creating the threads equal to no. of rows equal to no. of threads. The worker function is taking the index of first loop as argument but for 0 index function isnt working and for some strange reason worker function showing reptation of values. This is the worker function:

void* multiplyMatrixRows(void* arg) {
    int i = (*(int*)arg);
    
    printf("%d Loop \n", i);
    
    
    for (int j = 0; j < result.cols; j++) 
    {
        result.data[i][j] = 0;
        for (int k = 0; k < mat2->rows; k++) {
            result.data[i][j] += mat1->data[i][k] * mat2->data[k][j];
        }
        //printf("Value of Result: %d \n", result.data[i][j]);
    }
    
    pthread_exit(NULL);
}

And for some reason pthread_join also giving segmentation fault but work fine if I remove the comments and let them print those lines from code below:

pthread_t* threads = (pthread_t*)malloc(mat1->rows * sizeof(pthread_t));

    struct timeval start, end;
    for (int i = 0; i < result.rows; i++) {
        //printf("Entering thread %d \n", i);
        pthread_create(&threads[i], NULL, multiplyMatrixRows, (void*)&i);
        //printf("After thread %d \n", i);
    }

    for (int i = 0; i < result.rows; i++) {
        //printf("Entering join %d \n", i);
        pthread_join(threads[i], NULL);
        //printf("After join %d \n", i);
    }

mat1, mat2 and result are global variables here.This is the output. It is showing with which value of i you entered the worker function

I thought of using pthread_mutex_lock in main function as it is sending value for i but what is the use of lock in main thread. I tried printing values of i before and after creating a thread, it shows correct value of i but not in worker function. It would be really helpful if someone could help me with this.

Answer 1

I don't know if this is your only bug, but this line definitely has a bug:

pthread_create(&threads[i], NULL, multiplyMatrixRows, (void*)&i)

You have only one variable i, you pass a pointer to that variable to every thread, and you don't have anything to prevent the initial thread from changing the value of i, possibly several times, before each thread has a chance to read the value intended for it.

The simplest fix is to pass each thread the value you wanted to give it, rather than a pointer to a variable that, at one moment in the past, contained the intended value:

// add to top of file if not there already:
#include <stdint.h>

// change first line of multiplyMatrixRows to:
int i = (int)(intptr_t)arg;

// change pthread_create call to:
pthread_create(&threads[i], NULL, multiplyMatrixRows,
               (void*)(intptr_t)i);

You may have heard that casting between pointers and integers is risky, and that's true, but this particular pair of casts is safe.