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

[abelkocsis]

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.

[abelkocsis]

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]

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

89

Please sort by check name.

clang-tools-extra/docs/ReleaseNotes.rst

78

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.

[njames93]

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

137

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]

Small fixes in doc and test files.

[abelkocsis]

Would you give me some feedback according to this checker?

[gamesh411]

clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp

21

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.

[whisperity]

clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp

38

atomic variable '%0'

clang-tools-extra/docs/ReleaseNotes.rst

82

atomic variables which are

[jfb]

clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp

10

Can you cover std::atomic as well?

10

We deprecated ATOMIC_VAR_INIT in C++20 and C17, I wouldn't use it here (even if it's just your own macro).

77

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.

[aaron.ballman]

In D77493#1962465, @njames93 wrote:

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

77

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

[abelkocsis]

Fixes, std::atomic variables add

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
+#include "DoNotReferAtomicTwiceCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"

         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
+    CheckFactories.registerCheck<DoNotReferAtomicTwiceCheck>(
+        "bugprone-do-not-refer-atomic-twice");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
+  DoNotReferAtomicTwiceCheck.cpp
   DynamicStaticInitializersCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp

clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.h

+//===--- DoNotReferAtomicTwiceCheck.h - clang-tidy --------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DONOTREFERATOMICTWICECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DONOTREFERATOMICTWICECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds atomic variable which is referred twice in an expression.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-do-not-refer-atomic-twice.html
+class DoNotReferAtomicTwiceCheck : public ClangTidyCheck {
+public:
+  DoNotReferAtomicTwiceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DONOTREFERATOMICTWICECHECK_H

clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp

+//===--- DoNotReferAtomicTwiceCheck.cpp - clang-tidy ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DoNotReferAtomicTwiceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void DoNotReferAtomicTwiceCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      declRefExpr(hasType(hasUnqualifiedDesugaredType(atomicType())),
+                  to(varDecl().bind("atomic")),
+                  hasAncestor(binaryOperator(
+                      unless(hasDescendant(atomicExpr())),
+                      hasRHS(hasDescendant(
+                          declRefExpr(to(varDecl(equalsBoundNode("atomic"))))
+                              .bind("rhs"))))),
+                  unless(equalsBoundNode("rhs"))),
+      this);
+}
+
+void DoNotReferAtomicTwiceCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedVar = Result.Nodes.getNodeAs<VarDecl>("atomic");
+  const auto *MatchedRef = Result.Nodes.getNodeAs<DeclRefExpr>("rhs");
+  if (!MatchedRef || !MatchedVar)
+    return;
+  diag(MatchedRef->getExprLoc(),
+       "Do not refer to '%0' atomic variable twice in an expression")
+      << MatchedVar->getName();
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../bugprone/BadSignalToKillThreadCheck.h"
+#include "../bugprone/DoNotReferAtomicTwiceCheck.h"
 #include "../bugprone/ReservedIdentifierCheck.h"
 #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h"
 #include "../bugprone/UnhandledSelfAssignmentCheck.h"

     // CON
     CheckFactories.registerCheck<bugprone::SpuriouslyWakeUpFunctionsCheck>(
         "cert-con36-c");
+    CheckFactories.registerCheck<bugprone::DoNotReferAtomicTwiceCheck>(
+        "cert-con40-c");
     // DCL
     CheckFactories.registerCheck<misc::StaticAssertCheck>("cert-dcl03-c");
     CheckFactories.registerCheck<readability::UppercaseLiteralSuffixCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

 
 New checks
 ^^^^^^^^^^
+
+- New :doc:`bugprone-do-not-refer-atomic-twice
+  <clang-tidy/checks/bugprone-do-not-refer-atomic-twice>` check.
+
+  Finds atomic variable which is referred twice in an expression.
+
 - New :doc:`cppcoreguidelines-avoid-non-const-global-variables
   <clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables>` check.
   Finds non-const global variables as described in check I.2 of C++ Core

   :doc:`bugprone-spuriously-wake-up-functions
   <clang-tidy/checks/bugprone-spuriously-wake-up-functions>` was added.
 
