Unix c program to calculate pi using threads

Question

Been working on this assignment for class. Put this code together but its giving me several errors I'm not able to solve.

Code

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

//global variables
int N, T;
double vsum[T];

//pie function
void* pie_runner(void* arg)
{
    double *limit_ptr = (double*) arg;
    double j = *limit_ptr;

    for(int i = (N/T)*j; i<=((N/T)*(j+1)-1); j++)
    {
        if(i %2 =0)
            vsum[j] += 4/((2*j)*(2*j+1)*(2*j+2)); 
        else
            vsum[j] -= 4/((2*j)*(2*j+1)*(2*j+2));

    }

    pthread_exit(0);
}

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

    if(argc != 3) {
        printf("Error: Must send it 2 parameters, you sent %s", argc);
        exit(1);
    }
    N = atoi[1];
    T = atoi[2]; 

    if(N !> T) {
        printf("Error: Number of terms must be greater then number of threads.");
        exit(1);    
    }

    for(int p=0; p<T; p++)        //initialize array to 0
    {
        vsum[p] = 0;
    }

    double pie = 3;
    //launch threads
    pthread_t tids[T];

    for(int i = 0; i<T; i++)
    {
        pthread_attr_t attr;
        pthread_attr_init(&attr);
        pthread_create(&tids[i], &attr, pie_runner, &i);
    }

    //wait for threads...
    for(int k = 0; k<T; k++)
    {
        pthread_join(tids[k], NULL);
    }

    for(int x=0; x<T; x++)
    {
        pie += vsum[x];
    }

    printf("pi computed with %d terms in %s threads is %k\n", N, T, pie);


}

One of the problems I'm having is with the array up top. It needs to be a global variable but it keeps telling me it's not a constant, even when I declare it as such.

Any help is appreciated, with the rest of the code also.

**EDIT: After updating the code using the comments below, here is the new code. I have a few errors still there and would appreciate help dealing with them. 1) Warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] int j = (int)arg; 2)Warning: cast to pointer from integer of different size [Wint - to - pointer - cast] pthread_create(.......... , (void*)i);

NEW CODE:

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

//global variables
int N, T;
double *vsum;

//pie function
void* pie_runner(void* arg)
{
    long j = (long)arg;    
    //double *limit_ptr = (double*) arg;
    //double j = *limit_ptr;

    //for(int i = (j-1)*N/T; i < N*(j) /T; i++)
    for(int i = (N/T)*(j-1); i < ((N/T)*(j)); i++)
    {

        if(i % 2 == 0){
            vsum[j] += 4.0/((2*j)*(2*j+1)*(2*j+2)); 
            //printf("vsum %lu = %f\n", j, vsum[j]);
                  }
        else{
            vsum[j] -= 4.0/((2*j)*(2*j+1)*(2*j+2));
            //printf("vsum %lu = %f\n", j, vsum[j]);
                  }


    }

    pthread_exit(0);
}

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

    if(argc != 3) {
        printf("Error: Must send it 2 parameters, you sent %d\n", argc-1);
        exit(1);
    }
    N = atoi(argv[1]);
    T = atoi(argv[2]); 

    vsum = malloc((T+1) * sizeof(*vsum));
    if(vsum == NULL) {
        fprintf(stderr, "Memory allocation problem\n");
        exit(1);
    }

    if(N <= T) {
        printf("Error: Number of terms must be greater then number of threads.\n");
        exit(1);    
    }

    for(int p=1; p<=T; p++)        //initialize array to 0
    {
        vsum[p] = 0;
    }

    double pie = 3.0;
    //launch threads
    pthread_t tids[T+1];

    for(long i = 1; i<=T; i++)
    {
        pthread_attr_t attr;
        pthread_attr_init(&attr);
        pthread_create(&tids[i], &attr, pie_runner, (void*)i);
    }

    //wait for threads...
    for(int k = 1; k<=T; k++)
    {
        pthread_join(tids[k], NULL);
    }

    for(int x=1; x<=T; x++)
    {
        pie += vsum[x];
    }

    printf("pi computed with %d terms in %d threads is %.20f\n", N, T, pie);

    //printf("pi computed with %d terms in %d threads is %20f\n", N, T, pie);

    free(vsum);
}

Values not working:

./pie1 2 1
pi computed with 2 terms in 1 threads is 3.00000000000000000000
./pie1 3 1
pi computed with 3 terms in 1 threads is 3.16666666666666651864
 ./pie1 3 2
pi computed with 3 terms in 2 threads is 3.13333333333333330373
 ./pie1 4 2
pi computed with 4 terms in 2 threads is 3.00000000000000000000
 ./pie1 4 1
pi computed with 4 terms in 1 threads is 3.00000000000000000000
 ./pie1 4 3
pi computed with 4 terms in 3 threads is 3.14523809523809516620
 ./pie1 10 1
pi computed with 10 terms in 1 threads is 3.00000000000000000000
 ./pie1 10 2
