CreateThread parameter value changes unexpectedly

Question

Im trying to create 4 threads to run a function simultaneously at my 4 CPU cores. The function i call will change some loop offsets depending on val variable value.

I tried this, but it doesnt increase the val counter properly, some of the threads report same values, it seems to change randomly:

int val = 1;
threads[0] = CreateThread(0, 0, my_thread_1, &val, 0, 0);
val++;
threads[1] = CreateThread(0, 0, my_thread_1, &val, 0, 0);
val++;
threads[2] = CreateThread(0, 0, my_thread_1, &val, 0, 0);
val++;
threads[3] = CreateThread(0, 0, my_thread_1, &val, 0, 0);

But this seems to work just fine:

int val1 = 1;
int val2 = 2;
int val3 = 3;
int val4 = 4;
threads[0] = CreateThread(0, 0, my_thread_1, &val1, 0, 0);
threads[1] = CreateThread(0, 0, my_thread_1, &val2, 0, 0);
threads[2] = CreateThread(0, 0, my_thread_1, &val3, 0, 0);
threads[3] = CreateThread(0, 0, my_thread_1, &val4, 0, 0);

What could be the reason for this, and how is it properly done to give some parameter to a thread?

This is my function:

DWORD WINAPI my_thread_1(void *params){ 
    int val = *(int *)params;
...
}

Answer 1

This fails simply because in your first example you are passing the pointer to the same memory location for all 4 threads. The fact that you incremented the value before passing the pointer is inconsequential, as the memory location stays the same.

In your second example you instead pass 4, mutually exclusive pointers to the 4 threads. Therefore, the threads all read independent values.

You could restructure your code slightly to help with maintainability and flexibility:

int threadData[NTHREADS]; /* in the future this could be an array of structs */
HANDLE threads[NTHREADS];
int tt;

for (tt = 0; tt < NTHREADS; ++tt)
{
    threadData[tt] = tt + 1; /* placeholder for actual logic */
    threads[tt] = CreateThread(
        NULL,            /* lpThreadAttributes */
        0,               /* dwStackSize */
        my_thread_1,     /* lpStartAddress */
        &threadData[tt], /* lpParameter: each thread will receive its own data */
        0,               /* dwCreationFlags */
        NULL             /* lpThreadId */);
}

/* Since threadData and threads are local to the enclosing scope,
 * we must wait for them to finish here to ensure we don't read
 * data we no longer own
 */
WaitForMultipleObjects(NTHREADS, threads, TRUE, INFINITE);

Answer 2

In the first case, you're passing the same object to all threads. In the second case, you're passing different objects.

In both cases, there is a potential problem that if the function which creates threads returns, then all the created threads will have pointer to int which doesn't exist anymore, because all int objects are local objects to the function which created the threads. This invokes undefined behavior.

So if the function doesn't wait and returns, then in that case, you shouldn't pass pointer to local object, instead pass dynamically allocated object:

int *v1 = new int;
threads[0] = CreateThread(0, 0, my_thread_1, v1 , 0, 0);