-
Notifications
You must be signed in to change notification settings - Fork 11.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: [clang-tidy] [analyzer] Move nondeterministic pointer usage check to tidy #110471
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ~: If you use |
||
anyOf(hasName("std::unordered_set"), hasName("std::unordered_map"), | ||
hasName("std::unordered_multiset"), | ||
hasName("std::unordered_multimap"))))))); | ||
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change these multiple |
||
|
||
Finder->addMatcher( | ||
stmt(cxxForRangeStmt(hasRangeInit(RangeInit.bind("rangeinit")), | ||
hasLoopVariable(LoopVariable.bind("loopVar")))) | ||
Comment on lines
+27
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you swap the two inner matchers of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the binding of "loopVar", it is never used. |
||
.bind("cxxForRangeStmt"), | ||
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to wrap |
||
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")))); | ||
Comment on lines
+32
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above with auto SortFuncM = callee(functionDecl(hasAnyName(...))) |
||
|
||
auto IteratesPointerEltsM = hasArgument( | ||
0, | ||
cxxMemberCallExpr(on(hasType(cxxRecordDecl(has(fieldDecl(hasType( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ~: |
||
hasCanonicalType(pointsTo(hasCanonicalType(pointerType()))))))))))); | ||
vabridgers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Finder->addMatcher(stmt(callExpr(allOf(SortFuncM, IteratesPointerEltsM))) | ||
.bind("sortsemantic"), | ||
Comment on lines
+45
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to wrap |
||
this); | ||
} | ||
|
||
void NondeterministicPointerUsageCheck::check( | ||
const MatchFinder::MatchResult &Result) { | ||
const auto *ForRangePointers = | ||
Result.Nodes.getNodeAs<Stmt>("cxxForRangeStmt"); | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once the outer, unnecessary |
||
const auto *SortPointers = Result.Nodes.getNodeAs<Stmt>("sortsemantic"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be moved after the first if to avoid the lookup in case there is no need to do one |
||
|
||
if ((ForRangePointers) && !(ForRangePointers->getBeginLoc().isMacroID())) { | ||
const auto *Node = dyn_cast<CXXForRangeStmt>(ForRangePointers); | ||
diag(Node->getRParenLoc(), "Iteration of pointers is nondeterministic"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Diagnostics in clang-tidy are start lower-cased (+1 below) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the diagnostic anchored at the r-paren seems off to me. const auto *RangeInit = Result.Nodes.getNodeAs<Stmt>("rangeinit");
SourceRange R = RangeInit->getSourceRange();
diag(R.getBegin(), ...) << R; ( (+1 for the other diagnostic) |
||
} | ||
Comment on lines
+58
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a return here to exit early |
||
|
||
if ((SortPointers) && !(SortPointers->getBeginLoc().isMacroID())) { | ||
const auto *Node = dyn_cast<Stmt>(SortPointers); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unnecessary cast, given that |
||
diag(Node->getBeginLoc(), "Sorting pointers is nondeterministic"); | ||
} | ||
} | ||
|
||
} // namespace clang::tidy::bugprone |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
//===------- 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 { | ||
|
||
/// For the user-facing documentation see: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add the sentence from the release note here as well |
||
/// 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please sort this in alphabetically, so one entry up |
||
<clang-tidy/checks/bugprone/nondeterministic-pointer-iteration-order>` | ||
check. | ||
|
||
Finds nondeterministic usages of pointers in unordered containers. | ||
|
||
New check aliases | ||
^^^^^^^^^^^^^^^^^ | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
.. 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. Other | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unordered sets and maps |
||
similar usages will not be found and are false negatives. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
|
||
#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; | ||
}; | ||
public: | ||
Comment on lines
+18
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: weird indentation + not needed |
||
Key *val; | ||
iterator begin() const { return iterator(val); } | ||
iterator end() const { return iterator(val + 1); } | ||
}; | ||
|
||
} // namespace std | ||
|
||
#endif // _SIM_MAP |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
|
||
#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; | ||
}; | ||
|
||
public: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: not needed |
||
Key *val; | ||
iterator begin() const { return iterator(val); } | ||
iterator end() const { return iterator(val + 1); } | ||
}; | ||
|
||
} // namespace std | ||
|
||
#endif // _SIM_SET |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
#ifndef _SIM_STL_PAIR | ||
#define _SIM_STL_PAIR | ||
|
||
#pragma clang system_header | ||
|
||
#include "sim_type_traits" | ||
|
||
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 <typename T1, typename T2> | ||
pair<typename remove_reference<T1>::type, typename remove_reference<T2>::type> | ||
make_pair(T1 &&, T2 &&) { | ||
return {}; | ||
}; | ||
|
||
} // namespace std | ||
|
||
#endif // _SIM_STL_PAIR | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)