+- New alias :doc:`cert-con40-c
+  <clang-tidy/checks/cert-con40-c>` to
+  :doc:`bugprone-do-not-refer-atomic-twice
+  <clang-tidy/checks/bugprone-do-not-refer-atomic-twice>` was added.
+
 - New alias :doc:`cert-con54-cpp
   <clang-tidy/checks/cert-con54-cpp>` to
   :doc:`bugprone-spuriously-wake-up-functions

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

+.. title:: clang-tidy - bugprone-do-not-refer-atomic-twice
+
+bugprone-do-not-refer-atomic-twice
+==================================
+
+Finds atomic variable which is referred twice in an expression.
+
+.. code-block:: c
+
+    atomic_int n = ATOMIC_VAR_INIT(0);
+    int compute_sum(void) {
+        return n * (n + 1) / 2;
+    }
+
+This check corresponds to the CERT C Coding Standard rule
+`CON40-C. Do not refer to an atomic variable twice in an expression
+<https://wiki.sei.cmu.edu/confluence/display/c/CON40-C.+Do+not+refer+to+an+atomic+variable+twice+in+an+expression>`_.

clang-tools-extra/docs/clang-tidy/checks/cert-con40-c.rst

+.. title:: clang-tidy - cert-con40-c
+.. meta::
+   :http-equiv=refresh: 5;URL=bugprone-do-not-refer-atomic-twice.html
+
+cert-con40-c
+============
+
+The cert-con40-c check is an alias, please see
+`bugprone-do-not-refer-atomic-twice <bugprone-do-not-refer-atomic-twice>`_
+for more information.

clang-tools-extra/docs/clang-tidy/checks/list.rst

    `bugprone-branch-clone <bugprone-branch-clone.html>`_,
    `bugprone-copy-constructor-init <bugprone-copy-constructor-init.html>`_, "Yes"
    `bugprone-dangling-handle <bugprone-dangling-handle.html>`_,
+   `bugprone-do-not-refer-atomic-twice <bugprone-do-not-refer-atomic-twice.html>`_,
    `bugprone-dynamic-static-initializers <bugprone-dynamic-static-initializers.html>`_,
    `bugprone-exception-escape <bugprone-exception-escape.html>`_,
    `bugprone-fold-init-type <bugprone-fold-init-type.html>`_,

    `bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
    `bugprone-use-after-move <bugprone-use-after-move.html>`_,
    `bugprone-virtual-near-miss <bugprone-virtual-near-miss.html>`_, "Yes"
+   `cert-con40-c <cert-con40-c.html>`_,
    `cert-dcl21-cpp <cert-dcl21-cpp.html>`_,
    `cert-dcl50-cpp <cert-dcl50-cpp.html>`_,
    `cert-dcl58-cpp <cert-dcl58-cpp.html>`_,

    `hicpp-use-override <hicpp-use-override.html>`_, `modernize-use-override <modernize-use-override.html>`_, "Yes"
    `hicpp-vararg <hicpp-vararg.html>`_, `cppcoreguidelines-pro-type-vararg <cppcoreguidelines-pro-type-vararg.html>`_,
    `llvm-qualified-auto <llvm-qualified-auto.html>`_, `readability-qualified-auto <readability-qualified-auto.html>`_, "Yes"
-

clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp

+// RUN: %check_clang_tidy %s bugprone-do-not-refer-atomic-twice %t
+#define _Bool bool
+typedef _Atomic _Bool atomic_bool;
+typedef _Atomic int atomic_int;
+#define ATOMIC_VAR_INIT(VALUE) (VALUE)
+
+atomic_bool b = ATOMIC_VAR_INIT(false);
+atomic_int n = ATOMIC_VAR_INIT(0);
+_Atomic int n2 = ATOMIC_VAR_INIT(0);
+_Atomic(int) n3 = ATOMIC_VAR_INIT(0);
+
+void warn1() {
+  n = (n + 1) / 2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Do not refer to 'n' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+  n2 = (n2 + 1) / 2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Do not refer to 'n2' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+  n3 = (n3 + 1) / 2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Do not refer to 'n3' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+  b = b && true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Do not refer to 'b' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+}
+
+int warn2_1() {
+  return n * (n + 1) / 2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Do not refer to 'n' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+}
+
+int warn2_2() {
+  return n2 * (n2 + 1) / 2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Do not refer to 'n2' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+}
+
+int warn2_3() {
+  return n3 * (n3 + 1) / 2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: Do not refer to 'n3' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+}
+
+int warn2_4() {
+  return (b && true) || b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: Do not refer to 'b' atomic variable twice in an expression [bugprone-do-not-refer-atomic-twice]
+}
+
+void good1() {
+  n = 12;
+  n2 = 12;
+  n3 = 12;
+  b = true;
+}
+
+int good2_1() {
+  return n;
+}
+
+int good2_2() {
+  return n2;
+}
+
+int good2_3() {
+  return n3;
+}
+
+bool good2_4() {
+  return b;
+}
+
+void good3() {
+  n += 12;
+  n2 += 12;
+  n3 += 12;
+  b ^= 1;
+}
+
+void good4() {
+  n++;
+  n2++;
+  n3++;
+}