-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
Add means to handle negative capabilities in the Clang Thread Safety annotations #19249
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f8213c0
@ajtowns @practicalswift @ryanofsky Mind reviewing this PR? |
Concept ACK. Please adjust the minimum clang version to https://releases.llvm.org/3.6.0/tools/clang/docs/ThreadSafetyAnalysis.html#negative This should be uncontroversial, because even debian oldoldstable comes with clang-4 https://packages.debian.org/jessie/clang-4.0 |
Concept ACK, but we'll have to opt-in to Can you add a temporary test case to verify that Travis catches an example incorrect lock? |
This feature lives since the Clang version 3.5.0: llvm/llvm-project@3efd049 Please note that the Clang version checking is required not only for this PR changes but also for all subsequent adding of Would it be a rational decision to bump the minimum required Clang version from the current 3.3 to 3.5? |
Yes, as I said this should be uncontroversial, as all supported operating systems come with at least clang-4.0 (oldoldstable debian) |
Not exactly :)
Without
Done in #19251. |
@practicalswift |
https://packages.debian.org/jessie/clang:
|
Also, I agree with @practicalswift that this should be added to the default flags. Otherwise it misses the whole point of helping developers write better code. There should not be a single negative annotation in our code, so there can't be any warnings. What am I missing? |
All classes and free functions that use mutexes are required to be modernized as it done with the At the end of this way there is a hope to get rid of |
@MarcoFalke @practicalswift The next stage is as @practicalswift said:
|
Just to clarify with an example why 556 size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
557 {
558 LOCK(m_addrman_mutex); // TODO: Cache this in an atomic to avoid this overhead
559 return sizeNonLockerHelper();
560 }
561
562 void f()
563 {
564 LOCK(m_addrman_mutex);
565 size();
566 } When this code is compiled with just
So what does
|
Does it warn even for recursive mutexes? |
Approach ACK f8213c0 This simply adds a member function and doesn't need to bump the minimum compiler version. I don't see a reason to disallow this annotation for temporary testing by not having the member function. |
If we're even slightly worried about supporting old compilers, why not add a new macro to threadsafety.h? #if defined(__clang__) && __clang_major__ >= 4
#define EXCLUSIVE_LOCKS_FORBIDDEN(a) __attribute__((exclusive_locks_required(!a)))
#else
#define EXCLUSIVE_LOCKS_FORBIDDEN(a)
#endif // clang 4
class CAddrMan {
...
size_t size() const EXCLUSIVE_LOCKS_FORBIDDEN(m_addrman_mutex);
...
}; It seems if you add member functions in CAddrMan that call things that forbid holding m_addrman_mutex, then you have to also mark those member functions as forbidding holding m_addrman_mutex, but for functions outside of those modules, you don't have to. If I do: +void wtf(CConnman& c, const CService &addr, ServiceFlags nServices)
+{
+ LOCK(c.addrman.m_addrman_mutex);
+ c.SetServices(addr, nServices);
+}
+
void CConnman::SetServices(const CService &addr, ServiceFlags nServices)
{
addrman.SetServices(addr, nServices);
} (and make addrman and m_addrman_mutex public members) then I don't get a compile time warning. Based on the google paper I think the what's happening is "The analyzer assumes that it holds a negative capability for any object that is not defined within the current lexical scope" -- so for mutexes that are private class members, it should be fine, I think; and also for module-specific globals. But I think it will not work right for cross-file globals like cs_main? |
Yes, it does. See: |
Agree to postpone the minimum compiler version bumping until C++17. In any case, Clang just ignores unknown annotations. FWIW, I've made some retro tests:
|
Agree with you. |
Currently all thread-safety macros map to attributes with identical names: #define FOO() __attribute__((foo)) That is good because somebody who is familiar with the clang attributes, but not with bitcoin core specifics does not have to look up what a macro does in |
This looks like an uncontroversial addition of a member function with no risk of breaking anything, so I am planning to merge this in the coming days. To ask more c++ experienced people, @ryanofsky do you think this could break anything? |
… Clang Thread Safety annotations f8213c0 Add means to handle negative capabilities in thread safety annotations (Hennadii Stepanov) Pull request description: This commit is separated from bitcoin#19238, and it adds support of [Negative Capabilities](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) in the Clang Thread Safety Analysis attributes. > Negative requirements are an alternative `EXCLUDES` [`LOCKS_EXCLUDED`] that provide a stronger safety guarantee. A negative requirement uses the `REQUIRES` [`EXCLUSIVE_LOCKS_REQUIRED`] attribute, in conjunction with the ! operator, to indicate that a capability should not be held. Examples of usage: - bitcoin#19238 (for a class) - https://github.com/hebasto/bitcoin/tree/200610-addrman-tsn (for the whole code base) ACKs for top commit: MarcoFalke: Approach ACK f8213c0 vasild: ACK f8213c0 Tree-SHA512: 86d992826b87579661bd228712ae5ee6acca6f70b885ef7e96458974eac184e4874a525c669607ba6b6c861aa4806409a8792d100e6914c858bcab43d31cfb1b
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
…ead safety annotations
merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements
Summary: > This commit is separated from #19238, and it adds support of Negative Capabilities in the Clang Thread Safety Analysis attributes. > > > Negative requirements are an alternative EXCLUDES [LOCKS_EXCLUDED] that provide a stronger safety guarantee. A negative requirement uses the REQUIRES [EXCLUSIVE_LOCKS_REQUIRED] attribute, in conjunction with the ! operator, to indicate that a capability should not be held. This is a backport of [[bitcoin/bitcoin#19249 | core#19249]] Test Plan: `ninja` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9937
This commit is separated from #19238, and it adds support of Negative Capabilities in the Clang Thread Safety Analysis attributes.
Examples of usage: