RFC: [clang-tidy] [analyzer] Move nondeterministic pointer usage check to tidy #110471
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: None (vabridgers) ChangesRFC: This PR is not ready to merge yet, preliminary comments are welcome. Q) Is the name and file location ok in the directory structure? 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 clang-tidy checks 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. Patch is 75.02 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110471.diff 13 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 689eb92a3d8d17..7c177196d76f58 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -48,6 +48,7 @@
#include "MultipleStatementMacroCheck.h"
#include "NoEscapeCheck.h"
#include "NonZeroEnumToBoolConversionCheck.h"
+#include "NondeterministicPointerUsageCheck.h"
#include "NotNullTerminatedResultCheck.h"
#include "OptionalValueConversionCheck.h"
#include "ParentVirtualCallCheck.h"
@@ -179,6 +180,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
"bugprone-narrowing-conversions");
CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
+ CheckFactories.registerCheck<NondeterministicPointerUsageCheck>(
+ "bugprone-nondeterministic-pointer-usage");
CheckFactories.registerCheck<NonZeroEnumToBoolConversionCheck>(
"bugprone-non-zero-enum-to-bool-conversion");
CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index cb0d8ae98bac58..5628572b984226 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -44,6 +44,7 @@ add_clang_library(clangTidyBugproneModule
MultipleNewInOneExpressionCheck.cpp
MultipleStatementMacroCheck.cpp
NoEscapeCheck.cpp
+ NondeterministicPointerUsageCheck.cpp
NonZeroEnumToBoolConversionCheck.cpp
NotNullTerminatedResultCheck.cpp
OptionalValueConversionCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
new file mode 100644
index 00000000000000..ddc314af739d97
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
@@ -0,0 +1,67 @@
+//===--- NondetermnisticPointerUsageCheck.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 "NondeterministicPointerUsageCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void NondeterministicPointerUsageCheck::registerMatchers(MatchFinder *Finder) {
+
+ auto LoopVariable = varDecl(hasType(hasCanonicalType(pointerType())));
+
+ auto RangeInit = declRefExpr(to(varDecl(hasType(recordDecl(
+ anyOf(hasName("std::unordered_set"), hasName("std::unordered_map"),
+ hasName("std::unordered_multiset"),
+ hasName("std::unordered_multimap")))))));
+
+ Finder->addMatcher(
+ stmt(cxxForRangeStmt(hasRangeInit(RangeInit.bind("rangeinit")),
+ hasLoopVariable(LoopVariable.bind("loopVar"))))
+ .bind("cxxForRangeStmt"),
+ this);
+
+ auto SortFuncM = anyOf(callee(functionDecl(hasName("std::is_sorted"))),
+ callee(functionDecl(hasName("std::nth_element"))),
+ callee(functionDecl(hasName("std::sort"))),
+ callee(functionDecl(hasName("std::partial_sort"))),
+ callee(functionDecl(hasName("std::partition"))),
+ callee(functionDecl(hasName("std::stable_partition"))),
+ callee(functionDecl(hasName("std::stable_sort"))));
+
+ auto IteratesPointerEltsM = hasArgument(
+ 0,
+ cxxMemberCallExpr(on(hasType(cxxRecordDecl(has(fieldDecl(hasType(
+ hasCanonicalType(pointsTo(hasCanonicalType(pointerType())))))))))));
+
+ Finder->addMatcher(stmt(callExpr(allOf(SortFuncM, IteratesPointerEltsM)))
+ .bind("sortsemantic"),
+ this);
+}
+
+void NondeterministicPointerUsageCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *ForRangePointers =
+ Result.Nodes.getNodeAs<Stmt>("cxxForRangeStmt");
+ const auto *SortPointers = Result.Nodes.getNodeAs<Stmt>("sortsemantic");
+
+ if ((ForRangePointers) && !(ForRangePointers->getBeginLoc().isMacroID())) {
+ const auto *Node = dyn_cast<CXXForRangeStmt>(ForRangePointers);
+ diag(Node->getRParenLoc(), "Iteration of pointers is nondeterministic");
+ }
+
+ if ((SortPointers) && !(SortPointers->getBeginLoc().isMacroID())) {
+ const auto *Node = dyn_cast<Stmt>(SortPointers);
+ diag(Node->getBeginLoc(), "Sorting pointers is nondeterministic");
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
new file mode 100644
index 00000000000000..556a4fce4dffe2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
@@ -0,0 +1,36 @@
+//===--- NondeterministicPointerUsageCheck.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__NONDETERMINISTICPOINTERUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE__NONDETERMINISTICPOINTERUSAGECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds temporaries that look like RAII objects.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.html
+class NondeterministicPointerUsageCheck : public ClangTidyCheck {
+public:
+ NondeterministicPointerUsageCheck(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__NONDETERMINISTICPOINTERUSAGECHECK_H
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
new file mode 100644
index 00000000000000..8625487c6d694a
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - bugprone-nondeterministic-pointer-usage
+
+nondeterministic-pointer-usage
+==============================
+
+Finds nondeterministic usages of pointers in unordered containers.
+
+One canonical example is iteration across a container of pointers.
+
+.. code-block:: c++
+
+ {
+ for (auto i : UnorderedPtrSet)
+ f(i);
+ }
+
+Another such example is sorting a container of pointers.
+
+.. code-block:: c++
+
+ {
+ 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. Other
+similar usages will not be found and are false negatives.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
new file mode 100644
index 00000000000000..b279a8f20d8ddd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
@@ -0,0 +1,1450 @@
+// Like the compiler, the static analyzer treats some functions differently if
+// they come from a system header -- for example, it is assumed that system
+// functions do not arbitrarily free() their parameters, and that some bugs
+// found in system headers cannot be fixed by the user and should be
+// suppressed.
+#pragma clang system_header
+
+typedef unsigned char uint8_t;
+
+typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
+void *memmove(void *s1, const void *s2, size_t n);
+
+namespace std {
+ typedef size_t size_type;
+#if __cplusplus >= 201103L
+ using nullptr_t = decltype(nullptr);
+#endif
+}
+
+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;
+ };
+}
+
+template <typename T, typename Ptr, typename Ref> struct __vector_iterator {
+ typedef __vector_iterator<T, T *, T &> iterator;
+ typedef __vector_iterator<T, const T *, const T &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __vector_iterator(const Ptr p = 0) : ptr(p) {}
+ __vector_iterator(const iterator &rhs): ptr(rhs.base()) {}
+ __vector_iterator<T, Ptr, Ref>& operator++() { ++ ptr; return *this; }
+ __vector_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ ++ ptr;
+ return tmp;
+ }
+ __vector_iterator<T, Ptr, Ref> operator--() { -- ptr; return *this; }
+ __vector_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this; -- ptr;
+ return tmp;
+ }
+ __vector_iterator<T, Ptr, Ref> operator+(difference_type n) {
+ return ptr + n;
+ }
+ friend __vector_iterator<T, Ptr, Ref> operator+(
+ difference_type n,
+ const __vector_iterator<T, Ptr, Ref> &iter) {
+ return n + iter.ptr;
+ }
+ __vector_iterator<T, Ptr, Ref> operator-(difference_type n) {
+ return ptr - n;
+ }
+ __vector_iterator<T, Ptr, Ref> operator+=(difference_type n) {
+ return ptr += n;
+ }
+ __vector_iterator<T, Ptr, Ref> operator-=(difference_type n) {
+ return ptr -= n;
+ }
+
+ template<typename U, typename Ptr2, typename Ref2>
+ difference_type operator-(const __vector_iterator<U, Ptr2, Ref2> &rhs);
+
+ Ref operator*() const { return *ptr; }
+ Ptr operator->() const { return ptr; }
+
+ Ref operator[](difference_type n) {
+ return *(ptr+n);
+ }
+
+ bool operator==(const iterator &rhs) const { return ptr == rhs.ptr; }
+ bool operator==(const const_iterator &rhs) const { return ptr == rhs.ptr; }
+
+ bool operator!=(const iterator &rhs) const { return ptr != rhs.ptr; }
+ bool operator!=(const const_iterator &rhs) const { return ptr != rhs.ptr; }
+
+ const Ptr& base() const { return ptr; }
+
+private:
+ Ptr ptr;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __deque_iterator {
+ typedef __deque_iterator<T, T *, T &> iterator;
+ typedef __deque_iterator<T, const T *, const T &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __deque_iterator(const Ptr p = 0) : ptr(p) {}
+ __deque_iterator(const iterator &rhs): ptr(rhs.base()) {}
+ __deque_iterator<T, Ptr, Ref>& operator++() { ++ ptr; return *this; }
+ __deque_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ ++ ptr;
+ return tmp;
+ }
+ __deque_iterator<T, Ptr, Ref> operator--() { -- ptr; return *this; }
+ __deque_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this; -- ptr;
+ return tmp;
+ }
+ __deque_iterator<T, Ptr, Ref> operator+(difference_type n) {
+ return ptr + n;
+ }
+ friend __deque_iterator<T, Ptr, Ref> operator+(
+ difference_type n,
+ const __deque_iterator<T, Ptr, Ref> &iter) {
+ return n + iter.ptr;
+ }
+ __deque_iterator<T, Ptr, Ref> operator-(difference_type n) {
+ return ptr - n;
+ }
+ __deque_iterator<T, Ptr, Ref> operator+=(difference_type n) {
+ return ptr += n;
+ }
+ __deque_iterator<T, Ptr, Ref> operator-=(difference_type n) {
+ return ptr -= n;
+ }
+
+ Ref operator*() const { return *ptr; }
+ Ptr operator->() const { return ptr; }
+
+ Ref operator[](difference_type n) {
+ return *(ptr+n);
+ }
+
+ bool operator==(const iterator &rhs) const { return ptr == rhs.ptr; }
+ bool operator==(const const_iterator &rhs) const { return ptr == rhs.ptr; }
+
+ bool operator!=(const iterator &rhs) const { return ptr != rhs.ptr; }
+ bool operator!=(const const_iterator &rhs) const { return ptr != rhs.ptr; }
+
+ const Ptr& base() const { return ptr; }
+
+private:
+ Ptr ptr;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __list_iterator {
+ typedef __list_iterator<T, __typeof__(T::data) *, __typeof__(T::data) &> iterator;
+ typedef __list_iterator<T, const __typeof__(T::data) *, const __typeof__(T::data) &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::bidirectional_iterator_tag iterator_category;
+
+ __list_iterator(T* it = 0) : item(it) {}
+ __list_iterator(const iterator &rhs): item(rhs.item) {}
+ __list_iterator<T, Ptr, Ref>& operator++() { item = item->next; return *this; }
+ __list_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ item = item->next;
+ return tmp;
+ }
+ __list_iterator<T, Ptr, Ref> operator--() { item = item->prev; return *this; }
+ __list_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this;
+ item = item->prev;
+ return tmp;
+ }
+
+ Ref operator*() const { return item->data; }
+ Ptr operator->() const { return &item->data; }
+
+ bool operator==(const iterator &rhs) const { return item == rhs->item; }
+ bool operator==(const const_iterator &rhs) const { return item == rhs->item; }
+
+ bool operator!=(const iterator &rhs) const { return item != rhs->item; }
+ bool operator!=(const const_iterator &rhs) const { return item != rhs->item; }
+
+ const T* &base() const { return item; }
+
+ template <typename UT, typename UPtr, typename URef>
+ friend struct __list_iterator;
+
+private:
+ T* item;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __fwdl_iterator {
+ typedef __fwdl_iterator<T, __typeof__(T::data) *, __typeof__(T::data) &> iterator;
+ typedef __fwdl_iterator<T, const __typeof__(T::data) *, const __typeof__(T::data) &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::forward_iterator_tag iterator_category;
+
+ __fwdl_iterator(T* it = 0) : item(it) {}
+ __fwdl_iterator(const iterator &rhs): item(rhs.item) {}
+ __fwdl_iterator<T, Ptr, Ref>& operator++() { item = item->next; return *this; }
+ __fwdl_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ item = item->next;
+ return tmp;
+ }
+ Ref operator*() const { return item->data; }
+ Ptr operator->() const { return &item->data; }
+
+ bool operator==(const iterator &rhs) const { return item == rhs->item; }
+ bool operator==(const const_iterator &rhs) const { return item == rhs->item; }
+
+ bool operator!=(const iterator &rhs) const { return item != rhs->item; }
+ bool operator!=(const const_iterator &rhs) const { return item != rhs->item; }
+
+ const T* &base() const { return item; }
+
+ template <typename UT, typename UPtr, typename URef>
+ friend struct __fwdl_iterator;
+
+private:
+ T* item;
+};
+
+namespace std {
+ template <class T1, class T2>
+ struct pair {
+ T1 first;
+ T2 second;
+
+ pair() : first(), second() {}
+ pair(const T1 &a, const T2 &b) : first(a), second(b) {}
+
+ template<class U1, class U2>
+ pair(const pair<U1, U2> &other) : first(other.first),
+ second(other.second) {}
+ };
+
+ template<class T2, class T1>
+ T2& get(pair<T1, T2>& p) ;
+ template<class T1, class T2>
+ T1& get(const pair<T1, T2>& p) ;
+
+ typedef __typeof__(sizeof(int)) size_t;
+
+ template <class T> class initializer_list;
+
+ template< class T > struct remove_reference {typedef T type;};
+ template< class T > struct remove_reference<T&> {typedef T type;};
+ template< class T > struct remove_reference<T&&> {typedef T type;};
+
+ template<typename T> typename remove_reference<T>::type&& move(T&& a);
+ template <typename T> T *__addressof(T &x);
+ template <typename T> T *addressof(T &x);
+ template <typename T> const T& as_const(T& x);
+ template <typename T> T&& forward(T&& x);
+ // FIXME: Declare forward_like
+ // FIXME: Declare move_if_noexcept
+
+ template< class T >
+ using remove_reference_t = typename remove_reference<T>::type;
+
+ template <class T>
+ void swap(T &a, T &b) {
+ T c(std::move(a));
+ a = std::move(b);
+ b = std::move(c);
+ }
+
+ template<typename T>
+ class vector {
+ T *_start;
+ T *_finish;
+ T *_end_of_storage;
+
+ public:
+ typedef T value_type;
+ typedef size_t size_type;
+ typedef __vector_iterator<T, T *, T &> iterator;
+ typedef __vector_iterator<T, const T *, const T &> const_iterator;
+
+ vector() : _start(0), _finish(0), _end_of_storage(0) {}
+ template <typename InputIterator>
+ vector(InputIterator first, InputIterator last);
+ vector(const vector &other);
+ vector(vector &&other);
+ ~vector();
+
+ size_t size() const {
+ return size_t(_finish - _start);
+ }
+
+ vector& operator=(const vector &other);
+ vector& operator=(vector &&other);
+ vector& operator=(std::initializer_list<T> ilist);
+
+ void assign(size_type count, const T &value);
+ template <typename InputIterator >
+ void assign(InputIterator first, InputIterator last);
+ void assign(std::initializer_list<T> ilist);
+
+ void clear();
+
+ void push_back(const T &value);
+ void push_back(T &&value);
+ template<class... Args>
+ void emplace_back(Args&&... args);
+ void pop_back();
+
+ iterator insert(const_iterator position, const value_type &val);
+ iterator insert(const_iterator position, size_type n,
+ const value_type &val);
+ template <typename InputIterator>
+ iterator insert(const_iterator position, InputIterator first,
+ InputIterator last);
+ iterator insert(const_iterator position, value_type &&val);
+ iterator insert(const_iterator position, initializer_list<value_type> il);
+
+ template <class... Args>
+ iterator emplace(const_iterator position, Args&&... args);
+
+ iterator erase(const_iterator position);
+ iterator erase(const_iterator first, const_iterator last);
+
+ T &operator[](size_t n) {
+ return _start[n];
+ }
+
+ const T &operator[](size_t n) const {
+ return _start[n];
+ }
+
+ iterator begin() { return iterator(_start); }
+ const_iterator begin() const { return const_iterator(_start); }
+ const_iterator cbegin() const { return const_iterator(_start); }
+ iterator end() { return iterator(_finish); }
+ const_iterator end() const { return const_iterator(_finish); }
+ const_iterator cend() const { return const_iterator(_finish); }
+ T& front() { return *begin(); }
+ const T& front() const { return *begin(); }
+ T& back() { return *(end() - 1); }
+ const T& back() const { re...
[truncated]
|
|
@llvm/pr-subscribers-clang-tidy Author: None (vabridgers) ChangesRFC: This PR is not ready to merge yet, preliminary comments are welcome. Q) Is the name and file location ok in the directory structure? 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 clang-tidy checks 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. Patch is 75.02 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110471.diff 13 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 689eb92a3d8d17..7c177196d76f58 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -48,6 +48,7 @@
#include "MultipleStatementMacroCheck.h"
#include "NoEscapeCheck.h"
#include "NonZeroEnumToBoolConversionCheck.h"
+#include "NondeterministicPointerUsageCheck.h"
#include "NotNullTerminatedResultCheck.h"
#include "OptionalValueConversionCheck.h"
#include "ParentVirtualCallCheck.h"
@@ -179,6 +180,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
"bugprone-narrowing-conversions");
CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
+ CheckFactories.registerCheck<NondeterministicPointerUsageCheck>(
+ "bugprone-nondeterministic-pointer-usage");
CheckFactories.registerCheck<NonZeroEnumToBoolConversionCheck>(
"bugprone-non-zero-enum-to-bool-conversion");
CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index cb0d8ae98bac58..5628572b984226 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -44,6 +44,7 @@ add_clang_library(clangTidyBugproneModule
MultipleNewInOneExpressionCheck.cpp
MultipleStatementMacroCheck.cpp
NoEscapeCheck.cpp
+ NondeterministicPointerUsageCheck.cpp
NonZeroEnumToBoolConversionCheck.cpp
NotNullTerminatedResultCheck.cpp
OptionalValueConversionCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
new file mode 100644
index 00000000000000..ddc314af739d97
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
@@ -0,0 +1,67 @@
+//===--- NondetermnisticPointerUsageCheck.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 "NondeterministicPointerUsageCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void NondeterministicPointerUsageCheck::registerMatchers(MatchFinder *Finder) {
+
+ auto LoopVariable = varDecl(hasType(hasCanonicalType(pointerType())));
+
+ auto RangeInit = declRefExpr(to(varDecl(hasType(recordDecl(
+ anyOf(hasName("std::unordered_set"), hasName("std::unordered_map"),
+ hasName("std::unordered_multiset"),
+ hasName("std::unordered_multimap")))))));
+
+ Finder->addMatcher(
+ stmt(cxxForRangeStmt(hasRangeInit(RangeInit.bind("rangeinit")),
+ hasLoopVariable(LoopVariable.bind("loopVar"))))
+ .bind("cxxForRangeStmt"),
+ this);
+
+ auto SortFuncM = anyOf(callee(functionDecl(hasName("std::is_sorted"))),
+ callee(functionDecl(hasName("std::nth_element"))),
+ callee(functionDecl(hasName("std::sort"))),
+ callee(functionDecl(hasName("std::partial_sort"))),
+ callee(functionDecl(hasName("std::partition"))),
+ callee(functionDecl(hasName("std::stable_partition"))),
+ callee(functionDecl(hasName("std::stable_sort"))));
+
+ auto IteratesPointerEltsM = hasArgument(
+ 0,
+ cxxMemberCallExpr(on(hasType(cxxRecordDecl(has(fieldDecl(hasType(
+ hasCanonicalType(pointsTo(hasCanonicalType(pointerType())))))))))));
+
+ Finder->addMatcher(stmt(callExpr(allOf(SortFuncM, IteratesPointerEltsM)))
+ .bind("sortsemantic"),
+ this);
+}
+
+void NondeterministicPointerUsageCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *ForRangePointers =
+ Result.Nodes.getNodeAs<Stmt>("cxxForRangeStmt");
+ const auto *SortPointers = Result.Nodes.getNodeAs<Stmt>("sortsemantic");
+
+ if ((ForRangePointers) && !(ForRangePointers->getBeginLoc().isMacroID())) {
+ const auto *Node = dyn_cast<CXXForRangeStmt>(ForRangePointers);
+ diag(Node->getRParenLoc(), "Iteration of pointers is nondeterministic");
+ }
+
+ if ((SortPointers) && !(SortPointers->getBeginLoc().isMacroID())) {
+ const auto *Node = dyn_cast<Stmt>(SortPointers);
+ diag(Node->getBeginLoc(), "Sorting pointers is nondeterministic");
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
new file mode 100644
index 00000000000000..556a4fce4dffe2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
@@ -0,0 +1,36 @@
+//===--- NondeterministicPointerUsageCheck.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__NONDETERMINISTICPOINTERUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE__NONDETERMINISTICPOINTERUSAGECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds temporaries that look like RAII objects.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.html
+class NondeterministicPointerUsageCheck : public ClangTidyCheck {
+public:
+ NondeterministicPointerUsageCheck(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__NONDETERMINISTICPOINTERUSAGECHECK_H
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
new file mode 100644
index 00000000000000..8625487c6d694a
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - bugprone-nondeterministic-pointer-usage
+
+nondeterministic-pointer-usage
+==============================
+
+Finds nondeterministic usages of pointers in unordered containers.
+
+One canonical example is iteration across a container of pointers.
+
+.. code-block:: c++
+
+ {
+ for (auto i : UnorderedPtrSet)
+ f(i);
+ }
+
+Another such example is sorting a container of pointers.
+
+.. code-block:: c++
+
+ {
+ 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. Other
+similar usages will not be found and are false negatives.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
new file mode 100644
index 00000000000000..b279a8f20d8ddd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
@@ -0,0 +1,1450 @@
+// Like the compiler, the static analyzer treats some functions differently if
+// they come from a system header -- for example, it is assumed that system
+// functions do not arbitrarily free() their parameters, and that some bugs
+// found in system headers cannot be fixed by the user and should be
+// suppressed.
+#pragma clang system_header
+
+typedef unsigned char uint8_t;
+
+typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
+void *memmove(void *s1, const void *s2, size_t n);
+
+namespace std {
+ typedef size_t size_type;
+#if __cplusplus >= 201103L
+ using nullptr_t = decltype(nullptr);
+#endif
+}
+
+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;
+ };
+}
+
+template <typename T, typename Ptr, typename Ref> struct __vector_iterator {
+ typedef __vector_iterator<T, T *, T &> iterator;
+ typedef __vector_iterator<T, const T *, const T &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __vector_iterator(const Ptr p = 0) : ptr(p) {}
+ __vector_iterator(const iterator &rhs): ptr(rhs.base()) {}
+ __vector_iterator<T, Ptr, Ref>& operator++() { ++ ptr; return *this; }
+ __vector_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ ++ ptr;
+ return tmp;
+ }
+ __vector_iterator<T, Ptr, Ref> operator--() { -- ptr; return *this; }
+ __vector_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this; -- ptr;
+ return tmp;
+ }
+ __vector_iterator<T, Ptr, Ref> operator+(difference_type n) {
+ return ptr + n;
+ }
+ friend __vector_iterator<T, Ptr, Ref> operator+(
+ difference_type n,
+ const __vector_iterator<T, Ptr, Ref> &iter) {
+ return n + iter.ptr;
+ }
+ __vector_iterator<T, Ptr, Ref> operator-(difference_type n) {
+ return ptr - n;
+ }
+ __vector_iterator<T, Ptr, Ref> operator+=(difference_type n) {
+ return ptr += n;
+ }
+ __vector_iterator<T, Ptr, Ref> operator-=(difference_type n) {
+ return ptr -= n;
+ }
+
+ template<typename U, typename Ptr2, typename Ref2>
+ difference_type operator-(const __vector_iterator<U, Ptr2, Ref2> &rhs);
+
+ Ref operator*() const { return *ptr; }
+ Ptr operator->() const { return ptr; }
+
+ Ref operator[](difference_type n) {
+ return *(ptr+n);
+ }
+
+ bool operator==(const iterator &rhs) const { return ptr == rhs.ptr; }
+ bool operator==(const const_iterator &rhs) const { return ptr == rhs.ptr; }
+
+ bool operator!=(const iterator &rhs) const { return ptr != rhs.ptr; }
+ bool operator!=(const const_iterator &rhs) const { return ptr != rhs.ptr; }
+
+ const Ptr& base() const { return ptr; }
+
+private:
+ Ptr ptr;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __deque_iterator {
+ typedef __deque_iterator<T, T *, T &> iterator;
+ typedef __deque_iterator<T, const T *, const T &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __deque_iterator(const Ptr p = 0) : ptr(p) {}
+ __deque_iterator(const iterator &rhs): ptr(rhs.base()) {}
+ __deque_iterator<T, Ptr, Ref>& operator++() { ++ ptr; return *this; }
+ __deque_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ ++ ptr;
+ return tmp;
+ }
+ __deque_iterator<T, Ptr, Ref> operator--() { -- ptr; return *this; }
+ __deque_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this; -- ptr;
+ return tmp;
+ }
+ __deque_iterator<T, Ptr, Ref> operator+(difference_type n) {
+ return ptr + n;
+ }
+ friend __deque_iterator<T, Ptr, Ref> operator+(
+ difference_type n,
+ const __deque_iterator<T, Ptr, Ref> &iter) {
+ return n + iter.ptr;
+ }
+ __deque_iterator<T, Ptr, Ref> operator-(difference_type n) {
+ return ptr - n;
+ }
+ __deque_iterator<T, Ptr, Ref> operator+=(difference_type n) {
+ return ptr += n;
+ }
+ __deque_iterator<T, Ptr, Ref> operator-=(difference_type n) {
+ return ptr -= n;
+ }
+
+ Ref operator*() const { return *ptr; }
+ Ptr operator->() const { return ptr; }
+
+ Ref operator[](difference_type n) {
+ return *(ptr+n);
+ }
+
+ bool operator==(const iterator &rhs) const { return ptr == rhs.ptr; }
+ bool operator==(const const_iterator &rhs) const { return ptr == rhs.ptr; }
+
+ bool operator!=(const iterator &rhs) const { return ptr != rhs.ptr; }
+ bool operator!=(const const_iterator &rhs) const { return ptr != rhs.ptr; }
+
+ const Ptr& base() const { return ptr; }
+
+private:
+ Ptr ptr;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __list_iterator {
+ typedef __list_iterator<T, __typeof__(T::data) *, __typeof__(T::data) &> iterator;
+ typedef __list_iterator<T, const __typeof__(T::data) *, const __typeof__(T::data) &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::bidirectional_iterator_tag iterator_category;
+
+ __list_iterator(T* it = 0) : item(it) {}
+ __list_iterator(const iterator &rhs): item(rhs.item) {}
+ __list_iterator<T, Ptr, Ref>& operator++() { item = item->next; return *this; }
+ __list_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ item = item->next;
+ return tmp;
+ }
+ __list_iterator<T, Ptr, Ref> operator--() { item = item->prev; return *this; }
+ __list_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this;
+ item = item->prev;
+ return tmp;
+ }
+
+ Ref operator*() const { return item->data; }
+ Ptr operator->() const { return &item->data; }
+
+ bool operator==(const iterator &rhs) const { return item == rhs->item; }
+ bool operator==(const const_iterator &rhs) const { return item == rhs->item; }
+
+ bool operator!=(const iterator &rhs) const { return item != rhs->item; }
+ bool operator!=(const const_iterator &rhs) const { return item != rhs->item; }
+
+ const T* &base() const { return item; }
+
+ template <typename UT, typename UPtr, typename URef>
+ friend struct __list_iterator;
+
+private:
+ T* item;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __fwdl_iterator {
+ typedef __fwdl_iterator<T, __typeof__(T::data) *, __typeof__(T::data) &> iterator;
+ typedef __fwdl_iterator<T, const __typeof__(T::data) *, const __typeof__(T::data) &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::forward_iterator_tag iterator_category;
+
+ __fwdl_iterator(T* it = 0) : item(it) {}
+ __fwdl_iterator(const iterator &rhs): item(rhs.item) {}
+ __fwdl_iterator<T, Ptr, Ref>& operator++() { item = item->next; return *this; }
+ __fwdl_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ item = item->next;
+ return tmp;
+ }
+ Ref operator*() const { return item->data; }
+ Ptr operator->() const { return &item->data; }
+
+ bool operator==(const iterator &rhs) const { return item == rhs->item; }
+ bool operator==(const const_iterator &rhs) const { return item == rhs->item; }
+
+ bool operator!=(const iterator &rhs) const { return item != rhs->item; }
+ bool operator!=(const const_iterator &rhs) const { return item != rhs->item; }
+
+ const T* &base() const { return item; }
+
+ template <typename UT, typename UPtr, typename URef>
+ friend struct __fwdl_iterator;
+
+private:
+ T* item;
+};
+
+namespace std {
+ template <class T1, class T2>
+ struct pair {
+ T1 first;
+ T2 second;
+
+ pair() : first(), second() {}
+ pair(const T1 &a, const T2 &b) : first(a), second(b) {}
+
+ template<class U1, class U2>
+ pair(const pair<U1, U2> &other) : first(other.first),
+ second(other.second) {}
+ };
+
+ template<class T2, class T1>
+ T2& get(pair<T1, T2>& p) ;
+ template<class T1, class T2>
+ T1& get(const pair<T1, T2>& p) ;
+
+ typedef __typeof__(sizeof(int)) size_t;
+
+ template <class T> class initializer_list;
+
+ template< class T > struct remove_reference {typedef T type;};
+ template< class T > struct remove_reference<T&> {typedef T type;};
+ template< class T > struct remove_reference<T&&> {typedef T type;};
+
+ template<typename T> typename remove_reference<T>::type&& move(T&& a);
+ template <typename T> T *__addressof(T &x);
+ template <typename T> T *addressof(T &x);
+ template <typename T> const T& as_const(T& x);
+ template <typename T> T&& forward(T&& x);
+ // FIXME: Declare forward_like
+ // FIXME: Declare move_if_noexcept
+
+ template< class T >
+ using remove_reference_t = typename remove_reference<T>::type;
+
+ template <class T>
+ void swap(T &a, T &b) {
+ T c(std::move(a));
+ a = std::move(b);
+ b = std::move(c);
+ }
+
+ template<typename T>
+ class vector {
+ T *_start;
+ T *_finish;
+ T *_end_of_storage;
+
+ public:
+ typedef T value_type;
+ typedef size_t size_type;
+ typedef __vector_iterator<T, T *, T &> iterator;
+ typedef __vector_iterator<T, const T *, const T &> const_iterator;
+
+ vector() : _start(0), _finish(0), _end_of_storage(0) {}
+ template <typename InputIterator>
+ vector(InputIterator first, InputIterator last);
+ vector(const vector &other);
+ vector(vector &&other);
+ ~vector();
+
+ size_t size() const {
+ return size_t(_finish - _start);
+ }
+
+ vector& operator=(const vector &other);
+ vector& operator=(vector &&other);
+ vector& operator=(std::initializer_list<T> ilist);
+
+ void assign(size_type count, const T &value);
+ template <typename InputIterator >
+ void assign(InputIterator first, InputIterator last);
+ void assign(std::initializer_list<T> ilist);
+
+ void clear();
+
+ void push_back(const T &value);
+ void push_back(T &&value);
+ template<class... Args>
+ void emplace_back(Args&&... args);
+ void pop_back();
+
+ iterator insert(const_iterator position, const value_type &val);
+ iterator insert(const_iterator position, size_type n,
+ const value_type &val);
+ template <typename InputIterator>
+ iterator insert(const_iterator position, InputIterator first,
+ InputIterator last);
+ iterator insert(const_iterator position, value_type &&val);
+ iterator insert(const_iterator position, initializer_list<value_type> il);
+
+ template <class... Args>
+ iterator emplace(const_iterator position, Args&&... args);
+
+ iterator erase(const_iterator position);
+ iterator erase(const_iterator first, const_iterator last);
+
+ T &operator[](size_t n) {
+ return _start[n];
+ }
+
+ const T &operator[](size_t n) const {
+ return _start[n];
+ }
+
+ iterator begin() { return iterator(_start); }
+ const_iterator begin() const { return const_iterator(_start); }
+ const_iterator cbegin() const { return const_iterator(_start); }
+ iterator end() { return iterator(_finish); }
+ const_iterator end() const { return const_iterator(_finish); }
+ const_iterator cend() const { return const_iterator(_finish); }
+ T& front() { return *begin(); }
+ const T& front() const { return *begin(); }
+ T& back() { return *(end() - 1); }
+ const T& back() const { re...
[truncated]
|
|
@llvm/pr-subscribers-clang-tools-extra Author: None (vabridgers) ChangesRFC: This PR is not ready to merge yet, preliminary comments are welcome. Q) Is the name and file location ok in the directory structure? 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 clang-tidy checks 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. Patch is 75.02 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110471.diff 13 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 689eb92a3d8d17..7c177196d76f58 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -48,6 +48,7 @@
#include "MultipleStatementMacroCheck.h"
#include "NoEscapeCheck.h"
#include "NonZeroEnumToBoolConversionCheck.h"
+#include "NondeterministicPointerUsageCheck.h"
#include "NotNullTerminatedResultCheck.h"
#include "OptionalValueConversionCheck.h"
#include "ParentVirtualCallCheck.h"
@@ -179,6 +180,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
"bugprone-narrowing-conversions");
CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
+ CheckFactories.registerCheck<NondeterministicPointerUsageCheck>(
+ "bugprone-nondeterministic-pointer-usage");
CheckFactories.registerCheck<NonZeroEnumToBoolConversionCheck>(
"bugprone-non-zero-enum-to-bool-conversion");
CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index cb0d8ae98bac58..5628572b984226 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -44,6 +44,7 @@ add_clang_library(clangTidyBugproneModule
MultipleNewInOneExpressionCheck.cpp
MultipleStatementMacroCheck.cpp
NoEscapeCheck.cpp
+ NondeterministicPointerUsageCheck.cpp
NonZeroEnumToBoolConversionCheck.cpp
NotNullTerminatedResultCheck.cpp
OptionalValueConversionCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
new file mode 100644
index 00000000000000..ddc314af739d97
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
@@ -0,0 +1,67 @@
+//===--- NondetermnisticPointerUsageCheck.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 "NondeterministicPointerUsageCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void NondeterministicPointerUsageCheck::registerMatchers(MatchFinder *Finder) {
+
+ auto LoopVariable = varDecl(hasType(hasCanonicalType(pointerType())));
+
+ auto RangeInit = declRefExpr(to(varDecl(hasType(recordDecl(
+ anyOf(hasName("std::unordered_set"), hasName("std::unordered_map"),
+ hasName("std::unordered_multiset"),
+ hasName("std::unordered_multimap")))))));
+
+ Finder->addMatcher(
+ stmt(cxxForRangeStmt(hasRangeInit(RangeInit.bind("rangeinit")),
+ hasLoopVariable(LoopVariable.bind("loopVar"))))
+ .bind("cxxForRangeStmt"),
+ this);
+
+ auto SortFuncM = anyOf(callee(functionDecl(hasName("std::is_sorted"))),
+ callee(functionDecl(hasName("std::nth_element"))),
+ callee(functionDecl(hasName("std::sort"))),
+ callee(functionDecl(hasName("std::partial_sort"))),
+ callee(functionDecl(hasName("std::partition"))),
+ callee(functionDecl(hasName("std::stable_partition"))),
+ callee(functionDecl(hasName("std::stable_sort"))));
+
+ auto IteratesPointerEltsM = hasArgument(
+ 0,
+ cxxMemberCallExpr(on(hasType(cxxRecordDecl(has(fieldDecl(hasType(
+ hasCanonicalType(pointsTo(hasCanonicalType(pointerType())))))))))));
+
+ Finder->addMatcher(stmt(callExpr(allOf(SortFuncM, IteratesPointerEltsM)))
+ .bind("sortsemantic"),
+ this);
+}
+
+void NondeterministicPointerUsageCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *ForRangePointers =
+ Result.Nodes.getNodeAs<Stmt>("cxxForRangeStmt");
+ const auto *SortPointers = Result.Nodes.getNodeAs<Stmt>("sortsemantic");
+
+ if ((ForRangePointers) && !(ForRangePointers->getBeginLoc().isMacroID())) {
+ const auto *Node = dyn_cast<CXXForRangeStmt>(ForRangePointers);
+ diag(Node->getRParenLoc(), "Iteration of pointers is nondeterministic");
+ }
+
+ if ((SortPointers) && !(SortPointers->getBeginLoc().isMacroID())) {
+ const auto *Node = dyn_cast<Stmt>(SortPointers);
+ diag(Node->getBeginLoc(), "Sorting pointers is nondeterministic");
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
new file mode 100644
index 00000000000000..556a4fce4dffe2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
@@ -0,0 +1,36 @@
+//===--- NondeterministicPointerUsageCheck.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__NONDETERMINISTICPOINTERUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE__NONDETERMINISTICPOINTERUSAGECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds temporaries that look like RAII objects.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.html
+class NondeterministicPointerUsageCheck : public ClangTidyCheck {
+public:
+ NondeterministicPointerUsageCheck(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__NONDETERMINISTICPOINTERUSAGECHECK_H
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
new file mode 100644
index 00000000000000..8625487c6d694a
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - bugprone-nondeterministic-pointer-usage
+
+nondeterministic-pointer-usage
+==============================
+
+Finds nondeterministic usages of pointers in unordered containers.
+
+One canonical example is iteration across a container of pointers.
+
+.. code-block:: c++
+
+ {
+ for (auto i : UnorderedPtrSet)
+ f(i);
+ }
+
+Another such example is sorting a container of pointers.
+
+.. code-block:: c++
+
+ {
+ 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. Other
+similar usages will not be found and are false negatives.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
new file mode 100644
index 00000000000000..b279a8f20d8ddd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
@@ -0,0 +1,1450 @@
+// Like the compiler, the static analyzer treats some functions differently if
+// they come from a system header -- for example, it is assumed that system
+// functions do not arbitrarily free() their parameters, and that some bugs
+// found in system headers cannot be fixed by the user and should be
+// suppressed.
+#pragma clang system_header
+
+typedef unsigned char uint8_t;
+
+typedef __typeof__(sizeof(int)) size_t;
+typedef __typeof__((char*)0-(char*)0) ptrdiff_t;
+void *memmove(void *s1, const void *s2, size_t n);
+
+namespace std {
+ typedef size_t size_type;
+#if __cplusplus >= 201103L
+ using nullptr_t = decltype(nullptr);
+#endif
+}
+
+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;
+ };
+}
+
+template <typename T, typename Ptr, typename Ref> struct __vector_iterator {
+ typedef __vector_iterator<T, T *, T &> iterator;
+ typedef __vector_iterator<T, const T *, const T &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __vector_iterator(const Ptr p = 0) : ptr(p) {}
+ __vector_iterator(const iterator &rhs): ptr(rhs.base()) {}
+ __vector_iterator<T, Ptr, Ref>& operator++() { ++ ptr; return *this; }
+ __vector_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ ++ ptr;
+ return tmp;
+ }
+ __vector_iterator<T, Ptr, Ref> operator--() { -- ptr; return *this; }
+ __vector_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this; -- ptr;
+ return tmp;
+ }
+ __vector_iterator<T, Ptr, Ref> operator+(difference_type n) {
+ return ptr + n;
+ }
+ friend __vector_iterator<T, Ptr, Ref> operator+(
+ difference_type n,
+ const __vector_iterator<T, Ptr, Ref> &iter) {
+ return n + iter.ptr;
+ }
+ __vector_iterator<T, Ptr, Ref> operator-(difference_type n) {
+ return ptr - n;
+ }
+ __vector_iterator<T, Ptr, Ref> operator+=(difference_type n) {
+ return ptr += n;
+ }
+ __vector_iterator<T, Ptr, Ref> operator-=(difference_type n) {
+ return ptr -= n;
+ }
+
+ template<typename U, typename Ptr2, typename Ref2>
+ difference_type operator-(const __vector_iterator<U, Ptr2, Ref2> &rhs);
+
+ Ref operator*() const { return *ptr; }
+ Ptr operator->() const { return ptr; }
+
+ Ref operator[](difference_type n) {
+ return *(ptr+n);
+ }
+
+ bool operator==(const iterator &rhs) const { return ptr == rhs.ptr; }
+ bool operator==(const const_iterator &rhs) const { return ptr == rhs.ptr; }
+
+ bool operator!=(const iterator &rhs) const { return ptr != rhs.ptr; }
+ bool operator!=(const const_iterator &rhs) const { return ptr != rhs.ptr; }
+
+ const Ptr& base() const { return ptr; }
+
+private:
+ Ptr ptr;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __deque_iterator {
+ typedef __deque_iterator<T, T *, T &> iterator;
+ typedef __deque_iterator<T, const T *, const T &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::random_access_iterator_tag iterator_category;
+
+ __deque_iterator(const Ptr p = 0) : ptr(p) {}
+ __deque_iterator(const iterator &rhs): ptr(rhs.base()) {}
+ __deque_iterator<T, Ptr, Ref>& operator++() { ++ ptr; return *this; }
+ __deque_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ ++ ptr;
+ return tmp;
+ }
+ __deque_iterator<T, Ptr, Ref> operator--() { -- ptr; return *this; }
+ __deque_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this; -- ptr;
+ return tmp;
+ }
+ __deque_iterator<T, Ptr, Ref> operator+(difference_type n) {
+ return ptr + n;
+ }
+ friend __deque_iterator<T, Ptr, Ref> operator+(
+ difference_type n,
+ const __deque_iterator<T, Ptr, Ref> &iter) {
+ return n + iter.ptr;
+ }
+ __deque_iterator<T, Ptr, Ref> operator-(difference_type n) {
+ return ptr - n;
+ }
+ __deque_iterator<T, Ptr, Ref> operator+=(difference_type n) {
+ return ptr += n;
+ }
+ __deque_iterator<T, Ptr, Ref> operator-=(difference_type n) {
+ return ptr -= n;
+ }
+
+ Ref operator*() const { return *ptr; }
+ Ptr operator->() const { return ptr; }
+
+ Ref operator[](difference_type n) {
+ return *(ptr+n);
+ }
+
+ bool operator==(const iterator &rhs) const { return ptr == rhs.ptr; }
+ bool operator==(const const_iterator &rhs) const { return ptr == rhs.ptr; }
+
+ bool operator!=(const iterator &rhs) const { return ptr != rhs.ptr; }
+ bool operator!=(const const_iterator &rhs) const { return ptr != rhs.ptr; }
+
+ const Ptr& base() const { return ptr; }
+
+private:
+ Ptr ptr;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __list_iterator {
+ typedef __list_iterator<T, __typeof__(T::data) *, __typeof__(T::data) &> iterator;
+ typedef __list_iterator<T, const __typeof__(T::data) *, const __typeof__(T::data) &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::bidirectional_iterator_tag iterator_category;
+
+ __list_iterator(T* it = 0) : item(it) {}
+ __list_iterator(const iterator &rhs): item(rhs.item) {}
+ __list_iterator<T, Ptr, Ref>& operator++() { item = item->next; return *this; }
+ __list_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ item = item->next;
+ return tmp;
+ }
+ __list_iterator<T, Ptr, Ref> operator--() { item = item->prev; return *this; }
+ __list_iterator<T, Ptr, Ref> operator--(int) {
+ auto tmp = *this;
+ item = item->prev;
+ return tmp;
+ }
+
+ Ref operator*() const { return item->data; }
+ Ptr operator->() const { return &item->data; }
+
+ bool operator==(const iterator &rhs) const { return item == rhs->item; }
+ bool operator==(const const_iterator &rhs) const { return item == rhs->item; }
+
+ bool operator!=(const iterator &rhs) const { return item != rhs->item; }
+ bool operator!=(const const_iterator &rhs) const { return item != rhs->item; }
+
+ const T* &base() const { return item; }
+
+ template <typename UT, typename UPtr, typename URef>
+ friend struct __list_iterator;
+
+private:
+ T* item;
+};
+
+template <typename T, typename Ptr, typename Ref> struct __fwdl_iterator {
+ typedef __fwdl_iterator<T, __typeof__(T::data) *, __typeof__(T::data) &> iterator;
+ typedef __fwdl_iterator<T, const __typeof__(T::data) *, const __typeof__(T::data) &> const_iterator;
+
+ typedef ptrdiff_t difference_type;
+ typedef T value_type;
+ typedef Ptr pointer;
+ typedef Ref reference;
+ typedef std::forward_iterator_tag iterator_category;
+
+ __fwdl_iterator(T* it = 0) : item(it) {}
+ __fwdl_iterator(const iterator &rhs): item(rhs.item) {}
+ __fwdl_iterator<T, Ptr, Ref>& operator++() { item = item->next; return *this; }
+ __fwdl_iterator<T, Ptr, Ref> operator++(int) {
+ auto tmp = *this;
+ item = item->next;
+ return tmp;
+ }
+ Ref operator*() const { return item->data; }
+ Ptr operator->() const { return &item->data; }
+
+ bool operator==(const iterator &rhs) const { return item == rhs->item; }
+ bool operator==(const const_iterator &rhs) const { return item == rhs->item; }
+
+ bool operator!=(const iterator &rhs) const { return item != rhs->item; }
+ bool operator!=(const const_iterator &rhs) const { return item != rhs->item; }
+
+ const T* &base() const { return item; }
+
+ template <typename UT, typename UPtr, typename URef>
+ friend struct __fwdl_iterator;
+
+private:
+ T* item;
+};
+
+namespace std {
+ template <class T1, class T2>
+ struct pair {
+ T1 first;
+ T2 second;
+
+ pair() : first(), second() {}
+ pair(const T1 &a, const T2 &b) : first(a), second(b) {}
+
+ template<class U1, class U2>
+ pair(const pair<U1, U2> &other) : first(other.first),
+ second(other.second) {}
+ };
+
+ template<class T2, class T1>
+ T2& get(pair<T1, T2>& p) ;
+ template<class T1, class T2>
+ T1& get(const pair<T1, T2>& p) ;
+
+ typedef __typeof__(sizeof(int)) size_t;
+
+ template <class T> class initializer_list;
+
+ template< class T > struct remove_reference {typedef T type;};
+ template< class T > struct remove_reference<T&> {typedef T type;};
+ template< class T > struct remove_reference<T&&> {typedef T type;};
+
+ template<typename T> typename remove_reference<T>::type&& move(T&& a);
+ template <typename T> T *__addressof(T &x);
+ template <typename T> T *addressof(T &x);
+ template <typename T> const T& as_const(T& x);
+ template <typename T> T&& forward(T&& x);
+ // FIXME: Declare forward_like
+ // FIXME: Declare move_if_noexcept
+
+ template< class T >
+ using remove_reference_t = typename remove_reference<T>::type;
+
+ template <class T>
+ void swap(T &a, T &b) {
+ T c(std::move(a));
+ a = std::move(b);
+ b = std::move(c);
+ }
+
+ template<typename T>
+ class vector {
+ T *_start;
+ T *_finish;
+ T *_end_of_storage;
+
+ public:
+ typedef T value_type;
+ typedef size_t size_type;
+ typedef __vector_iterator<T, T *, T &> iterator;
+ typedef __vector_iterator<T, const T *, const T &> const_iterator;
+
+ vector() : _start(0), _finish(0), _end_of_storage(0) {}
+ template <typename InputIterator>
+ vector(InputIterator first, InputIterator last);
+ vector(const vector &other);
+ vector(vector &&other);
+ ~vector();
+
+ size_t size() const {
+ return size_t(_finish - _start);
+ }
+
+ vector& operator=(const vector &other);
+ vector& operator=(vector &&other);
+ vector& operator=(std::initializer_list<T> ilist);
+
+ void assign(size_type count, const T &value);
+ template <typename InputIterator >
+ void assign(InputIterator first, InputIterator last);
+ void assign(std::initializer_list<T> ilist);
+
+ void clear();
+
+ void push_back(const T &value);
+ void push_back(T &&value);
+ template<class... Args>
+ void emplace_back(Args&&... args);
+ void pop_back();
+
+ iterator insert(const_iterator position, const value_type &val);
+ iterator insert(const_iterator position, size_type n,
+ const value_type &val);
+ template <typename InputIterator>
+ iterator insert(const_iterator position, InputIterator first,
+ InputIterator last);
+ iterator insert(const_iterator position, value_type &&val);
+ iterator insert(const_iterator position, initializer_list<value_type> il);
+
+ template <class... Args>
+ iterator emplace(const_iterator position, Args&&... args);
+
+ iterator erase(const_iterator position);
+ iterator erase(const_iterator first, const_iterator last);
+
+ T &operator[](size_t n) {
+ return _start[n];
+ }
+
+ const T &operator[](size_t n) const {
+ return _start[n];
+ }
+
+ iterator begin() { return iterator(_start); }
+ const_iterator begin() const { return const_iterator(_start); }
+ const_iterator cbegin() const { return const_iterator(_start); }
+ iterator end() { return iterator(_finish); }
+ const_iterator end() const { return const_iterator(_finish); }
+ const_iterator cend() const { return const_iterator(_finish); }
+ T& front() { return *begin(); }
+ const T& front() const { return *begin(); }
+ T& back() { return *(end() - 1); }
+ const T& back() const { re...
[truncated]
|
NagyDonat
reviewed
Sep 30, 2024
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
Outdated
Show resolved
Hide resolved
vabridgers
force-pushed
the
nondeterministic-pointer-usage
branch
from
faf3644
to
60c0977
Compare
steakhal
reviewed
Sep 30, 2024
There was a problem hiding this comment.
I wish we had something like this. It might be possible, but really really challenging to implement.
To me the problem is that the iteration or lookups are not necessarily bad. And to determine if it's bad one needs to understand how the result of the lookup or the individual elements of the iteration are used.
For example, this is completely fine and efficient:
int numInterestingExprs = 0; // TODO: Use accumulate.
for (const Expr *E : MyBlocks/*some unordered set-like container*/) {
if (isInteresting(E)) ++numInterestingExprs;
}But this is not fine, because we apply a sideffect that is dependent on the nondeterministic iteration order. In some context, this could be still fine btw. It depends on what your constraints are:
for (const Expr *E : MyBlocks/*some unordered set-like container*/) {
if (isInteresting(E)) llvm::errs() << E->printPretty() << "\n"; // eeeh, the lines may get reshuffled from run-to-run...
}Only looking at the for loop here is not enough to decide this.
Limiting the check to only trigger if it sees the iteration AND also the use-site - like in the case you match std algorithms, like is_sorted or partition is a much more careful approach. It should work unless the algorithm takes a callback/functor that we can't really reason about and we could end up with a FP.
Remember that begin might be an ADL free-function, and not a memer function, such as for int *arr[10]. C++ also has std::ranges::*, which we may wanna cover too in the future.
You mentioned that you evaluated the checker at scale. What was the result of the evaluation? I assume you have seen FPs, and TPs as well.
clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.h
Outdated
Show resolved
Hide resolved
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
5chmidti and
HerrCai0907
earnol
reviewed
Sep 30, 2024
clang-tools-extra/clang-tidy/bugprone/NondeterministicPointerUsageCheck.cpp
Show resolved
Hide resolved
@vabridgers Definitely do such things in separate PRs. It makes the current patch cleaner if the additions to the sysroot simulation only contains what is needed for your patch. |
whisperity
requested changes
Sep 30, 2024
There was a problem hiding this comment.
(Also, for reference, I dug out the original CSA checkers' initial reviews from the history: https://reviews.llvm.org/D50488 and https://reviews.llvm.org/D59279.)
Even though the removed checks are alpha, maybe their removal is worth a mention in the release notes for CSA, if we have such a thing in the first place. @Szelethus?
clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/system-header-simulator-cxx.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/nondeterministic-pointer-usage.cpp
Outdated
Show resolved
Hide resolved
And the issue here is that, in the extreme case, even a completely omniscient call graph oracle would still fail at proving immunity to non-determinism even in your second case, because the determinism requirement could come from something like I'm unsure as to what the right approach would be here. I was thinking about instead marking every variable or field declared with a type like But your second example, @steakhal, touches on the problem nicely. What if we tried to match archetypes of "container elements are printed" or "container elements are sent to the network"? It will never be a complete list, but we could reasonably assume (at least this is a safe-ish generalisation, and for everything else, there is NOLINT...) that if the order escapes via some I/O, then non-determinism is... maybe if not outright bad, but definitely uncanny, if for nothing else, due to the troubles it causes when debugging. 😉 |
whisperity
reviewed
Sep 30, 2024
clang-tools-extra/docs/clang-tidy/checks/bugprone/nondeterministic-pointer-usage.rst
Outdated
Show resolved
Hide resolved
vabridgers
force-pushed
the
nondeterministic-pointer-usage
branch
from
60c0977
to
52e3908
Compare
Hi @steakhal , I completed a test run on the LLVM source base and detected no issues with this check. I will be expanding the tests, and wanted to mention I just found
I completed an initial test run on the llvm/clang repo with no crashes or results at all. Meaning no FPs or TPs. I will be expanding the tests to more repos, and would love to hear suggestions if you have any. I suppose abseil would be a good one. Also just found @Xazax-hun 's testbench to help with this here - https://github.com/Xazax-hun/csa-testbench. Really looking forward to trying that :) Thanks for the comments. Will keep driving this forward. |
Why isn't |
|
Maybe i misunderstood, I thought you meant creating a new clang-tidy module called |
|
As I understand the collective comments so far:
Next update should address these points. Thanks for the comments! |
vabridgers
force-pushed
the
nondeterministic-pointer-usage
branch
from
52e3908
to
4d8d105
Compare
|
Hi all, I believe all comments have been addressed. Thanks. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…tidy 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 clang-tidy checks 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.
vabridgers
force-pushed
the
nondeterministic-pointer-usage
branch
from
4d8d105
to
3ab0347
Compare
vabridgers
changed the title
5chmidti
requested changes
Oct 8, 2024
There was a problem hiding this comment.
I've added comments for cleaning up the implementation, and also questions/suggestions for future enhancements that are prefixed with ~:. I went with basically-NFC-cleanups, instead of improvements, in order to have this PR stay a moving to clang-tidy PR but improving the implementation quality.
There are various points that should be addressed to enhance the check after this lands, and we should think about better detection rates, as discussed above, some more.
Comment on lines
+178
to
+179
| CheckFactories.registerCheck<NondeterministicPointerUsageCheck>( | ||
| "bugprone-nondeterministic-pointer-iteration-order"); |
There was a problem hiding this comment.
The class and the check have different names, please choose one as that is an expectable pattern, and helps in navigating the files.
In case of the class and files needing a rename, checkout https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/rename_check.py)
Comment on lines
+21
to
+24
| auto RangeInit = declRefExpr(to(varDecl(hasType(recordDecl( | ||
| anyOf(hasName("std::unordered_set"), hasName("std::unordered_map"), | ||
| hasName("std::unordered_multiset"), | ||
| hasName("std::unordered_multimap"))))))); |
There was a problem hiding this comment.
Please change these multiple hasName matchers to the hasAnyName matcher (variadic)
Comment on lines
+32
to
+38
| auto SortFuncM = anyOf(callee(functionDecl(hasName("std::is_sorted"))), | ||
| callee(functionDecl(hasName("std::nth_element"))), | ||
| callee(functionDecl(hasName("std::sort"))), | ||
| callee(functionDecl(hasName("std::partial_sort"))), | ||
| callee(functionDecl(hasName("std::partition"))), | ||
| callee(functionDecl(hasName("std::stable_partition"))), | ||
| callee(functionDecl(hasName("std::stable_sort")))); |
There was a problem hiding this comment.
Same as above with hasName, but this can also be replaced with
auto SortFuncM = callee(functionDecl(hasAnyName(...)))|
|
||
| auto LoopVariable = varDecl(hasType(hasCanonicalType(pointerType()))); | ||
|
|
||
| auto RangeInit = declRefExpr(to(varDecl(hasType(recordDecl( |
There was a problem hiding this comment.
~: If you use varDecl(hasType(qualType(hasCanonicalType(...)))), variables where the type is an alias to one of these, will also be found
|
|
||
| auto IteratesPointerEltsM = hasArgument( | ||
| 0, | ||
| cxxMemberCallExpr(on(hasType(cxxRecordDecl(has(fieldDecl(hasType( |
There was a problem hiding this comment.
~: hasType -> hasType(qualType(hasCanonicalType( with the same aliasing reasoning as above
| Key *ptr; | ||
| }; | ||
|
|
||
| public: |
Comment on lines
+18
to
+27
| 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; | ||
| }; | ||
| public: |
There was a problem hiding this comment.
Nit: Weird indentation + not needed public in ln 27
| Key *ptr; | ||
| }; | ||
|
|
||
| public: |
| @@ -121,6 +121,12 @@ New checks | |||
| Gives warnings for tagged unions, where the number of tags is | |||
| different from the number of data members inside the union. | |||
|
|
|||
| - New :doc:`bugprone-nondeterministic-pointer-iteration-order | |||
There was a problem hiding this comment.
Please sort this in alphabetically, so one entry up
Comment on lines
+37
to
+38
| for (auto &i : UnorderedPtrSet) // no-warning | ||
| f(i); |
There was a problem hiding this comment.
~: Don't we want to detect this case as well?
Fine with me, yes, but there needs to be some sort of plan on how to address some of the pointed out challenges with this check, that were raised by others.
+1
That's of course possible, but may grow to 'too many' patterns fast, and then there comes the question of which ones to add and which are considered to be too infrequent. The other issue is that doing this would mean that either clang-tidy has to encode the use of many different libraries, and how side effects can happen by using them, or by providing customization options, which would in turn be limited by the patterns that are considered generic enough to warrant the customizable detection pattern (e.g., printing and formatting, and then there are Maybe (maybe not literally, to avoid using the parent map too much for traversal) For calling functions, there could also be a (cached) lookup of their bodies (if available), using |
|
@5chmidti , thanks for the comments. I'll start to work through those. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change moves the
alpha.nondeterministic.PointerSortingandalpha.nondeterministic.PointerIterationstatic analyzer checkers to a singleclang-tidycheck. Those checkers were implemented as simpleclang-tidycheck-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.