Pthreads program produces wrong outputs and correct outputs sometimes

Question

I am learning parallel programming using Pthreads. I wrote the following simple program as a try. It takes two vectors from stdin (the user specifies the length as command line argument) then it adds them, subtracts them, multiply them element-wise and divide them element-wise by creating a thread for each operation of the four. the problem is sometimes the code works fine and when i use the same input again it just prints zeros.

Why does it behave like this?

I am using Ubuntu 14.04 on virtual machine.

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

#define loop(i, n) for (i = 0; i < n; i++)

int thread_count, n;
int *a;
int *b;
int** results; // holds the four output vectors
void* math(void* opl);


int main(int argc, char* argv[]){
    int thread = 0, i;
    pthread_t* thread_handles;

    n = strtol(argv[1], NULL, 10);

    results = malloc(4 * sizeof(int*));
    a = malloc(n * sizeof(int));
    b = malloc(n * sizeof(int));

    loop(i, n)
        scanf("%d", a + i);

    loop(i, n)
        scanf("%d", b + i);

    thread_handles = malloc(4*sizeof(pthread_t));

    loop(thread, 4){
        results[thread] = malloc(n * sizeof(int));
        pthread_create(&thread_handles[thread], NULL, math, (void *)thread);
    }

    printf("Hello from the main thread\n");

    loop(thread, 4);
        pthread_join(thread_handles[thread], NULL);

    loop(thread, 4){
        printf("Results of operation %d:\n", thread); 
        loop(i, n)
            printf("%d ", results[thread][i]);
        printf("\n");
       free(results[thread]);
    }

    free(thread_handles);
    free(a);
    free(b);
    free(results);

   return 0;    
}

void* math(void* op1){
    int op = (int) op1, i;

    printf("%d ", op);
    if (op == 0)
        loop(i, n)
             results[op][i] = a[i] + b[i];
    else if (op == 1)
         loop(i, n)
             results[op][i] = a[i] - b[i];
    else if (op == 2)
         loop(i, n)
             results[op][i] = a[i] * b[i];
    else
        loop(i, n)
            results[op][i] = a[i] / b[i];



    return NULL;
}

If the user fails to provide an argument to the program, this program should SEGFAULT. You should check argc before indexing into argv. — tweej, Nov 02 '15 at 01:50
I also suggest you look at this [answer](http://stackoverflow.com/a/24090532/3857942) about casting to (void *) . I mention this not because it is related to your problem, but something you may find if you compile in a different environment. — Michael Petch, Nov 02 '15 at 02:14
such wonderfully meaningful variable names, like 'a' and 'b' *sigh* . This macro: `loop(i, n)` is useless, clutters the code, and makes it harder to understand. Strongly suggest replacing those macro invocations with the actual 'for()` statement. — user3629249, Nov 02 '15 at 23:06
When calling the malloc() or calloc() or realloc() functions always check (!=NULL) the returned value, before using that value, to assure the operation was successful — user3629249, Nov 02 '15 at 23:07
a much better way to write the `math()` function would be to use a `switch()` statement, which could then have a `default:` case for when the operator is not one of the 4 valid values — user3629249, Nov 02 '15 at 23:09
the posted code leaks memory in a big way. When freeing 'results[]' use: `for( int i=0; i<4; i++) { free( results[i]); }` then `free( results );` as code has to free each returned value from malloc(), not just one of the returned values from malloc — user3629249, Nov 02 '15 at 23:12
Suggest, for maintainability and for the purpose of keeping data and functionality contained to the minimal 'scope', to declare the variables 'a', 'b', 'n', 'results', etc inside the `main()` function add pass them as parameters when needed. — user3629249, Nov 02 '15 at 23:19
for readability/understandability/ease of documentation, code only one statement per line, and only declare one variable per statement. — user3629249, Nov 02 '15 at 23:20
When compiling, always enable all the warnings, (for gcc, at a minimum use: `-Wall -Wextra -pedantic` ), then fix the warnings. (for the posted code there are several, two of which are severe) — user3629249, Nov 02 '15 at 23:27
the posted code contains some `magic` numbers. `magic` numbers make the code much harder to understand, more difficult to debug and/or maintain. Suggest using #define or an enum to give the numeric values meaningful names, then use those meaningful names throughout the code, One of the `magic` numbers is 4. — user3629249, Nov 02 '15 at 23:35

score 3 · Accepted Answer · edited Nov 02 '15 at 02:02

3

The problem is induced by the godawful loop define you've learned. Please unlearn it!

#define loop(i, n) for (i = 0; i < n; i++)

This is the problem:

loop(thread, 4);
//             ^ BAD! Putting a semicolon here makes the loop empty.

The correct version using the awful define you've chosen is:

loop(thread, 4)

A better way would be just to say what you mean!

for(i=0; i<n; ++i)
{
   // Do Stuff
}

The problem results in pthread_join being called just once, and then the main thread proceeds on. This likely occurs before all threads have completed, and provided their output in the results variable. No good!

edited Nov 02 '15 at 02:02

DaoWen

32,589
6
74
101

answered Nov 02 '15 at 01:46

tweej

832
4
16

The problem isn't caused by the `loop` macro (although, I agree, using a macro like that will probably do more harm than good for readability). The problem is just the extra semicolon in `loop(thread, 4);`. The OP would have had the same problem with a semicolon before the body of normal for loop: `for (i=0; i – DaoWen Nov 02 '15 at 02:00
@DaoWen I agree with your assessment. I was careful to say *induced* by the macro, not that the macro was the root cause. – tweej Nov 02 '15 at 02:04
1

I would have been OK with 'macros are crap' – Martin James Nov 02 '15 at 02:14
1

"*The problem results in pthread_join being called just once*" and this one call accesses `thread_handles` out of bounds (using index 5) and with this invokes undefined behaviour from which on anything could happen ... – alk Nov 02 '15 at 12:11
This solved my problem. I will never ever use macros again. – Abd ElRahman Abbas Nov 02 '15 at 13:01
There are many very useful scenarios for macros, using them to insert code is NOT one of the useful scenarios. However, using them to give meaningful names to numeric values, etc is a very useful scenario – user3629249 Nov 02 '15 at 23:29

Pthreads program produces wrong outputs and correct outputs sometimes

1 Answers1