Improved checking for repeated macro argument with side effects when macro contains ?:

[danielmarjamaki]

Summary

This patch fixes possible false negatives when macro contains ?:.

I tested this improvemed checker on 1398 open source projects, but did not see any new warnings.. At least that means that it did not cause any new false positives even though it does fix the false negatives that are shown in the testing.

Question: Is it a good idea to use std::stack or would some other container be better in clang-tidy?

[alexfh]

Question: Is it a good idea to use std::stack or would some other container be better in clang-tidy?

You can try to use std::stack<unsigned, SmallVector<unsigned, 8>> to avoid heap allocation in most cases.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

126–127

I guess, the main use case is __builtin_constant_p which doesn't evaluate its argument? Maybe specifically check for it? Or at least this needs some more explanation.

[danielmarjamaki]

I have refactored my patch.

• I added more comments.
• now the handling of __builtin_constant_p is more explicit.
• I changed uppercase=>lowercase in two functionnames in accordance with the llvm style guide.

I have rerun the patch on various projects to make sure that the more explicit handling of builtins don't cause false positives.

[alexfh]

LG with a couple of comments.

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

93

nits:

• s/result from/result of/
• "If we have seen a __builtin_constant_p(x) and then there is a ..." - "If we see __builtin_constant_p(x) followed by a ..."
• add a comma before "then we need to reason about ..."
• s/bailout/bail out/

127

nits:

• dont -> don't
• argument -> arguments

[danielmarjamaki]

This was committed in r241245

clang-tidy/misc/MacroRepeatedSideEffectsCheck.cpp

public:
 
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override;
 
 private:
   ClangTidyCheck &Check;
   Preprocessor &PP;
 
-  unsigned CountArgumentExpansions(const MacroInfo *MI,
+  unsigned countArgumentExpansions(const MacroInfo *MI,
                                    const IdentifierInfo *Arg) const;
 
-  bool HasSideEffects(const Token *ResultArgToks) 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);
+            return T.isOneOf(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 (HasSideEffects(ResultArgToks) &&
-        CountArgumentExpansions(MI, Arg) >= 2) {
+    if (hasSideEffects(ResultArgToks) &&
+        countArgumentExpansions(MI, Arg) >= 2) {
       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(
+unsigned MacroRepeatedPPCallbacks::countArgumentExpansions(
     const MacroInfo *MI, const IdentifierInfo *Arg) const {
-  unsigned CountInMacro = 0;
+  // Current argument count. When moving forward to a different control-flow
+  // path this can decrease.
+  unsigned Current = 0;
+  // Max argument count.
+  unsigned Max = 0;
   bool SkipParen = false;
   int SkipParenCount = 0;
+  // Has a __builtin_constant_p been found?
+  bool FoundBuiltin = false;
+  // Count when "?" is reached. The "Current" will get this value when the ":"
+  // is reached.
+  std::stack<unsigned, SmallVector<unsigned, 8>> CountAtQuestion;
   for (const auto &T : MI->tokens()) {
+    // The result from __builtin_constant_p(x) is 0 if x is a macro argument
+    // with side effects. If we have seen a __builtin_constant_p(x) and then
+    // there is a "?" "&&" or "||" then we need to reason about control flow
+    // to report warnings correctly. Until such reasoning is added, bailout
+    // when this happens.
+    if (FoundBuiltin && T.isOneOf(tok::question, tok::ampamp, tok::pipepipe))
+      return Max;
+
+    // Handling of ? and :.
+    if (T.is(tok::question)) {
+      CountAtQuestion.push(Current);
+    } else if (T.is(tok::colon)) {
+      if (CountAtQuestion.empty())
+        return 0;
+      Current = CountAtQuestion.top();
+      CountAtQuestion.pop();
+    }
+
     // 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) {
+    // If a __builtin_constant_p is found within the macro definition, dont
+    // count argument inside the parentheses and remember that it has been seen
+    // in case there are "?", "&&" or "||" operators later.
+    if (TII->getBuiltinID() == Builtin::BI__builtin_constant_p) {
+      FoundBuiltin = true;
       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++;
+    if (TII == Arg) {
+      Current++;
+      if (Current > Max)
+        Max = Current;
+    }
   }
-  return CountInMacro;
+  return Max;
 }
 
-bool MacroRepeatedPPCallbacks::HasSideEffects(
+bool MacroRepeatedPPCallbacks::hasSideEffects(
     const Token *ResultArgToks) const {
   for (; ResultArgToks->isNot(tok::eof); ++ResultArgToks) {
     if (ResultArgToks->isOneOf(tok::plusplus, tok::minusminus))
       return true;
   }
   return false;
 }
 

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

void bad(int ret, int a, int 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'
 }
 
 
+#define MIN(A,B)     ((A) < (B) ? (A) : (B))                        // single ?:
+#define LIMIT(X,A,B) ((X) < (A) ? (A) : ((X) > (B) ? (B) : (X)))    // two ?:
+void question(int x) {
+  MIN(x++, 12);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: side effects in the 1st macro argument 'A'
+  MIN(34, x++);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: side effects in the 2nd macro argument 'B'
+  LIMIT(x++, 0, 100);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: side effects in the 1st macro argument 'X'
+  LIMIT(20, x++, 100);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: side effects in the 2nd macro argument 'A'
+  LIMIT(20, 0, x++);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: side effects in the 3rd macro argument 'B'
+}
+
 // 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'
 }
 
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))
+// Passing macro argument as argument to __builtin_constant_p and macros.
+#define builtinbad(x)      (__builtin_constant_p(x) + (x) + (x))
+#define builtingood1(x)    (__builtin_constant_p(x) + (x))
+#define builtingood2(x)    ((__builtin_constant_p(x) && (x)) || (x))
+#define macrobad(x)        (builtingood1(x) + (x) + (x))
+#define macrogood(x)       (builtingood1(x) + (x))
 void builtins(int ret, int a) {
-  ret += builtinA(a++);
-  ret += builtinB(a++);
+  ret += builtinbad(a++);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: side effects in the 1st macro argument 'x'
+
+  ret += builtingood1(a++);
+  ret += builtingood2(a++);
+
+  ret += macrobad(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'
+
+  ret += macrogood(a++);
 }
 
 // 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)
+void conditionals(int a, int b)
 {
-  ret += condA(a++, b++);
   condB(a, b++);
 }