diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 435cb1e3fbcff..3d84b6138ad63 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -23,6 +23,7 @@ #include "DynamicStaticInitializersCheck.h" #include "EasilySwappableParametersCheck.h" #include "EmptyCatchCheck.h" +#include "EvalOrderCheck.h" #include "ExceptionEscapeCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" @@ -119,6 +120,7 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck( "bugprone-easily-swappable-parameters"); CheckFactories.registerCheck("bugprone-empty-catch"); + CheckFactories.registerCheck("bugprone-eval-order"); CheckFactories.registerCheck( "bugprone-exception-escape"); CheckFactories.registerCheck("bugprone-fold-init-type"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 70e7fbc7ec0c1..624a4f5cd95fe 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -18,6 +18,7 @@ add_clang_library(clangTidyBugproneModule DynamicStaticInitializersCheck.cpp EasilySwappableParametersCheck.cpp EmptyCatchCheck.cpp + EvalOrderCheck.cpp ExceptionEscapeCheck.cpp FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp new file mode 100644 index 0000000000000..e9f1c0f3c3c7d --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp @@ -0,0 +1,59 @@ +//===--- EvalOrderCheck.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 "EvalOrderCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include + +using namespace clang::ast_matchers; +using ::clang::ast_matchers::internal::Matcher; + +namespace clang::tidy::bugprone { + +EvalOrderCheck::EvalOrderCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + +bool EvalOrderCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + return LangOpts.CPlusPlus; +} + +void EvalOrderCheck::registerMatchers(MatchFinder *Finder) { + auto Ctor = cxxConstructExpr(unless(isListInitialization())).bind("ctor"); + auto Fun = callExpr().bind("fun"); + auto Mut = unless(hasDeclaration(cxxMethodDecl(isConst()))); + auto Mc1 = hasDescendant( + cxxMemberCallExpr( + Mut, callee(cxxMethodDecl(hasParent(recordDecl().bind("rd1"))))) + .bind("mc1")); + auto Mc2 = hasDescendant( + cxxMemberCallExpr( + unless(equalsBoundNode("mc1")), + callee(cxxMethodDecl(hasParent(recordDecl(equalsBoundNode("rd1")))))) + .bind("mc2")); + Finder->addMatcher(expr(anyOf(Ctor, Fun), allOf(Mc1, Mc2)), this); +} + +void EvalOrderCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Ctor = Result.Nodes.getNodeAs("ctor"); + const auto *Fun = Result.Nodes.getNodeAs("fun"); + + if (Ctor) { + diag(Ctor->getExprLoc(), "Order of evaluation of constructor " + "arguments is unspecified."); + } else { + diag(Fun->getExprLoc(), "Order of evaluation of function " + "arguments is unspecified. "); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h new file mode 100644 index 0000000000000..a7e96a4afa0a4 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h @@ -0,0 +1,32 @@ +//===--- EvalOrderCheck.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_EVALORDERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EVALORDERCHECK_H + +#include "../ClangTidyCheck.h" +#include + +namespace clang::tidy::bugprone { + +/// Detects and suggests addressing issues with unspecified evaluation order of +/// constructor and function parameters. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/eval-order.html +class EvalOrderCheck : public ClangTidyCheck { +public: + EvalOrderCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EVALORDERCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3437b6cf9b59e..9a684c18be70d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -149,6 +149,11 @@ New checks Detects equality comparison between pointer to member virtual function and anything other than null-pointer-constant. +- New :doc:`bugprone-eval-order ` check. + + Detects suspect code, which is assumed to rely on unspecified order of + evaluation of function and constructor parameters. + - New :doc:`bugprone-inc-dec-in-conditions ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst new file mode 100644 index 0000000000000..b91766365887d --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst @@ -0,0 +1,40 @@ +.. title:: clang-tidy - bugprone-eval-order + +bugprone-eval-order +=================== + +Order of evaluation is unspecified for function and constructor parameters, +unless the constructor uses C++11 list-initialization. + +This may lead to issues in code that is written with the assumption of a +specified evaluation order. + +This tidy rule will print a warning when at least two parameters in a call are +sourced from any method of a class, and at least one of those method calls is +non-``const``. A fix is not offered. + +C++11 constructor list-initialization are evaluated in order of appearance and +can be used as a fix. + +In case of false positives, the check can be suppressed, or disabled completely. + +``static`` methods and global functions may also lead to issues, but they are +not considered in this check, for now. + +The check is limited to member functions, because presumably the most common +issue in real code is some kind of reader object: + +.. code-block:: c++ + + struct Reader { + char buf[4]{}; + int pos{}; + char Pop() { return buf[pos++]; } + }; + + int Calc(char byte1, char byte2) { return byte1 > byte2; }; + + int main() { + Reader r{{1, 2, 3}}; + return Calc(r.Pop(), r.Pop()); // May return 0 or 1 + } diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2f86121ad8729..f24e3240d6d23 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -89,6 +89,7 @@ Clang-Tidy Checks :doc:`bugprone-dynamic-static-initializers `, :doc:`bugprone-easily-swappable-parameters `, :doc:`bugprone-empty-catch `, + :doc:`bugprone-eval-order `, :doc:`bugprone-exception-escape `, :doc:`bugprone-fold-init-type `, :doc:`bugprone-forward-declaration-namespace `, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp new file mode 100644 index 0000000000000..2369b02f0d1bf --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp @@ -0,0 +1,46 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-eval-order %t + +struct Food { + int cnt{}; + int i() { return ++cnt; } + bool b() { return ++cnt > 1; } + int ci() const { return cnt; } + int cb() const { return cnt > 1; } + static int I() {return {};} +}; + +int I() { + static int cnt{}; + return ++cnt; +} + +struct Cat { + Cat(int, bool) {} +}; + +void Dog(int, bool) {} + +int copy(int i) { return i; } + +void function() { + Food f; + const Food cf; + const Food& crf{cf}; + Food& mutf{f}; + // Assume const member functions are fine + Cat a(cf.ci(), crf.cb()); + Cat(f.ci(), f.cb()); + Dog(f.cb(), f.ci()); + // Assume static and global functions are fine + Dog(f.I(), f.b()); + Dog(I(), I()); + // C++11 list-init is fine + Cat{f.i(), f.b()}; + // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: Order of evaluation of constructor arguments is unspecified. + Cat(f.i(), f.b()); + // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: Order of evaluation of function arguments is unspecified. + Dog(copy(mutf.i()), f.b()); + // ... even if one is const + // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: Order of evaluation of function arguments is unspecified. + Dog(f.i(), f.cb()); +}