pi computed with 10 terms in 2 threads is 3.13333333333333330373
 ./pie1 10 3
pi computed with 10 terms in 3 threads is 3.14523809523809516620
 ./pie1 10 4
pi computed with 10 terms in 4 threads is 3.00000000000000000000
./pie1 10 5
pi computed with 10 terms in 5 threads is 3.00000000000000000000
./pie1 10 6
pi computed with 10 terms in 6 threads is 3.14088134088134074418
 ./pie1 10 7
pi computed with 10 terms in 7 threads is 3.14207181707181693042
./pie1 10 8
pi computed with 10 terms in 8 threads is 3.14125482360776464574
 ./pie1 10 9
pi computed with 10 terms in 9 threads is 3.14183961892940200045
./pie1 11 2
pi computed with 11 terms in 2 threads is 3.13333333333333330373
 ./pie1 11 4
pi computed with 11 terms in 4 threads is 3.00000000000000000000

Answer 1

There are numerous problems with that code. Your specific problem is that, in C, variable length arrays (VLAs) are not permitted at file scope.

So, if you want that array to be dynamic, you will have to declare the pointer to it and allocate it yourself:

int N, T;
double *vsum;

and then, in main() after T has been set:

vsum = malloc (T * sizeof(*vsum));
if (vsum == NULL) {
    fprintf (stderr, "Memory allocation problem\n");
    exit (1);
}

remembering to free it before exiting (not technically required but good form anyway):

free (vsum);

Among the other problems:

---

1/ There is no !> operator in C, I suspect the line should be:

if (N > T) {

rather than:

if (N !> T) {

---

2/ To get the arguments from the command line, change:

N = atoi[1];
T = atoi[2];

into:

N = atoi(argv[1]);
T = atoi(argv[2]);

---

3/ The comparison operator is ==, not =, so you need to change:

if(i %2 =0)

into:

if (i % 2 == 0)

---

4/ Your error message about not having enough parameters needs to use %d rather than %s, as argc is an integral type:

printf ("Error: Must send it 2 parameters, you sent %d\n", argc-1);

Ditto for your calculation message at the end (and fixing the %k for the floating point value):

printf ("pi computed with %d terms in %d threads is %.20f\n", N, T, pie);

---

5/ You pass an integer pointer into your thread function but there are two problems with that.

The first is that you then extract it into a double j, which cannot be used as an array index. If it's an integer being passed in, it should be turned back into an integer.

The second is that there is no guarantee the new thread will extract the value (or even start running its code at all) before the main thread changes that value to start up another thread. You should probably just convert the integer to a void * directly rather than messing about with integer pointers.

To fix both those, use this when creating the thread:

pthread_create (&tids[i], &attr, pie_runner, (void*)i);

and this at the start of the thread function:

int j = (int) arg;

If you get warnings or experience problems with that, it's probably because your integers and pointers are not compatible sizes. In that case, you could try something like:

pthread_create (&tids[i], &attr, pie_runner, (void*)(intptr_t)i);

though I'm not sure that will work any better.

Alternatively (though it's a bit of a kludge), stick with your pointer solution and just make sure there's no possibility of race conditions (by passing a unique pointer per thread).

First, revert the thread function to receiving its value by a pointer:

int j = *((int*) arg);

Then, before you start creating threads, you need to create a thread integer array and, for each thread created, populate and pass the (address of the) correct index of that array:

int tvals[T];                   // add this line.
for (int i = 0; i < T; i++) {
    tvals[i] = i;               // and this one.
    pthread_attr_t attr;
    pthread_attr_init (&attr);
    pthread_create (&tids[i], &attr, pie_runner, &(tvals[i]));
}

That shouldn't be too onerous unless you have so many threads the estra array will be problematic. But, if you have that many threads, you're going to have far greater problems.

---

6/ Your loop in the thread incorrectly incremented j rather than i. Since this is the same area touched by the following section, I'll correct it there.

---

7/ The use of integers in what is predominantly a floating point calculation means that you have to arrange your calculations so that they don't truncate divisions, such as 10 / 4 -> 2 where it should be 2.5.

That means the loop in the thread function should be changed as follows (including incrementing i as in previous point):

for (int i = j*N/T; i <= N * (j+1) / T - 1; i++)
    if(i % 2 == 0)
        vsum[j] += 4.0/((2*j)*(2*j+1)*(2*j+2));
    else
        vsum[j] -= 4.0/((2*j)*(2*j+1)*(2*j+2));

---

With all those changes, you get a reasonably sensible result:

$ ./picalc 100 101
pi computed with 100 terms in 101 threads is 3.14159241097198238535

Answer 2

Two problems with that array: The first is that T is not a compile-time constant, which it needs to be if you're programming in C++. The second is that T is initialized to zero, meaning the array will have a size of zero and all indexing of the array will be out of bounds.

You need to allocate the array dynamically once you have read T and know the size. In C you use malloc for that, in C++ you should use std::vector instead.