Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang thread safety analysis does not take into account assert_exclusive_lock attribute #123512

Open
krinkinmu opened this issue Jan 19, 2025 · 0 comments

Comments

@krinkinmu
Copy link

It seems like clang version 18 (specifically I tested it on 18.1.8) fails to take into account assert_exclusive_lock/assert_capability annotations.

Here is a minimal example I came up with that reproduces the issue for me:

#define GUARDED_BY(x) __attribute__((guarded_by(x)))
#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
#define LOCKABLE __attribute__((lockable))
#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__)))
#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__)))
#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__((assert_exclusive_lock(__VA_ARGS__)))

struct LOCKABLE Lock {
  void lock() EXCLUSIVE_LOCK_FUNCTION() {}
  void unlock() UNLOCK_FUNCTION() {}
  void assert() ASSERT_EXCLUSIVE_LOCK(this) {}
};

struct ComplexValue {
};

struct Parent {
  Lock lock;
};

struct Child {
  Child(Parent& p) : p(p) {}
  const ComplexValue& value() ASSERT_EXCLUSIVE_LOCK(p.lock) {
    p.lock.assert();
    return data;
  }

  Parent &p;
  ComplexValue data GUARDED_BY(p.lock);
};

When compiling this code with the following command:

clang++-18 -c -Wthread-safety -Wthread-safety-reference-return -Werror -std=c++20 test.cc

I get an error:

test.cc:25:12: error: returning variable 'data' by reference requires holding mutex 'p.lock' [-Werror,-Wthread-safety-reference-return]
   25 |     return data;
      |            ^
1 error generated.

I understand that returning a reference to a protected variable in general might be incorrect, but in this case before returning the reference I call a function with assert_exclusive_lock annotation, so by the time we get to the return statement, clang should be able to figure out that the lock is being held.

Naturally, replacing assert_exclusive_lock with exclusive_locks_required attribute eliminates the warning. However, in more complex cases it might be hard for clang to figure out what locks are held in what contexts, so I want to help clang with putting asserts like this to let clang know that the lock is actually held, but it does not seem that clang takes those into account or derives anything from exclusive_locks_required, which seems wrong.

Is my understanding of the purpose of exclusive_locks_required annotation somehow incorrect?

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jan 19, 2025
@EugeneZelenko EugeneZelenko added clang:analysis and removed clang Clang issues not falling into any other category labels Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants