Linux: swapped arguments to `__sync_val_compare_and_swap` make the one-time-init guard a no-op

[Simoleons-d]

Linux: swapped arguments to __sync_val_compare_and_swap make the one-time-init guard a no-op

Summary

On non-Windows platforms, __itt_interlocked_compare_exchange forwards its arguments to
GCC's __sync_val_compare_and_swap in the wrong order. As a result the compare-and-swap
never matches and never writes, and it returns the same value to every caller.

Its only two callers are ITT_MUTEX_INIT_AND_LOCK and ITT_MUTEX_DESTROY, so the
one-time-initialization guard around _ittapi_global.mutex does not exclude anyone.
Every thread that makes its first ITT API call concurrently runs __itt_mutex_init() on
the same pthread_mutex_t and then locks it. Concurrently initializing a mutex that
another thread has already locked is undefined behaviour, and in practice it aborts with
a glibc assertion.

Present in v3.24.4 and unchanged on master (c83553fd). Windows is unaffected.

Affected code

ittapi/src/ittnotify/ittnotify_config.h

Lines 360 to 363 in c83553f

	__itt_interlocked_compare_exchange(volatile long* ptr, long exchange, long comperand)
	{
	return __sync_val_compare_and_swap(ptr, exchange, comperand);
	}

ITT_INLINE long
__itt_interlocked_compare_exchange(volatile long* ptr, long exchange, long comperand)
{
    return __sync_val_compare_and_swap(ptr, exchange, comperand);
}

GCC's builtin is type __sync_val_compare_and_swap(type *ptr, type oldval, type newval):
it writes newval if *ptr == oldval. So exchange is being passed as oldval and
comperand as newval, which is reversed.

The Windows implementation a few lines above is correct, because
InterlockedCompareExchange(Destination, Exchange, Comperand) genuinely does take the
written value before the compared-against value. The non-Windows branch appears to have
reused that argument order without adapting to the builtin's opposite convention.

Effect on ITT_MUTEX_INIT_AND_LOCK

ittapi/src/ittnotify/ittnotify_static.c

Lines 114 to 130 in c83553f

	#define ITT_MUTEX_INIT_AND_LOCK(p) { \
	if (PTHREAD_SYMBOLS) \
	{ \
	if (!p.mutex_initialized) \
	{ \
	if (__itt_interlocked_compare_exchange(&p.atomic_counter, 1, 0) == 0) \
	{ \
	__itt_mutex_init(&p.mutex); \
	p.mutex_initialized = 1; \
	} \
	else \
	while (!p.mutex_initialized) \
	__itt_thread_yield(); \
	} \
	__itt_mutex_lock(&p.mutex); \
	} \
	}

if (!p.mutex_initialized)
{
    if (__itt_interlocked_compare_exchange(&p.atomic_counter, 1, 0) == 0)
    {
        __itt_mutex_init(&p.mutex);
        p.mutex_initialized = 1;
    }
    else
        while (!p.mutex_initialized)
            __itt_thread_yield();
}
__itt_mutex_lock(&p.mutex);

atomic_counter starts at 0. The intent is "compare against 0, write 1, return the
previous value", so the winner observes 0 and everyone else observes 1 and spins.

With the arguments as written, the expansion is __sync_val_compare_and_swap(ptr, 1, 0),
which compares *ptr against 1. Since it is 0 there is never a match, nothing is
ever written, and every caller receives 0. Every thread therefore takes the
"I won the race" branch.

Minimal reproducer

No Intel tooling or hardware required, this is purely the builtin's semantics.

#include <stdio.h>
#include <pthread.h>
static volatile long counter = 0;
static int winners = 0;
static pthread_mutex_t wl = PTHREAD_MUTEX_INITIALIZER;

