According to
https://wiki.sei.cmu.edu/confluence/display/c/CON40-C.+Do+not+refer+to+an+atomic+variable+twice+in+an+expression
bugprone-do-not-refer-atomic-twice checker is created. The checker finds atomic variable which is referred twice in an expression.
Details
Details
- Reviewers
aaron.ballman alexfh hokein njames93
Diff Detail
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
Comment Actions
The checker can't solve the whole SEI CERT rule, but can tighten the scope by guarding atomic variables which occurs in the both sides of a binary operator. Solving the whole rule may be a task for the CSA.
Comment Actions
In my mind this check is definitely in the realm of the static analyser, clang-tidy just isn't designed for this.
| clang-tools-extra/docs/clang-tidy/checks/list.rst | ||
|---|---|---|
| 144 | All these artefacts from the add-new-check script don't need to be in here. | |
| clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp | ||
| 80 | don't need a main call in the test case | |
| clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp | ||
|---|---|---|
| 22 | I have run the tests and I found that the last bad test case is not detected. I have played with clang-query to investigate and I have found that the main reason is the other declRefExpr that matches is not on the RHS but also on LHS, just one level deeper in the AST. You could patch it further, but IMO that would surely become way too complex to understand and maintain in the long run. What I propose is to not try to solve everything with ASTMatchers. This problem is inherently symmetric, and there is no way match based on the complexity of an expression, which could be used a way to deduplicate results. I propose the following: use a matcher like this: expr(
hasDescendant(
declRefExpr(
to(
varDecl(
hasType(
hasUnqualifiedDesugaredType(
atomicType()
)
)
).bind("varDecl")
)
).bind("one")
),
hasDescendant(
declRefExpr(
to(
varDecl(
equalsBoundNode("varDecl")
)
),
unless(equalsBoundNode("one"))
).bind("other")
)
);This will match conflicting DeclRefs with every pair-combinations. Then you could use bound nodes one and other and a set data structure to deduplicate the reports as needed. Note you should also watch out for expressions where 3 references exist to the same VarDecl. That case would emit 6 diagnostics, 4 references 12, etc. (Also 4 references could result in the first 2 then the second two being processed, and you would still have duplication. Handling this is probably not really worth it, as I would think this case is unlikely in real-world code.) I would consider this approach more customizable because you decide how to deduplicate the results. It is also more understandable, as deduplicating with Matchers is not really idiomatic in this symmetric case IMHO. I would not go as far as to say anything about performance, but I would be surprised to find a good, straightforward filtering solution using Matchers. | |
| clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp | ||
|---|---|---|
| 11 | Can you cover std::atomic as well? | |
| 11 | We deprecated ATOMIC_VAR_INIT in C++20 and C17, I wouldn't use it here (even if it's just your own macro). | |
| 78 | Can you check that non-atomic-accesses to the variable also work, for example taking the atomic's address, using it in unevaluated sizeof / offsetof, etc. | |
Comment Actions
+1, I think this probably should be handled in the static analyzer if we want to catch the truly problematic cases.
| clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp | ||
|---|---|---|
| 78 | Additionally, this may be one of the rare cases where we do want to warn even if the problem happens due to macro expansion, so there should be some tests involving atomic uses within macros. A degenerate case we should be careful of is the ?: operator: atomic ? atomic : non_atomic; // bad, two reads whatever ? atomic : atomic; // fine, only one read | |
I have run the tests and I found that the last bad test case is not detected. I have played with clang-query to investigate and I have found that the main reason is the other declRefExpr that matches is not on the RHS but also on LHS, just one level deeper in the AST.
You could patch it further, but IMO that would surely become way too complex to understand and maintain in the long run. What I propose is to not try to solve everything with ASTMatchers. This problem is inherently symmetric, and there is no way match based on the complexity of an expression, which could be used a way to deduplicate results.
I propose the following:
use a matcher like this:
expr( hasDescendant( declRefExpr( to( varDecl( hasType( hasUnqualifiedDesugaredType( atomicType() ) ) ).bind("varDecl") ) ).bind("one") ), hasDescendant( declRefExpr( to( varDecl( equalsBoundNode("varDecl") ) ), unless(equalsBoundNode("one")) ).bind("other") ) );This will match conflicting DeclRefs with every pair-combinations. Then you could use bound nodes one and other and a set data structure to deduplicate the reports as needed. Note you should also watch out for expressions where 3 references exist to the same VarDecl. That case would emit 6 diagnostics, 4 references 12, etc. (Also 4 references could result in the first 2 then the second two being processed, and you would still have duplication. Handling this is probably not really worth it, as I would think this case is unlikely in real-world code.)
I would consider this approach more customizable because you decide how to deduplicate the results. It is also more understandable, as deduplicating with Matchers is not really idiomatic in this symmetric case IMHO. I would not go as far as to say anything about performance, but I would be surprised to find a good, straightforward filtering solution using Matchers.