RFC: [clang-tidy] [analyzer] Move nondeterministic pointer usage check to tidy (#110471)

This change moves the `alpha.nondeterministic.PointerSorting` and
`alpha.nondeterministic.PointerIteration` static analyzer checkers to a
single `clang-tidy` check. Those checkers were implemented as simple
`clang-tidy` check-like code, wrapped in the static analyzer framework.
The documentation was updated to describe what the checks can and cannot
do, and testing was completed on a broad set of open-source projects.

Co-authored-by: Vince Bridgers <[email protected]>

Author: vabridgers

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

@@ -49,6 +49,7 @@
 #include "MultipleStatementMacroCheck.h"
 #include "NoEscapeCheck.h"
 #include "NonZeroEnumToBoolConversionCheck.h"
+#include "NondeterministicPointerIterationOrderCheck.h"
 #include "NotNullTerminatedResultCheck.h"
 #include "OptionalValueConversionCheck.h"
 #include "ParentVirtualCallCheck.h"
@@ -174,6 +175,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-multiple-new-in-one-expression");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<NondeterministicPointerIterationOrderCheck>(
+        "bugprone-nondeterministic-pointer-iteration-order");
     CheckFactories.registerCheck<OptionalValueConversionCheck>(
         "bugprone-optional-value-conversion");
     CheckFactories.registerCheck<PointerArithmeticOnPolymorphicObjectCheck>(

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

@@ -45,6 +45,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   MultipleNewInOneExpressionCheck.cpp
   MultipleStatementMacroCheck.cpp
   NoEscapeCheck.cpp
+  NondeterministicPointerIterationOrderCheck.cpp
   NonZeroEnumToBoolConversionCheck.cpp
   NotNullTerminatedResultCheck.cpp
   OptionalValueConversionCheck.cpp

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

@@ -0,0 +1,79 @@
+//===----- NondeterministicPointerIterationOrderCheck.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 "NondeterministicPointerIterationOrderCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void NondeterministicPointerIterationOrderCheck::registerMatchers(
+    MatchFinder *Finder) {
+
+  auto LoopVariable = varDecl(hasType(
+      qualType(hasCanonicalType(anyOf(referenceType(), pointerType())))));
+
+  auto RangeInit = declRefExpr(to(varDecl(
+      hasType(recordDecl(hasAnyName("std::unordered_set", "std::unordered_map",
+                                    "std::unordered_multiset",
+                                    "std::unordered_multimap"))
+                  .bind("recorddecl")))));
+
+  Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVariable),
+                                     hasRangeInit(RangeInit.bind("rangeinit")))
+                         .bind("cxxForRangeStmt"),
+                     this);
+
+  auto SortFuncM = callee(functionDecl(hasAnyName(
+      "std::is_sorted", "std::nth_element", "std::sort", "std::partial_sort",
+      "std::partition", "std::stable_partition", "std::stable_sort")));
+
+  auto IteratesPointerEltsM = hasArgument(
+      0,
+      cxxMemberCallExpr(on(hasType(cxxRecordDecl(has(fieldDecl(hasType(qualType(
+          hasCanonicalType(pointsTo(hasCanonicalType(pointerType()))))))))))));
+
+  Finder->addMatcher(
+      callExpr(allOf(SortFuncM, IteratesPointerEltsM)).bind("sortsemantic"),
+      this);
+}
+
+void NondeterministicPointerIterationOrderCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *ForRangePointers =
+      Result.Nodes.getNodeAs<CXXForRangeStmt>("cxxForRangeStmt");
+
+  if ((ForRangePointers) && !(ForRangePointers->getBeginLoc().isMacroID())) {
+    const auto *RangeInit = Result.Nodes.getNodeAs<Stmt>("rangeinit");
+    if (const auto *ClassTemplate =
+            Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>(
+                "recorddecl")) {
+      const TemplateArgumentList &TemplateArgs =
+          ClassTemplate->getTemplateArgs();
+      const llvm::StringRef AlgoName = ClassTemplate->getName();
+      const bool IsAlgoArgPointer =
+          TemplateArgs[0].getAsType()->isPointerType();
+
+      if (IsAlgoArgPointer) {
+        SourceRange R = RangeInit->getSourceRange();
+        diag(R.getBegin(), "iteration of pointers is nondeterministic") << R;
+      }
+    }
+    return;
+  }
+  const auto *SortPointers = Result.Nodes.getNodeAs<Stmt>("sortsemantic");
+
+  if ((SortPointers) && !(SortPointers->getBeginLoc().isMacroID())) {
+    SourceRange R = SortPointers->getSourceRange();
+    diag(R.getBegin(), "sorting pointers is nondeterministic") << R;
+  }
+}
+
+} // namespace clang::tidy::bugprone

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

@@ -0,0 +1,39 @@
+//=== NondeterministicPointerIterationOrderCheck.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_NONDETERMINISTIC_POINTER_ITERATION_ORDER_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NONDETERMINISTIC_POINTER_ITERATION_ORDER_CHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds nondeterministic usages of pointers in unordered containers. The
+/// check also finds calls to sorting-like algorithms on a container of
+/// pointers.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/nondeterministic-pointer-iteration-order.html
+class NondeterministicPointerIterationOrderCheck : public ClangTidyCheck {
+public:
+  NondeterministicPointerIterationOrderCheck(StringRef Name,
+                                             ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NONDETERMINISTIC_POINTER_ITERATION_ORDER_CHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -119,6 +119,12 @@ New checks
   Warns about code that tries to cast between pointers by means of
   ``std::bit_cast`` or ``memcpy``.
 
+- New :doc:`bugprone-nondeterministic-pointer-iteration-order
+  <clang-tidy/checks/bugprone/nondeterministic-pointer-iteration-order>`
+  check.
+
+  Finds nondeterministic usages of pointers in unordered containers.
+
 - New :doc:`bugprone-tagged-union-member-count
   <clang-tidy/checks/bugprone/tagged-union-member-count>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-iteration-order.rst

@@ -0,0 +1,44 @@
+.. title:: clang-tidy - bugprone-nondeterministic-pointer-iteration-order
+
+bugprone-nondeterministic-pointer-iteration-order
+=================================================
+
+Finds nondeterministic usages of pointers in unordered containers.
+
+One canonical example is iteration across a container of pointers.
+
+.. code-block:: c++
+
+  {
+    int a = 1, b = 2;
+    std::unordered_set<int *> UnorderedPtrSet = {&a, &b};
+    for (auto i : UnorderedPtrSet)
+      f(i);
+  }
+
+Another such example is sorting a container of pointers.
+
+.. code-block:: c++
+
+  {
+    int a = 1, b = 2;
+    std::vector<int *> VectorOfPtr = {&a, &b};
+    std::sort(VectorOfPtr.begin(), VectorOfPtr.end());
+  }
+
+Iteration of a containers of pointers may present the order of different
+pointers differently across different runs of a program. In some cases this
+may be acceptable behavior, in others this may be unexpected behavior. This
+check is advisory for this reason.
+
+This check only detects range-based for loops over unordered sets and maps. It
+also detects calls sorting-like algorithms on containers holding pointers.
+Other similar usages will not be found and are false negatives.
+
+Limitations:
+
+* This check currently does not check if a nondeterministic iteration order is
+  likely to be a mistake, and instead marks all such iterations as bugprone.
+* std::reference_wrapper is not considered yet.
+* Only for loops are considered, other iterators can be included in
+  improvements.

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

@@ -115,6 +115,7 @@ Clang-Tidy Checks
    :doc:`bugprone-multiple-new-in-one-expression <bugprone/multiple-new-in-one-expression>`,
    :doc:`bugprone-multiple-statement-macro <bugprone/multiple-statement-macro>`,
    :doc:`bugprone-no-escape <bugprone/no-escape>`,
+   :doc:`bugprone-nondeterministic-pointer-iteration-order <bugprone/nondeterministic-pointer-iteration-order>`,
    :doc:`bugprone-non-zero-enum-to-bool-conversion <bugprone/non-zero-enum-to-bool-conversion>`,
    :doc:`bugprone-not-null-terminated-result <bugprone/not-null-terminated-result>`, "Yes"
    :doc:`bugprone-optional-value-conversion <bugprone/optional-value-conversion>`, "Yes"

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator/sim_algorithm

@@ -0,0 +1,31 @@
+#ifndef _SIM_ALGORITHM
+#define _SIM_ALGORITHM
+
+#pragma clang system_header
+
+namespace std {
+
+template<class ForwardIt>
+bool is_sorted(ForwardIt first, ForwardIt last);
+
+template <class RandomIt>
+void nth_element(RandomIt first, RandomIt nth, RandomIt last);
+
+template<class RandomIt>
+void partial_sort(RandomIt first, RandomIt middle, RandomIt last);
+
+template<class RandomIt>
+void sort (RandomIt first, RandomIt last);
+
+template<class RandomIt>
+void stable_sort(RandomIt first, RandomIt last);
+
+template<class BidirIt, class UnaryPredicate>
+BidirIt partition(BidirIt first, BidirIt last, UnaryPredicate p);
+
+template<class BidirIt, class UnaryPredicate>
+BidirIt stable_partition(BidirIt first, BidirIt last, UnaryPredicate p);
+
+} // namespace std
+
+#endif // _SIM_ALGORITHM

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator/sim_c++config.h

@@ -0,0 +1,11 @@
+#ifndef _SIM_CPP_CONFIG_H
+#define _SIM_CPP_CONFIG_H
+
+#pragma clang system_header
+
+typedef unsigned char uint8_t;
+
+typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
+
+#endif // _SIM_CPP_CONFIG_H

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator/sim_initializer_list

@@ -0,0 +1,39 @@
+#ifndef _INITIALIZER_LIST
+#define _INITIALIZER_LIST
+
+#pragma clang system_header
+#
+#include "sim_c++config.h" // size_t
+
+namespace std {
+
+template <class _E>
+class initializer_list  {
+  const _E* __begin_;
+  size_t    __size_;
+
+  initializer_list(const _E* __b, size_t __s)
+    : __begin_(__b),
+      __size_(__s)
+  {}
+
+public:
+  typedef _E        value_type;
+  typedef const _E& reference;
+  typedef const _E& const_reference;
+  typedef size_t    size_type;
+
+  typedef const _E* iterator;
+  typedef const _E* const_iterator;
+
+  initializer_list() : __begin_(0), __size_(0) {}
+
+  size_t    size()  const {return __size_;}
+  const _E* begin() const {return __begin_;}
+  const _E* end()   const {return __begin_ + __size_;}
+
+}; // class initializer_list
+
+} // namespace std
+
+#endif // _INITIALIZER_LIST

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator/sim_iterator_base

@@ -0,0 +1,22 @@
+#ifndef _SIM_ITERATOR_BASE
+#define _SIM_ITERATOR_BASE
+
+namespace std {
+
+struct input_iterator_tag { };
+struct output_iterator_tag { };
+struct forward_iterator_tag : public input_iterator_tag { };
+struct bidirectional_iterator_tag : public forward_iterator_tag { };
+struct random_access_iterator_tag : public bidirectional_iterator_tag { };
+
+template <typename Iterator> struct iterator_traits {
+  typedef typename Iterator::difference_type difference_type;
+  typedef typename Iterator::value_type value_type;
+  typedef typename Iterator::pointer pointer;
+  typedef typename Iterator::reference reference;
+  typedef typename Iterator::iterator_category iterator_category;
+};
+
+} // namespace std
+
+#endif // _SIM_ITERATOR_BASE

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator/sim_map

@@ -0,0 +1,34 @@
+
+#ifndef _SIM_MAP
+#define _SIM_MAP
+
+#pragma clang system_header
+#include "sim_stl_pair"
+
+namespace std {
+
+template <typename Key, typename Value>
+class map {
+  public:
+    using value_type = pair<Key, Value>;
+    map();
+    map(initializer_list<pair<Key, Value>> initList);
+    value_type& operator[](const Key& key);
+    value_type& operator[](Key&& key);
+    class iterator {
+    public:
+      iterator(Key *key): ptr(key) {}
+      iterator& operator++() { ++ptr; return *this; }
+      bool operator!=(const iterator &other) const { return ptr != other.ptr; }
+      const Key &operator*() const { return *ptr; }
+    private:
+      Key *ptr;
+    };
+    Key *val;
+    iterator begin() const { return iterator(val); }
+    iterator end() const { return iterator(val + 1); }
+};
+
+} // namespace std
+
+#endif // _SIM_MAP

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator/sim_set

@@ -0,0 +1,44 @@
+
+#ifndef _SIM_SET
+#define _SIM_SET
+
+#pragma clang system_header
+#include "sim_initializer_list"
+
+namespace std {
+
+template< class T = void >
+struct less;
+
+template< class T >
+struct allocator;
+
+template< class Key >
+struct hash;
+
+template<
+  class Key,
+  class Compare = std::less<Key>,
+  class Alloc = std::allocator<Key>
+> class set {
+  public:
+    set(initializer_list<Key> __list) {}
+
+    class iterator {
+    public:
+      iterator(Key *key): ptr(key) {}
+      iterator& operator++() { ++ptr; return *this; }
+      bool operator!=(const iterator &other) const { return ptr != other.ptr; }
+      const Key &operator*() const { return *ptr; }
+    private:
+      Key *ptr;
+    };
+
+    Key *val;
+    iterator begin() const { return iterator(val); }
+    iterator end() const { return iterator(val + 1); }
+};
+
+} // namespace std
+
+#endif // _SIM_SET