Thread-safety analysis produces incorrect results for cleanup functions

[bvanassche]

Building the following code with -Wthread-safety produces compiler warnings while no warnings should be reported:

#include <stdio.h>

struct __attribute__((capability("mutex"))) mutex {
};

static void mutex_lock(struct mutex *m)
    __attribute__((exclusive_lock_function(*m)))
    __attribute__((no_thread_safety_analysis))
{
    puts(__func__);
}

static void mutex_unlock(struct mutex *m)
    __attribute__((unlock_function(*m)))
    __attribute__((no_thread_safety_analysis))
{
    puts(__func__);
}

static void mutex_cleanup(struct mutex **m)
    __attribute__((unlock_function(**m)))
{
    puts(__func__);
    mutex_unlock(*m);
}

int main(void)
{
    struct mutex m = {};
    mutex_lock(&m);
#if 1
    struct mutex *m_ptr __attribute__((cleanup(mutex_cleanup))) = &m;
#else
    mutex_unlock(&m);
#endif
    return 0;
}

The following compiler warnings are reported:

annotated-cleanup.c:32:19: warning: releasing mutex 'm_ptr' that was not held [-Wthread-safety-analysis]
   32 |     struct mutex *m_ptr __attribute__((cleanup(mutex_cleanup))) = &m;
      |                   ^
annotated-cleanup.c:37:1: warning: mutex 'm' is still held at the end of function [-Wthread-safety-analysis]
   37 | }
      | ^
annotated-cleanup.c:30:5: note: mutex acquired here
   30 |     mutex_lock(&m);
      |     ^
2 warnings generated.

The program output shows that both the lock and unlock functions are called:

mutex_lock
mutex_cleanup
mutex_unlock

[nickdesaulniers]

I would guess that these annotations were only tested with C++ destructors when they were first added to clang, and probably don't know to look for/handle cleanup attributes.

[aaronpuchert]

Support for cleanup functions was added by @tbaederr in https://reviews.llvm.org/D152504, and it looks relatively similar to the test case.

The problem might be the mutex variable being local. I think this isn't supported, not even in C++. But that's just guessing.

[aaronpuchert]

Ah, now I see it. You have to lock through the scoped variable: instead of mutex_lock(&m) write mutex_lock(m_ptr).

That's also what the warning message says. The mutex is unlocked via the cleanup function, but we don't do alias analysis.

[bvanassche]

Thanks for the feedback! This variant of the above code does not trigger any compiler warnings on my development setup:

#include <stdio.h>

struct __attribute__((capability("mutex"))) mutex {
};

static void mutex_lock(struct mutex *m)
    __attribute__((exclusive_lock_function(*m)))
    __attribute__((no_thread_safety_analysis))
{
    puts(__func__);
}

static void mutex_unlock(struct mutex *m)
    __attribute__((unlock_function(*m)))
    __attribute__((no_thread_safety_analysis))
{
    puts(__func__);
}

static void mutex_cleanup(struct mutex **m)
    __attribute__((unlock_function(**m)))
{
    puts(__func__);
    mutex_unlock(*m);
}

int main(void)
{
    struct mutex m = {};
    struct mutex *m_ptr __attribute__((cleanup(mutex_cleanup))) = NULL;
    m_ptr = &m;
    mutex_lock(m_ptr);
    return 0;
}