clang-tidy: check for repeated side effects in macro

[danielmarjamaki]

Summary

This is a new clang-tidy checker that warns when there is side effects in a macro argument that is repeated in the macro expansion.

I am running tests. Right now it has been tested on 7678 files in 180 projects. And there has been in total 7 warnings. As far as I see all these 7 warnings are real bugs.

Interestingly I get a couple of warnings for standard functions that are implemented as macros. As far as I see my headers are not standard-compliant:

format.c:774:28: warning: Side effects in macro argument is repeated in macro expansion [misc-macro-repeated-side-effects]
                tblt->instr[0] = toupper(*p++);
                                         ^
/usr/include/ctype.h:229:11: note: 'toupper' macro defined here
#  define toupper(c)    __tobody (c, toupper, *__ctype_toupper_loc (), (c))
          ^

tunnel.c:73:25: warning: Side effects in macro argument is repeated in macro expansion [misc-macro-repeated-side-effects]
      char *q = strchr (++p, '"');
                        ^
/usr/include/x86_64-linux-gnu/bits/string2.h:395:11: note: 'strchr' macro defined here
#  define strchr(s, c) \
  (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s)	      \
		  && (c) == '\0'					      \
		  ? (char *) __rawmemchr (s, c)				      \
		  : __builtin_strchr (s, c)))

[danielmarjamaki]

Interestingly I get a couple of warnings for standard functions that are implemented as macros. As far as I see my headers are not standard-compliant:

hmm. now I have the feeling I was wrong.

I don't know.. but I guess that probably those are some compile-time handling only.

[danielmarjamaki]

Now it doesn't warn about macros in system headers.

I have now tested it on 18045 files in 381 debian projects.

I saw warnings in 4 projects. In two projects I believe there are bugs. Imo the other two projects had false positives for these reasons:

1. repeated side effects is intended.
2. argument with side effects is repeated in macro, but in different conditional code paths, side effect is not repeated

For #1 I think the only reasonable solution is that this checker must be disabled by the user.

For #2 I think we could maybe fix some FPs in clang tidy by analysing the tokens. But it's not going to be easy and solid imho. I recommend that these users disable this warning. If we bailout when there are different conditional code paths there will be false negatives.

[alexfh]

Sorry for the delay. I was on vacation and currently digging out stuff out of my inbox. I'll try to review this thoroughly early next week. A few random comments for now.

In D9496#168953, @danielmarjamaki wrote:

Now it doesn't warn about macros in system headers.

It looks like the latest patch is exactly the same as the first one.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

42

The clang:: qualifier is not needed here and below.

51

Will a range-based for loop work here?

55

ditto

test/clang-tidy/misc-repeated-side-effects-in-macro.c

14

Do you need a separate function for each test?

Also, please leave the whole warning message once and truncate all the other instances to fit into 80 columns.

[danielmarjamaki]

Taken care of review comments. Dont warn about macros in system headers.

[alexfh]

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

48

nit: Please add a trailing period.

49

What system macros did get flagged here that you wouldn't want? Combining MIN/MAX with a side effect seems like a bad idea, for example.

53

So, will a range-based for loop work here?

Also, when I send patches for review, I find it convenient to answer each reviewer's comment with "Done" or a reason why the comment is not applicable. This helps keeping track of the issues I addressed.

77

s/sideeffects/side effects/

84

I'd make the message more specific by adding at least the index of the argument that is repeated. Maybe also add the name of the parameter in the note.

[danielmarjamaki]

Here are some results:
https://drive.google.com/file/d/0BykPmWrCOxt2YTdOU0Y3M1RmdGM/view?usp=sharing

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

49

For instance: toupper, strchr. I think these were false positives. :-(

For instance for strchr I assume "__builtin_constant_p (s)" does not use s at runtime and then it's safe to use s again later in the macro.

I don't see how we can teach the checker to not care about any compiletime behaviour. do you know if this is possible?

51

I don't see how I can do this without modifying the MacroInfo, I'd need a new method that returns a container with the arguments right? Maybe the MacroInfo::ArgumentList should be a container?

I would prefer to do such refactoring separately.

53

How?

Then I must modify MacroInfo right? Either the arguments must be stored in some container. Or else MacroInfo can have a method that creates a container with the arguments and returns it, I know this is not a good solution but seems easier for me to implement ;-).

I would prefer to not change MacroInfo in this patch.. but I could submit a parallell patch.

55

Impossible.

