Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -8,6 +8,7 @@ InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp MacroParenthesesCheck.cpp + MacroRepeatedSideEffectsCheck.cpp MiscTidyModule.cpp NoexceptMoveConstructorCheck.cpp StaticAssertCheck.cpp Index: clang-tidy/misc/MacroRepeatedSideEffectsCheck.h =================================================================== --- clang-tidy/misc/MacroRepeatedSideEffectsCheck.h +++ clang-tidy/misc/MacroRepeatedSideEffectsCheck.h @@ -0,0 +1,32 @@ +//===--- MacroRepeatedSideEffectsCheck.h - clang-tidy -----------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MACRO_REPEATED_SIDE_EFFECTS_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MACRO_REPEATED_SIDE_EFFECTS_CHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// \brief Checks for repeated argument with side effects in macros. +/// +class MacroRepeatedSideEffectsCheck : public ClangTidyCheck { +public: + MacroRepeatedSideEffectsCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerPPCallbacks(CompilerInstance &Compiler) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MACRO_REPEATED_SIDE_EFFECTS_CHECK_H Index: clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp =================================================================== --- clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp +++ clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp @@ -0,0 +1,146 @@ +//===--- MacroRepeatedSideEffectsCheck.cpp - clang-tidy--------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "MacroRepeatedSideEffectsCheck.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/PPCallbacks.h" +#include "clang/Lex/Preprocessor.h" +#include "clang/Lex/MacroArgs.h" + +namespace clang { +namespace tidy { +namespace misc { + +namespace { +class MacroRepeatedPPCallbacks : public PPCallbacks { +public: + MacroRepeatedPPCallbacks(ClangTidyCheck &Check, SourceManager &SM, + Preprocessor &PP) + : Check(Check), SM(SM), PP(PP) {} + + void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD, + SourceRange Range, const MacroArgs *Args) override; + +private: + ClangTidyCheck &Check; + SourceManager &SM; + Preprocessor &PP; + + unsigned CountArgumentExpansions(const MacroInfo *MI, + const IdentifierInfo *Arg) const; + + bool HasSideEffects(const Token *ResultArgToks) const; +}; +} // namespace + +void MacroRepeatedPPCallbacks::MacroExpands(const Token &MacroNameTok, + const MacroDefinition &MD, + SourceRange Range, + const MacroArgs *Args) { + // Ignore macro argument expansions. + if (!Range.getBegin().isFileID()) + return; + + const MacroInfo *MI = MD.getMacroInfo(); + + // Bail out if the contents of the macro are containing keywords that are + // making the macro too complex. + if (std::find_if( + MI->tokens().begin(), MI->tokens().end(), [](const Token &T) { + return T.isOneOf(tok::question, tok::kw_if, tok::kw_else, + tok::kw_switch, tok::kw_case, tok::kw_break, + tok::kw_while, tok::kw_do, tok::kw_for, + tok::kw_continue, tok::kw_goto, tok::kw_return); + }) != MI->tokens().end()) + return; + + for (unsigned ArgNo = 0U; ArgNo < MI->getNumArgs(); ++ArgNo) { + const IdentifierInfo *Arg = *(MI->arg_begin() + ArgNo); + const Token *ResultArgToks = Args->getUnexpArgument(ArgNo); + + if (CountArgumentExpansions(MI, Arg) < 2) + continue; + + if (!HasSideEffects(ResultArgToks)) + continue; + + Check.diag(ResultArgToks->getLocation(), + "side effects in the %ordinal0 macro argument '%1' are repeated " + "in macro expansion") + << (ArgNo + 1) << Arg->getName(); + Check.diag(MI->getDefinitionLoc(), "macro %0 defined here", + DiagnosticIDs::Note) + << MacroNameTok.getIdentifierInfo(); + } +} + +unsigned MacroRepeatedPPCallbacks::CountArgumentExpansions( + const MacroInfo *MI, const IdentifierInfo *Arg) const { + unsigned CountInMacro = 0; + bool SkipParen = false; + int SkipParenCount = 0; + for (const auto &T : MI->tokens()) { + // If current token is a parenthesis, skip it. + if (SkipParen) { + if (T.is(tok::l_paren)) + SkipParenCount++; + else if (T.is(tok::r_paren)) + SkipParenCount--; + SkipParen = (SkipParenCount != 0); + if (SkipParen) + continue; + } + + IdentifierInfo *TII = T.getIdentifierInfo(); + // If not existent, skip it. + if (TII == nullptr) + continue; + + // If a builtin is found within the macro definition, skip next + // parenthesis. + if (TII->getBuiltinID() != 0) { + SkipParen = true; + continue; + } + + // If another macro is found within the macro definition, skip the macro + // and the eventual arguments. + if (TII->hasMacroDefinition()) { + const MacroInfo *M = PP.getMacroDefinition(TII).getMacroInfo(); + if (M != nullptr && M->isFunctionLike()) + SkipParen = true; + continue; + } + + // Count argument. + if (TII == Arg) + CountInMacro++; + } + return CountInMacro; +} + +bool MacroRepeatedPPCallbacks::HasSideEffects( + const Token *ResultArgToks) const { + for (; ResultArgToks->isNot(tok::eof); ++ResultArgToks) { + if (ResultArgToks->isOneOf(tok::plusplus, tok::minusminus)) + return true; + } + return false; +} + +void MacroRepeatedSideEffectsCheck::registerPPCallbacks( + CompilerInstance &Compiler) { + Compiler.getPreprocessor().addPPCallbacks( + ::llvm::make_unique( + *this, Compiler.getSourceManager(), Compiler.getPreprocessor())); +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -17,6 +17,7 @@ #include "InaccurateEraseCheck.h" #include "InefficientAlgorithmCheck.h" #include "MacroParenthesesCheck.h" +#include "MacroRepeatedSideEffectsCheck.h" #include "NoexceptMoveConstructorCheck.h" #include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" @@ -45,6 +46,8 @@ "misc-inefficient-algorithm"); CheckFactories.registerCheck( "misc-macro-parentheses"); + CheckFactories.registerCheck( + "misc-macro-repeated-side-effects"); CheckFactories.registerCheck( "misc-noexcept-move-constructor"); CheckFactories.registerCheck( Index: test/clang-tidy/misc-repeated-side-effects-in-macro.c =================================================================== --- test/clang-tidy/misc-repeated-side-effects-in-macro.c +++ test/clang-tidy/misc-repeated-side-effects-in-macro.c @@ -0,0 +1,84 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-macro-repeated-side-effects %t +// REQUIRES: shell + +#define badA(x,y) ((x)+((x)+(y))+(y)) +void bad(int ret, int a, int b) { + ret = badA(a++, b); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: side effects in the 1st macro argument 'x' are repeated in macro expansion [misc-macro-repeated-side-effects] + ret = badA(++a, b); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: side effects in the 1st macro argument 'x' + ret = badA(a--, b); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: side effects in the 1st macro argument 'x' + ret = badA(--a, b); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: side effects in the 1st macro argument 'x' + ret = badA(a, b++); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: side effects in the 2nd macro argument 'y' + ret = badA(a, ++b); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: side effects in the 2nd macro argument 'y' + ret = badA(a, b--); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: side effects in the 2nd macro argument 'y' + ret = badA(a, --b); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: side effects in the 2nd macro argument 'y' +} + + +// False positive: Repeated side effects is intentional. +// It is hard to know when it's done by intention so right now we warn. +#define UNROLL(A) {A A} +void fp1(int i) { + UNROLL({ i++; }); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: side effects in the 1st macro argument 'A' +} + +// Do not produce a false positive on a strchr() macro. Explanation; Currently the '?' +// triggers the test to bail out, because it cannot evaluate __builtin_constant_p(c). +# define strchrs(s, c) \ + (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s) \ + && (c) == '\0' \ + ? (char *) __rawmemchr (s, c) \ + : __builtin_strchr (s, c))) +char* __rawmemchr(char* a, char b) { + return a; +} +void pass(char* pstr, char ch) { + strchrs(pstr, ch++); // No error. +} + +// Check large arguments (t=20, u=21). +#define largeA(a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, x, y, z) \ + ((a) + (a) + (b) + (b) + (c) + (c) + (d) + (d) + (e) + (e) + (f) + (f) + (g) + (g) + \ + (h) + (h) + (i) + (i) + (j) + (j) + (k) + (k) + (l) + (l) + (m) + (m) + (n) + (n) + \ + (o) + (o) + (p) + (p) + (q) + (q) + (r) + (r) + (s) + (s) + (t) + (t) + (u) + (u) + \ + (v) + (v) + (x) + (x) + (y) + (y) + (z) + (z)) +void large(int a) { + largeA(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, a++, 0, 0, 0, 0, 0, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:64: warning: side effects in the 19th macro argument 's' + largeA(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, a++, 0, 0, 0, 0, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:67: warning: side effects in the 20th macro argument 't' + largeA(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, a++, 0, 0, 0, 0); + // CHECK-MESSAGES: :[[@LINE-1]]:70: warning: side effects in the 21st macro argument 'u' +} + +// Builtins and macros should not be counted. +#define builtinA(x) (__builtin_constant_p(x) + (x)) +#define builtinB(x) (__builtin_constant_p(x) + (x) + (x)) +#define macroA(x) (builtinA(x) + (x)) +#define macroB(x) (builtinA(x) + (x) + (x)) +void builtins(int ret, int a) { + ret += builtinA(a++); + ret += builtinB(a++); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: side effects in the 1st macro argument 'x' + ret += macroA(a++); + ret += macroB(a++); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: side effects in the 1st macro argument 'x' +} + +// Bail out for conditionals. +#define condA(x,y) ((x)>(y)?(x):(y)) +#define condB(x,y) if(x) {x=y;} else {x=y + 1;} +void conditionals(int ret, int a, int b) +{ + ret += condA(a++, b++); + condB(a, b++); +} +