[clang-tidy] new check: bugprone-too-small-loop-variable

The new checker searches for those for loops which has a loop variable with a "too small" type which means this type can't represent all values which are part of the iteration range.

For example:

```
int main() {
  long size = 300000;
  for( short int i = 0; i < size; ++i) {}
}
```

The short type leads to infinite loop here because it can't store all values in the `[0..size]` interval. In a real use case, size means a container's size which depends on the user input. Which means for small amount of objects the algorithm works, but with a larger user input the software will freeze.

The idea of the checker comes from the LibreOffice project, where the same check was implemented as a clang compiler plugin, called `LoopVarTooSmall` (LLVM licensed).
The idea is the same behind this check, but the code is different because of the different framework.

Patch by ztamas.

Reviewers: alexfh, hokein, aaron.ballman, JonasToth, xazax.hun, whisperity

Reviewed By: JonasToth, whisperity

Differential Revision: https://reviews.llvm.org/D53974

llvm-svn: 346665

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -44,6 +44,7 @@
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
+#include "TooSmallLoopVariableCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnusedRaiiCheck.h"
@@ -96,6 +97,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
+        "bugprone-too-small-loop-variable");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -35,6 +35,7 @@ add_clang_library(clangTidyBugproneModule
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
+  TooSmallLoopVariableCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnusedRaiiCheck.cpp

clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp

@@ -0,0 +1,168 @@
+//===--- TooSmallLoopVariableCheck.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 "TooSmallLoopVariableCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+static constexpr llvm::StringLiteral LoopName =
+    llvm::StringLiteral("forLoopName");
+static constexpr llvm::StringLiteral LoopVarName =
+    llvm::StringLiteral("loopVar");
+static constexpr llvm::StringLiteral LoopVarCastName =
+    llvm::StringLiteral("loopVarCast");
+static constexpr llvm::StringLiteral LoopUpperBoundName =
+    llvm::StringLiteral("loopUpperBound");
+static constexpr llvm::StringLiteral LoopIncrementName =
+    llvm::StringLiteral("loopIncrement");
+
+/// \brief The matcher for loops with suspicious integer loop variable.
+///
+/// In this general example, assuming 'j' and 'k' are of integral type:
+/// \code
+///   for (...; j < 3 + 2; ++k) { ... }
+/// \endcode
+/// The following string identifiers are bound to these parts of the AST:
+///   LoopVarName: 'j' (as a VarDecl)
+///   LoopVarCastName: 'j' (after implicit conversion)
+///   LoopUpperBoundName: '3 + 2' (as an Expr)
+///   LoopIncrementName: 'k' (as an Expr)
+///   LoopName: The entire for loop (as a ForStmt)
+///
+void TooSmallLoopVariableCheck::registerMatchers(MatchFinder *Finder) {
+  StatementMatcher LoopVarMatcher =
+      expr(
+          ignoringParenImpCasts(declRefExpr(to(varDecl(hasType(isInteger()))))))
+          .bind(LoopVarName);
+
+  // We need to catch only those comparisons which contain any integer cast.
+  StatementMatcher LoopVarConversionMatcher =
+      implicitCastExpr(hasImplicitDestinationType(isInteger()),
+                       has(ignoringParenImpCasts(LoopVarMatcher)))
+          .bind(LoopVarCastName);
+
+  // We are interested in only those cases when the loop bound is a variable
+  // value (not const, enum, etc.).
+  StatementMatcher LoopBoundMatcher =
+      expr(ignoringParenImpCasts(allOf(hasType(isInteger()),
+                                       unless(integerLiteral()),
+                                       unless(hasType(isConstQualified())),
+                                       unless(hasType(enumType())))))
+          .bind(LoopUpperBoundName);
+
+  // We use the loop increment expression only to make sure we found the right
+  // loop variable.
+  StatementMatcher IncrementMatcher =
+      expr(ignoringParenImpCasts(hasType(isInteger()))).bind(LoopIncrementName);
+
+  Finder->addMatcher(
+      forStmt(
+          hasCondition(anyOf(
+              binaryOperator(hasOperatorName("<"),
+                             hasLHS(LoopVarConversionMatcher),
+                             hasRHS(LoopBoundMatcher)),
+              binaryOperator(hasOperatorName("<="),
+                             hasLHS(LoopVarConversionMatcher),
+                             hasRHS(LoopBoundMatcher)),
+              binaryOperator(hasOperatorName(">"), hasLHS(LoopBoundMatcher),
+                             hasRHS(LoopVarConversionMatcher)),
+              binaryOperator(hasOperatorName(">="), hasLHS(LoopBoundMatcher),
+                             hasRHS(LoopVarConversionMatcher)))),
+          hasIncrement(IncrementMatcher))
+          .bind(LoopName),
+      this);
+}
+
+/// Returns the positive part of the integer width for an integer type.
+static unsigned calcPositiveBits(const ASTContext &Context,
+                                 const QualType &IntExprType) {
+  assert(IntExprType->isIntegerType());
+
+  return IntExprType->isUnsignedIntegerType()
+             ? Context.getIntWidth(IntExprType)
+             : Context.getIntWidth(IntExprType) - 1;
+}
+
+/// \brief Calculate the upper bound expression's positive bits, but ignore
+/// constant like values to reduce false positives.
+static unsigned calcUpperBoundPositiveBits(const ASTContext &Context,
+                                           const Expr *UpperBound,
+                                           const QualType &UpperBoundType) {
+  // Ignore casting caused by constant values inside a binary operator.
+  // We are interested in variable values' positive bits.
+  if (const auto *BinOperator = dyn_cast<BinaryOperator>(UpperBound)) {
+    const Expr *RHSE = BinOperator->getRHS()->IgnoreParenImpCasts();
+    const Expr *LHSE = BinOperator->getLHS()->IgnoreParenImpCasts();
+
+    QualType RHSEType = RHSE->getType();
+    QualType LHSEType = LHSE->getType();
+
+    if (!RHSEType->isIntegerType() || !LHSEType->isIntegerType())
+      return 0;
+
+    bool RHSEIsConstantValue = RHSEType->isEnumeralType() ||
+                               RHSEType.isConstQualified() ||
+                               isa<IntegerLiteral>(RHSE);
+    bool LHSEIsConstantValue = LHSEType->isEnumeralType() ||
+                               LHSEType.isConstQualified() ||
+                               isa<IntegerLiteral>(LHSE);
+
+    // Avoid false positives produced by two constant values.
+    if (RHSEIsConstantValue && LHSEIsConstantValue)
+      return 0;
+    if (RHSEIsConstantValue)
+      return calcPositiveBits(Context, LHSEType);
+    if (LHSEIsConstantValue)
+      return calcPositiveBits(Context, RHSEType);
+
+    return std::max(calcPositiveBits(Context, LHSEType),
+                    calcPositiveBits(Context, RHSEType));
+  }
+
+  return calcPositiveBits(Context, UpperBoundType);
+}
+
+void TooSmallLoopVariableCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *LoopVar = Result.Nodes.getNodeAs<Expr>(LoopVarName);
+  const auto *UpperBound =
+      Result.Nodes.getNodeAs<Expr>(LoopUpperBoundName)->IgnoreParenImpCasts();
+  const auto *LoopIncrement =
+      Result.Nodes.getNodeAs<Expr>(LoopIncrementName)->IgnoreParenImpCasts();
+
+  // We matched the loop variable incorrectly.
+  if (LoopVar->getType() != LoopIncrement->getType())
+    return;
+
+  QualType LoopVarType = LoopVar->getType();
+  QualType UpperBoundType = UpperBound->getType();
+
+  ASTContext &Context = *Result.Context;
+
+  unsigned LoopVarPosBits = calcPositiveBits(Context, LoopVarType);
+  unsigned UpperBoundPosBits =
+      calcUpperBoundPositiveBits(Context, UpperBound, UpperBoundType);
+
+  if (UpperBoundPosBits == 0)
+    return;
+
+  if (LoopVarPosBits < UpperBoundPosBits)
+    diag(LoopVar->getBeginLoc(), "loop variable has narrower type %0 than "
+                                 "iteration's upper bound %1")
+        << LoopVarType << UpperBoundType;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.h

@@ -0,0 +1,43 @@
+//===--- TooSmallLoopVariableCheck.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_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// This check gives a warning if a loop variable has a too small type which
+/// might not be able to represent all values which are part of the whole range
+/// in which the loop iterates.
+/// If the loop variable's type is too small we might end up in an infinite
+/// loop. Example:
+/// \code
+///   long size = 294967296l;
+///   for (short i = 0; i < size; ++i) {} { ... }
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-too-small-loop-variable.html
+class TooSmallLoopVariableCheck : public ClangTidyCheck {
+public:
+  TooSmallLoopVariableCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TOOSMALLLOOPVARIABLECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -110,6 +110,13 @@ Improvements to clang-tidy
   Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests
   ``absl::StrAppend()`` should be used instead.
 
+- New :doc:`bugprone-too-small-loop-variable
+  <clang-tidy/checks/bugprone-too-small-loop-variable>` check.
+
+  Detects those ``for`` loops that have a loop variable with a "too small" type
+  which means this type can't represent all values which are part of the
+  iteration range.
+
 - New :doc:`cppcoreguidelines-macro-usage
   <clang-tidy/checks/cppcoreguidelines-macro-usage>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst

@@ -0,0 +1,29 @@
+.. title:: clang-tidy - bugprone-too-small-loop-variable
+
+bugprone-too-small-loop-variable
+================================
+
+Detects those ``for`` loops that have a loop variable with a "too small" type
+which means this type can't represent all values which are part of the
+iteration range.
+
+.. code-block:: c++
+
+  int main() {
+    long size = 294967296l;
+    for (short i = 0; i < size; ++i) {}
+  }
+
+This ``for`` loop is an infinite loop because the ``short`` type can't represent
+all values in the ``[0..size]`` interval.
+
+In a real use case size means a container's size which depends on the user input.
+
+.. code-block:: c++
+
+  int doSomething(const std::vector& items) {
+    for (short i = 0; i < items.size(); ++i) {}
+  }
+
+This algorithm works for small amount of objects, but will lead to freeze for a
+a larger user input.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -59,6 +59,7 @@ Clang-Tidy Checks
    bugprone-swapped-arguments
    bugprone-terminating-continue
    bugprone-throw-keyword-missing
+   bugprone-too-small-loop-variable
    bugprone-undefined-memory-manipulation
    bugprone-undelegated-constructor
    bugprone-unused-raii