Fortunately I have a patch pending (http://reviews.llvm.org/D9079) that makes it possible to do this. If it gets the OK and I can apply it I will of course use the range-based for loop here too.

84

Yes I agree.. will do.

[alexfh]

In D9496#176202, @danielmarjamaki wrote:

Here are some results:
https://drive.google.com/file/d/0BykPmWrCOxt2YTdOU0Y3M1RmdGM/view?usp=sharing

Nice, but the results are from a different check: misc-macro-parentheses.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

49

I can find some definitions of these macros, but I'm not sure they would be the same definitions you are talking about. It would be easier if you quoted the definitions here.

53

Yes, you'll need to add a method that returns a "container" with arguments same way as in D9079:

ArrayList<IdentifierInfo*> args() const {
  return ArrayList<IdentifierInfo*>(ArgumentList, NumArguments);
}

55

Phabricator says that you have committed that patch 10 days ago:

Closed by commit rL236975: Refactor MacroInfo so range for loops can be used to iterate its tokens. (authored by danielmarjamaki)

[danielmarjamaki]

Here are some results:
https://drive.google.com/file/d/0BykPmWrCOxt2YTdOU0Y3M1RmdGM/view?usp=sharing

Nice, but the results are from a different check: misc-macro-parentheses.

I see.. I dont have the results anymore. I am rerunning the checker and will probably post results tomorrow.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

48

Will be fixed in next patch

55

Yes I will of course use this. I totally agree range for loops are better.

77

Will be fixed in next patch

[danielmarjamaki]

The script I use to test Clang on many packages:
https://drive.google.com/file/d/0BykPmWrCOxt2Zy1wVXZQWVV0LVE/view?usp=sharing

Background:
In the Cppcheck project we run Cppcheck on all debian packages continuously with a script. The results are shown here:
http://cppcheck.sourceforge.net/devinfo/daca2-report/daca2.html

I modified the script that runs Cppcheck so it runs Clang instead.

How to use it:

Create a folder:
mkdir ~/daca-clang

Run Clang on debian source code:
python daca-clang.py --folders=abc --cc_path=~/llvm/build/Debug+Asserts/bin1

--folders=abc:

Select folders to check. This will select the folders "a", "b" and "c".. these contain all packages that start with "a", "b" or "c".

--cc_path=...

Specify where clang is found

this will only run normal compilation. To run a static analyser checker, add --checker=name-of-checker. To run clang-tidy, use --clang-tidy (however this currently only runs misc-macro* checkers).

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

49

Here is the toupper macro:

#  define toupper(c)    __tobody (c, toupper, *__ctype_toupper_loc (), (c))

The c is repeated but I assume that __tobody doesnt use both argument 1 and 4 at runtime.

Here is the strchr macro:

#  define strchr(s, c) \
  (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s)	      \
		  && (c) == '\0'					      \
		  ? (char *) __rawmemchr (s, c)				      \
		  : __builtin_strchr (s, c)))

The s is used in the condition and then in both conditional expressions. However I assume that the usage in the condition is compile-time only.

[danielmarjamaki]

fixed comments from previous revision.

[alexfh]

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

75

This would also benefit from Token::isOneOf that you can move from clang/lib/Format/FormatToken.h to clang::Token.

97

You seem to assume that the macro is function-like, but you'd better check it (PP->getMacroDefinition(TII)->getMacroInfo()->isFunctionLike() + all required nullptr checks).

124

Let's use standard facilities provided by the diagnostics engine:

Check.diag(..., "side effects in the %ordinal0 macro argument ....

If you want this to be more verbose, you can combine this with a %select{} modifier to add spellings for the first few ordinals.

test/clang-tidy/misc-repeated-side-effects-in-macro.c

67

You can omit the " is repeated in macro expansion [misc-macro-repeated-side-effects]" part from all check lines but the first one to make the tests more readable.

[• edse]

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

75

Fixed (the addition of Token::IsOneOf will be added in another patch).

97

Actually the SkipParen bails if the next token is not a left parenthesis, but there may be complications if a parenthesis is following a macro without arguments.

I will implement your Idea in the next patch. It seems more roboust.

124

So it did exist, have to dig deeper next time... I replaced the Tiers-functions with %ordinal0 and I think it is satisfying.

test/clang-tidy/misc-repeated-side-effects-in-macro.c

67

will do, see next patch.

[danielmarjamaki]

Updated patch. Fixed comments and refactored.

[alexfh]

Thank you for the update!

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

57

s/Bail/Bail out/, otherwise it means something different. Also, "the contents" and s/Macro/the macro/ (there's no variable named Macro).

60

clang-format, please.

63

s/paranthesis/parenthesis/

64

Missing comma before "then".

84

The line is too long. Please clang-format.

87

s/NULL/nullptr/

115

Maybe "side-effects ... _are_ repeated"?

118

"macro %0 defined here" would read better (given %0 is the name of the macro).

test/clang-tidy/misc-repeated-side-effects-in-macro.c

8

I thought, "%ordinal0" would generate "1st", not "first". Did you run tests after making changes?

27

nit: missing trailing period.

34

Please use proper capitalization and punctuation in comments. Here and below.

35

Did you mean "bail out"? IMHO, "bail" means something different.

45

Space after //, capitalization, trailing period.

48

ditto

49

If the test doesn't need special formatting, please clang-format.

73

s/bailout/bail out/

[danielmarjamaki]

took care of review comments

[alexfh]

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

55

I find it hard to read this function due to many nested control statements. Let's split this loop in two separate checks:

if (std::find_if(MI->tokens().begin(), MI->tokens().end(), [] (const Token &T) { return T.isOneOf(...);}) != MI->tokens.end())
  return;

and

if (CountArgumentExpansions(Arg /* or ResultArgToks */, MI->tokens()) < 2)
  continue;

And move most of it to a new CountArgumentExpansions function (if you have a better name, feel free to use it).

57

nit: "content ... is" or "contents ... are"
Also, s/to complex/too complex/.

63

nit: It's more correct to use plural here: "parentheses starting at the current position."

76

s/NULL/nullptr/

114

Please use isOneOf.

118

Looks like you're calling MI->getArgumentNum(Arg) twice. Maybe store the result?

[danielmarjamaki]

New patch for this checker. Fixed comments, refactoring.

[alexfh]

Looks good now with one nit.

Thank you for another awesome check!

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

71

nit: And now I'd revert the order of checks (HasSideEffects seems to be slightly cheaper?) and make this even simpler:

if (HasSideEffects(ResultArgToks) && CountArgumentExpansions(MI, Arg) >= 2) {
  Check.diag(...) ...
}

[alexfh]

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

71

... reverse the order of checks ...

[danielmarjamaki]

Checked in with 239909.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

55

Fixed

57

Fixed

63

I rewrote this

// If current token is a parenthesis, skip it.

maybe it can be improved further

76

fixed

114

Fixed

118

Fixed

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   InaccurateEraseCheck.cpp
   InefficientAlgorithmCheck.cpp
+  MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UseOverrideCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/misc/MacroRepeatedSideEffectsCheck.h

+//===--- 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

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

+//===--- 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();
+
+  // Don't write warnings for macros defined in system headers
+  if (SM.isInSystemHeader(MI->getDefinitionLoc()))
+    return;
+
+  typedef MacroInfo::arg_iterator ArgIter;
+  for (ArgIter AI = MI->arg_begin(), AE = MI->arg_end(); AI != AE; ++AI) {
+    const Token *ResultArgToks =
+        Args->getUnexpArgument(MI->getArgumentNum((*AI)));
+    int CountInMacro = 0;
+    for (const auto &T : MI->tokens()) {
+      IdentifierInfo *TII = T.getIdentifierInfo();
+      int ArgNo = (TII ? MI->getArgumentNum(TII) : -1);
+      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 sideeffects 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 macro argument is repeated in macro expansion");
+        Check.diag(MI->getDefinitionLoc(), "%0 macro defined here",
+                   DiagnosticIDs::Note)
+            << MacroNameTok.getIdentifierInfo();
+      }
+    }
+  }
+}
+
+void MacroRepeatedSideEffectsCheck::registerPPCallbacks(
+    CompilerInstance &Compiler) {
+  Compiler.getPreprocessor().addPPCallbacks(
+      ::llvm::make_unique<MacroRepeatedPPCallbacks>(
+          *this, Compiler.getSourceManager(), Compiler.getPreprocessor()));
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "InefficientAlgorithmCheck.h"
+#include "MacroRepeatedSideEffectsCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UseOverrideCheck.h"
 
 namespace clang {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
         "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "misc-inaccurate-erase");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
+    CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
+        "misc-macro-repeated-side-effects");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");

test/clang-tidy/misc-repeated-side-effects-in-macro.c

+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-macro-repeated-side-effects %t
+// REQUIRES: shell
+
+
+#define MIN(A,B)     (((A)<(B)) ? (A) : (B))
+
+void bad(int i, int j) {
+  int x;
+  x = MIN(i++, j);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: side effects in macro argument is repeated in macro expansion [misc-macro-repeated-side-effects]
+  x = MIN(++i, j);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: side effects in macro argument is
+  x = MIN(i--, j);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: side effects in macro argument is
+  x = MIN(--i, j);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: side effects in macro argument is
+}
+
+
+// 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 macro argument is
+}
+
+
+// False positive: Side effects in different conditional branches
+// As the checker only has tokens to look at it can be hard to determine if
+// side effects are repeated in different branches. Possibly we could bail out
+// for instance whenever an if is seen but that leads to false negatives.
+#define IFELSE(A,B)  ({ if (A) B; else B; })
+void fp2(int a, int b) {
+  IFELSE(a,b++);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: side effects in macro argument is
+}