This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add do-not-refer-atomic-twice check
Needs ReviewPublic

Authored by abelkocsis on Apr 5 2020, 5:25 AM.

Details

Summary

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.

Diff Detail

Event Timeline

abelkocsis created this revision.Apr 5 2020, 5:25 AM

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.

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
91

Please sort by check name.

clang-tools-extra/docs/ReleaseNotes.rst
87

Please insert empty line above.

clang-tools-extra/docs/clang-tidy/checks/bugprone-do-not-refer-atomic-twice.rst
9

Please use double colons in code blocks.

Eugene.Zelenko edited reviewers, added: njames93; removed: jfb, Charusso.Apr 5 2020, 7:55 AM
Eugene.Zelenko added a subscriber: cfe-commits.

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

abelkocsis marked 5 inline comments as done.Apr 5 2020, 8:52 AM
abelkocsis updated this revision to Diff 255170.Apr 5 2020, 8:56 AM

Small fixes in doc and test files.

Would you give me some feedback according to this checker?

gamesh411 added inline comments.
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.

dkrupp added a subscriber: dkrupp.Jul 3 2020, 5:13 AM
whisperity added inline comments.Jul 9 2020, 1:32 AM
clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp
39

atomic variable '%0'

clang-tools-extra/docs/ReleaseNotes.rst
82

atomic variables which are

jfb added inline comments.Jul 9 2020, 8:31 AM
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.

In my mind this check is definitely in the realm of the static analyser, clang-tidy just isn't designed for this.

+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
mgehre removed a subscriber: mgehre.Nov 10 2020, 11:12 AM
abelkocsis marked 5 inline comments as done.

Fixes, std::atomic variables add