static void* itt_order(void* a) { (void)a;                 /* as ittapi does */
    if (__sync_val_compare_and_swap(&counter, 1, 0) == 0) {
        pthread_mutex_lock(&wl); winners++; pthread_mutex_unlock(&wl);
    }
    return 0;
}
static void* correct_order(void* a) { (void)a;             /* compare 0, write 1 */
    if (__sync_val_compare_and_swap(&counter, 0, 1) == 0) {
        pthread_mutex_lock(&wl); winners++; pthread_mutex_unlock(&wl);
    }
    return 0;
}
static void run(const char* label, void* (*fn)(void*)) {
    pthread_t t[5]; counter = 0; winners = 0;
    for (int i = 0; i < 5; i++) pthread_create(&t[i], 0, fn, 0);
    for (int i = 0; i < 5; i++) pthread_join(t[i], 0);
    printf("%-26s winners=%d of 5   counter ends at %ld\n", label, winners, counter);
}
int main(void) {
    long r;
    counter = 0; r = __sync_val_compare_and_swap(&counter, 1, 0);
    printf("ittapi  (ptr,1,0) -> returned %ld, counter=%ld\n", r, counter);
    counter = 0; r = __sync_val_compare_and_swap(&counter, 0, 1);
    printf("correct (ptr,0,1) -> returned %ld, counter=%ld\n\n", r, counter);
    run("ittapi argument order:", itt_order);
    run("correct order:", correct_order);
    return 0;
}

gcc -O2 -pthread output:

ittapi  (ptr,1,0) -> returned 0, counter=0
correct (ptr,0,1) -> returned 0, counter=1

ittapi argument order:     winners=5 of 5   counter ends at 0
correct order:             winners=1 of 5   counter ends at 1

All five threads pass the guard.

ITT_MUTEX_DESTROY is also affected

ittapi/src/ittnotify/ittnotify_static.c

Lines 132 to 143 in c83553f

	#define ITT_MUTEX_DESTROY(p) { \
	if (PTHREAD_SYMBOLS) \
	{ \
	if (p.mutex_initialized) \
	{ \
	if (__itt_interlocked_compare_exchange(&p.atomic_counter, 0, 1) == 1) \
	{ \
	__itt_mutex_destroy(&p.mutex); \
	p.mutex_initialized = 0; \
	} \
	} \
	} \

if (__itt_interlocked_compare_exchange(&p.atomic_counter, 0, 1) == 1)

Intended: compare against 1, write 0, return 1 to the caller that wins. Actual expansion
compares against 0, and since atomic_counter is still 0 (see above) it matches, writes
1, and returns 0. The == 1 test fails, so __itt_mutex_destroy() is never reached
and mutex_initialized is never cleared, while atomic_counter is left at 1.

Real-world impact

Observed in a Linux application that starts several worker threads in quick succession,
each calling __itt_thread_set_name() as its first statement. Under load the process
aborts during startup:

pthread_mutex_lock.c:130: ___pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.

Backtrace (symbolized, innermost first):

pthread_mutex_lock+0x29d
__itt_init_ittlib+0x1f7
__itt_thread_set_name_init_3_0                (ittnotify_static.c)
std::thread trampoline -> worker thread entry
start_thread / clone3

That assertion lives on glibc's PTHREAD_MUTEX_TIMED_NP path, which is only taken when
__kind == 0. __itt_mutex_init() creates the mutex as PTHREAD_MUTEX_RECURSIVE, so
the assertion is consistent with one thread locking the mutex while another thread's
redundant pthread_mutex_init() has reset __kind, with __owner still set by the
first. Serializing the first ITT call per process removes the abort.

Environment

• ittapi 3.24.4, and master c83553fd by inspection
• x86_64, GCC, glibc 2.39, Ubuntu 24.04
• Reproducer above is platform-independent for any GCC-compatible compiler

Suggested fix

Swap the two arguments so they match the builtin's convention:

ITT_INLINE long
__itt_interlocked_compare_exchange(volatile long* ptr, long exchange, long comperand)
{
    return __sync_val_compare_and_swap(ptr, comperand, exchange);
}

Alternatively move to __atomic_compare_exchange_n, whose parameter names make the
expected/desired roles explicit and which is less prone to this class of mistake.

Adding a unit test asserting that exactly one of N concurrent threads passes
ITT_MUTEX_INIT_AND_LOCK would catch regressions here.