[clang][ThreadSafety]: fix discrepancy between capability attributes #139343
+2
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix the case where release_generic_capability did not correctly release when used as a reverse capability as enabled by commit 6a68efc.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
theuni
changed the title
|
@llvm/pr-subscribers-clang Author: Cory Fields (theuni) ChangesFix the case where I noticed this when trying to implement a reverse lock. My my project still uses the old I'm not at all familiar with the clang code so I have no idea if this is the correct fix, but it fixes my problem. Hopefully it's helpful. Here's a minimal reproducer: class __attribute__((capability(""))) Mutex {
public:
const Mutex& operator!() const { return *this; }
};
class __attribute__((scoped_lockable)) MutexLock {
public:
MutexLock(Mutex *mu) __attribute__((acquire_capability(mu))) {}
~MutexLock() __attribute__((release_capability())){}
};
class __attribute__((scoped_lockable)) MutexLockOld {
public:
MutexLockOld(Mutex *mu) __attribute__((exclusive_lock_function(mu))) {}
~MutexLockOld() __attribute__((unlock_function())){}
};
class __attribute__((scoped_lockable)) MutexLockGeneric {
public:
MutexLockGeneric(Mutex *mu) __attribute__((acquire_capability(mu))) {}
~MutexLockGeneric() __attribute__((release_generic_capability())){}
};
class __attribute__((scoped_lockable)) MutexUnlock {
public:
MutexUnlock(Mutex *mu) __attribute__((release_capability(mu))){}
~MutexUnlock() __attribute__((release_capability())){}
};
class __attribute__((scoped_lockable)) MutexUnlockOld {
public:
MutexUnlockOld(Mutex *mu) __attribute__((unlock_function(mu))){}
~MutexUnlockOld() __attribute__((unlock_function())){}
};
class __attribute__((scoped_lockable)) MutexUnlockGeneric {
public:
MutexUnlockGeneric(Mutex *mu) __attribute__((release_generic_capability(mu))){}
~MutexUnlockGeneric() __attribute__((release_generic_capability())){}
};
Mutex mut;
void req() __attribute__((requires_capability(mut))){}
void req2() __attribute__((exclusive_locks_required(mut))){}
void not_req() __attribute__((requires_capability(!mut))){}
void good()
{
MutexLock lock(&mut);
req();
{
MutexUnlock reverse_lock(&mut);
not_req();
}
req();
}
void bad()
{
MutexLockGeneric lock(&mut);
req();
{
MutexUnlockGeneric reverse_lock(&mut);
not_req();
}
req();
req2();
}
void bad2()
{
MutexLockOld lock(&mut);
req();
{
MutexUnlockOld reverse_lock(&mut);
not_req();
}
req();
req2();
}Result: clangtest.cpp:67:5: warning: calling function 'req' requires holding 'mut' exclusively [-Wthread-safety-analysis]
67 | req();
| ^
clangtest.cpp:68:5: warning: calling function 'req2' requires holding 'mut' exclusively [-Wthread-safety-analysis]
68 | req2();
| ^
clangtest.cpp:79:5: warning: calling function 'req' requires holding 'mut' exclusively [-Wthread-safety-analysis]
79 | req();
| ^
clangtest.cpp:80:5: warning: calling function 'req2' requires holding 'mut' exclusively [-Wthread-safety-analysis]
80 | req2();Full diff: https://github.com/llvm/llvm-project/pull/139343.diff 1 Files Affected:
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 7e86af6b4a317..a963bcda0c2d0 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2026,6 +2026,8 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
ScopedEntry->addExclusiveUnlock(M);
for (const auto &M : SharedLocksToRemove)
ScopedEntry->addSharedUnlock(M);
+ for (const auto &M : GenericLocksToRemove)
+ ScopedEntry->addExclusiveUnlock(M);
Analyzer->addLock(FSet, std::move(ScopedEntry));
}
}
|
|
@llvm/pr-subscribers-clang-analysis Author: Cory Fields (theuni) ChangesFix the case where I noticed this when trying to implement a reverse lock. My my project still uses the old I'm not at all familiar with the clang code so I have no idea if this is the correct fix, but it fixes my problem. Hopefully it's helpful. Here's a minimal reproducer: class __attribute__((capability(""))) Mutex {
public:
const Mutex& operator!() const { return *this; }
};
class __attribute__((scoped_lockable)) MutexLock {
public:
MutexLock(Mutex *mu) __attribute__((acquire_capability(mu))) {}
~MutexLock() __attribute__((release_capability())){}
};
class __attribute__((scoped_lockable)) MutexLockOld {
public:
MutexLockOld(Mutex *mu) __attribute__((exclusive_lock_function(mu))) {}
~MutexLockOld() __attribute__((unlock_function())){}
};
class __attribute__((scoped_lockable)) MutexLockGeneric {
public:
MutexLockGeneric(Mutex *mu) __attribute__((acquire_capability(mu))) {}
~MutexLockGeneric() __attribute__((release_generic_capability())){}
};
class __attribute__((scoped_lockable)) MutexUnlock {
public:
MutexUnlock(Mutex *mu) __attribute__((release_capability(mu))){}
~MutexUnlock() __attribute__((release_capability())){}
};
class __attribute__((scoped_lockable)) MutexUnlockOld {
public:
MutexUnlockOld(Mutex *mu) __attribute__((unlock_function(mu))){}
~MutexUnlockOld() __attribute__((unlock_function())){}
};
class __attribute__((scoped_lockable)) MutexUnlockGeneric {
public:
MutexUnlockGeneric(Mutex *mu) __attribute__((release_generic_capability(mu))){}
~MutexUnlockGeneric() __attribute__((release_generic_capability())){}
};
Mutex mut;
void req() __attribute__((requires_capability(mut))){}
void req2() __attribute__((exclusive_locks_required(mut))){}
void not_req() __attribute__((requires_capability(!mut))){}
void good()
{
MutexLock lock(&mut);
req();
{
MutexUnlock reverse_lock(&mut);
not_req();
}
req();
}
void bad()
{
MutexLockGeneric lock(&mut);
req();
{
MutexUnlockGeneric reverse_lock(&mut);
not_req();
}
req();
req2();
}
void bad2()
{
MutexLockOld lock(&mut);
req();
{
MutexUnlockOld reverse_lock(&mut);
not_req();
}
req();
req2();
}Result: clangtest.cpp:67:5: warning: calling function 'req' requires holding 'mut' exclusively [-Wthread-safety-analysis]
67 | req();
| ^
clangtest.cpp:68:5: warning: calling function 'req2' requires holding 'mut' exclusively [-Wthread-safety-analysis]
68 | req2();
| ^
clangtest.cpp:79:5: warning: calling function 'req' requires holding 'mut' exclusively [-Wthread-safety-analysis]
79 | req();
| ^
clangtest.cpp:80:5: warning: calling function 'req2' requires holding 'mut' exclusively [-Wthread-safety-analysis]
80 | req2();Full diff: https://github.com/llvm/llvm-project/pull/139343.diff 1 Files Affected:
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 7e86af6b4a317..a963bcda0c2d0 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2026,6 +2026,8 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
ScopedEntry->addExclusiveUnlock(M);
for (const auto &M : SharedLocksToRemove)
ScopedEntry->addSharedUnlock(M);
+ for (const auto &M : GenericLocksToRemove)
+ ScopedEntry->addExclusiveUnlock(M);
Analyzer->addLock(FSet, std::move(ScopedEntry));
}
}
|
|
Ping. I'm unsure of the fix here, but it'd be nice to have a confirmation of the bug it attempts to fix. |
ryanofsky
added a commit
to bitcoin/bitcoin
that referenced
this pull request
Jun 17, 2025
a201a99 thread-safety: fix annotations with REVERSE_LOCK (Cory Fields) aeea5f0 thread-safety: add missing lock annotation (Cory Fields) 832c57a thread-safety: modernize thread safety macros (Cory Fields) Pull request description: This is one of several PRs to cleanup/modernize our threading primitives. While replacing the old critical section locks in the mining code with a `REVERSE_LOCK`, I noticed that our thread-safety annotations weren't hooked up to it. This PR gets `REVERSE_LOCK` working properly. Firstly it modernizes the attributes as-recommended by the [clang docs](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) (ctrl+f for `USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES`). There's a subtle difference between the old `unlock_function` and new `release_capability`, where our `reverse_lock` only works with the latter. I believe this is an upstream bug. I've [reported and attempted a fix here](llvm/llvm-project#139343), but either way it makes sense to me to modernize. The second adds a missing annotation pointed out by a fixed `REVERSE_LOCK`. Because clang's thread-safety annotations aren't passed through a reference to `UniqueLock` as one may assume (see [here](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis) for more details), `cs_main` has to be listed explicitly as a requirement. The last commit actually fixes the `reverse_lock` by making it a `SCOPED_LOCK` and using the pattern [found in a clang test](https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/warn-thread-safety-analysis.cpp#L3126). Though the docs don't describe how to accomplish it, the functionality was added [in this commit](llvm/llvm-project@6a68efc). Due to aliasing issues (see link above), in order to work correctly, the original mutex has to be passed along with the lock, so all existing `REVERSE_LOCK`s have been updated. To ensure that the mutexes actually match, a runtime assertion is added. ACKs for top commit: fjahr: re-ACK a201a99 davidgumberg: reACK a201a99 theuni: Ok, done. Those last pushes can be ignored. ACKs on a201a99 are still fresh. ryanofsky: Code review ACK a201a99. Just dropping 0065b96 and fixing incorrect `reverse_lock::lockname` initialization since last review. TheCharlatan: Re-ACK a201a99 Tree-SHA512: 2755fae0c41021976a1a633014a86d927f104ccbc8014c01c06dae89af363f92e5bc5d4276ad6d759302ac4679fe02a543758124d48318074db1c370989af7a7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the case where
release_generic_capabilitydid not correctly release when used as a reverse capability as enabled by commit 6a68efc.I noticed this when trying to implement a reverse lock.
My my project still uses the old
UNLOCK_FUNCTIONmacro which maps tounlock_function. With that attribute, the MutexUnlock test-case seen here does not work.I'm not at all familiar with the clang code so I have no idea if this is the correct fix, but it fixes my problem. Hopefully it's helpful.
Here's a minimal reproducer:
Result: