Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -7,6 +7,7 @@ BoolPointerImplicitConversionCheck.cpp InaccurateEraseCheck.cpp InefficientAlgorithmCheck.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 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,135 @@ +//===--- 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; +}; +} // namespace + +void MacroRepeatedPPCallbacks::MacroExpands(const Token &MacroNameTok, + const MacroDefinition &MD, + SourceRange Range, + const MacroArgs *Args) { + SourceLocation Loc = Range.getBegin(); + // Ignore macro argument expansions. + if (!Loc.isFileID()) + return; + + const MacroInfo *MI = MD.getMacroInfo(); + for (const IdentifierInfo *Arg : MI->args()) { + const Token *ResultArgToks = + Args->getUnexpArgument(MI->getArgumentNum(Arg)); + + int CountInMacro = 0; + bool SkipParen = false; + int SkipParenCount = 0; + for (const auto &T : MI->tokens()) { + // Bail out if the contents of the macro is containing keywords that are + // making the macro to complex. + if (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)) + return; + // Skip the whole parenthesis that is starting at the current position. If + // none is starting, then do not skip anything. + 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 == NULL) + 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()) { + MacroInfo *MI = PP.getMacroDefinition(TII).getMacroInfo(); + if (MI != nullptr && MI->isFunctionLike()) + SkipParen = true; + continue; + } + int ArgNo = MI->getArgumentNum(TII); + if (ArgNo == -1) { + // This isn't an argument, continue. + continue; + } + const Token *Res = Args->getUnexpArgument(ArgNo); + if (ResultArgToks == Res) + CountInMacro++; + } + + if (CountInMacro < 2) + continue; + + // If the arg token didn't expand into anything, ignore it. + if (ResultArgToks->is(tok::eof)) + continue; + unsigned NumToks = MacroArgs::getArgLength(ResultArgToks); + + // Check for side effects in the argument expansions. + for (unsigned ArgumentIndex = 0; ArgumentIndex < NumToks; ++ArgumentIndex) { + const Token &AT = ResultArgToks[ArgumentIndex]; + + if (AT.is(tok::plusplus) || AT.is(tok::minusminus)) { + Check.diag(ResultArgToks->getLocation(), + "side effects in the %ordinal0 macro argument '%1' is " + "repeated in macro expansion") + << (MI->getArgumentNum(Arg) + 1) << Arg->getName(); + Check.diag(MI->getDefinitionLoc(), "macro %0 defined here", + DiagnosticIDs::Note) + << MacroNameTok.getIdentifierInfo(); + } + } + } +} + +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 @@ -16,6 +16,7 @@ #include "BoolPointerImplicitConversionCheck.h" #include "InaccurateEraseCheck.h" #include "InefficientAlgorithmCheck.h" +#include "MacroRepeatedSideEffectsCheck.h" #include "NoexceptMoveConstructorCheck.h" #include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" @@ -42,6 +43,8 @@ "misc-inaccurate-erase"); CheckFactories.registerCheck( "misc-inefficient-algorithm"); + 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' is 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++); +} +