[clang-tidy] Add check cppcoreguidelines-pro-bounds-pointer-arithmetic

[mgehre]

Summary

This check flags all usage of pointer arithmetic, because it could lead to an
invalid pointer.
Subtraction of two pointers is not flagged by this check.

Pointers should only refer to single objects, and pointer arithmetic is
fragile and easy to get wrong. array_view is a bounds-checked, safe type
for accessing arrays of data.

This rule is part of the "Bounds safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-bounds1-dont-use-pointer-arithmetic-use-array_view-instead

Depends on D13313

[mgehre]

Simplified matcher

[mgehre]

Port to cppcoreguidelines module.
Add checks for post/pre increment/decrement and array access.

[mgehre]

Remove inclusion of misc-no-reinterpret-cast in this patch set

[sbenza]

clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp

38 ↗

(On Diff #36305)

What is that implicitCastExpr() matching?
Did you mean to use ignoringImpCasts() instead?

test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp

17 ↗

(On Diff #36305)

You don't need to specify the whole message on each hit.
Use the first hit to verify the whole message.
Strip the tail of the rest to let them fit in a single line.

24 ↗

(On Diff #36305)

Why just literals/enums? Maybe one of these should use i/j to make sure variables also work.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp

48 ↗

(On Diff #36305)

Can elide the parenthetical.

clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.h

18 ↗

(On Diff #36305)

This could be a bit more usefully descriptive. ;-)

test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp

36 ↗

(On Diff #36305)

How does this check handle code like:

struct S {
  operator int() const;  
};

void f(S &s) {
  int *i;
  i = i + s;
}

38 ↗

(On Diff #36305)

s/okey/okay

;-)

[mgehre]

Incorporated comments
Simplied matcher: Instead of checking argument types, just check that the result of the arithmetic operation is a pointer

[mgehre]

Thank you for the review comments!

clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp

38 ↗

(On Diff #36305)

The AST for

int* p = 0;
int i;
i = p[1];

is

-BinaryOperator 0x22dd770 <line:52:3, col:10> 'int' lvalue '='
   |-DeclRefExpr 0x22dd6a8 <col:3> 'int' lvalue Var 0x227fca0 'i' 'int'
   `-ImplicitCastExpr 0x22dd758 <col:7, col:10> 'int' <LValueToRValue>
     `-ArraySubscriptExpr 0x22dd730 <col:7, col:10> 'int' lvalue
       |-ImplicitCastExpr 0x22dd718 <col:7> 'int *' <LValueToRValue>
       | `-DeclRefExpr 0x22dd6d0 <col:7> 'int *' lvalue Var 0x227fdf0 'p' 'int *'
       `-IntegerLiteral 0x22dd6f8 <col:9> 'int' 1

I guess you are right about ignoringImpCasts(), though.

[mgehre]

Fix typo

[sbenza]

clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp

35 ↗

(On Diff #36404)

Space after //

test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp

68 ↗

(On Diff #36404)

Space after //

[mgehre]

Fixed space after //

[aaron.ballman]

test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp

37 ↗

(On Diff #36564)

I don't think this comment is "done" yet. I still don't know how this check is intended to handle code like that. Does it currently diagnose? Does it not diagnose? Should it diagnose?

51 ↗

(On Diff #36564)

How well does this handle code like:

void f(int i[], size_t s) {
  i[s - 1] = 0;
}

Does it diagnose, and should it?

[mgehre]

test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp

37 ↗

(On Diff #36564)

I had added your code, see line 55 of this file.
It should diagnose, because an arithmetic operation (+) is used, which results in a (possibly) changed pointer.
It does diagnose, as the CHECK-MESSAGE shows.

51 ↗

(On Diff #36564)

Good point, I'll ask at https://github.com/isocpp/CppCoreGuidelines/issues/299

[aaron.ballman]

On Tue, Oct 6, 2015 at 5:31 PM, Matthias Gehre <[e-mail]> wrote:

mgehre marked 2 inline comments as done.

Comment at: test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp:37
@@ +36,3 @@
+ q -= i;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic

+ q -= ENUM_LITERAL;

aaron.ballman wrote:

I don't think this comment is "done" yet. I still don't know how this check is intended to handle code like that. Does it currently diagnose? Does it not diagnose? Should it diagnose?

I had added your code, see line 55 of this file.

I am pretty sure I missed this by accidentally fiddling with a Phab knob, because this is definitely in there this morning, but wasn't last night. Sorry for the noise!

It should diagnose, because an arithmetic operation (+) is used, which results in a (possibly) changed pointer.
It does diagnose, as the CHECK-MESSAGE shows.

Great, thank you!

Comment at: test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp:51
@@ +50,3 @@
+
+ i = p[1];

+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use pointer arithmetic

aaron.ballman wrote:

How well does this handle code like:

void f(int i[], size_t s) {
  i[s - 1] = 0;
}

Does it diagnose, and should it?

Good point, I'll ask at https://github.com/isocpp/CppCoreGuidelines/issues/299

Thanks! The answer to this question should not hold up this patch, IMO.

LGTM!

~Aaron

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
+  ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
+        "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
     CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");
   }
 };

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.h

+//===--- ProBoundsPointerArithmeticCheck.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_CPPCOREGUIDELINES_PRO_BOUNDS_POINTER_ARITHMETIC_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_POINTER_ARITHMETIC_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Flags all kinds of pointer arithmetic that have result of pointer type, i.e.
+/// +, -, +=, -=, ++, --. In addition, the [] operator on pointers (not on arrays) is flagged.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-pro-bounds-pointer-arithmetic.html
+class ProBoundsPointerArithmeticCheck : public ClangTidyCheck {
+public:
+  ProBoundsPointerArithmeticCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_BOUNDS_POINTER_ARITHMETIC_H

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp

+//===--- ProBoundsPointerArithmeticCheck.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 "ProBoundsPointerArithmeticCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void ProBoundsPointerArithmeticCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // Flag all operators +, -, +=, -=, ++, -- that result in a pointer
+  Finder->addMatcher(
+      binaryOperator(anyOf(hasOperatorName("+"), hasOperatorName("-"),
+                           hasOperatorName("+="), hasOperatorName("-=")),
+                     hasType(pointerType()))
+          .bind("expr"),
+      this);
+
+  Finder->addMatcher(
+      unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
+                    hasType(pointerType()))
+          .bind("expr"),
+      this);
+
+  // Array subscript on a pointer (not an array) is also pointer arithmetic
+  Finder->addMatcher(
+      arraySubscriptExpr(hasBase(ignoringImpCasts(anyOf(hasType(pointerType()),
+                                                        hasType(decayedType(hasDecayedType(pointerType())))))))
+          .bind("expr"),
+      this);
+}
+
+void
+ProBoundsPointerArithmeticCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<Expr>("expr");
+
+  diag(MatchedExpr->getExprLoc(), "do not use pointer arithmetic");
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-pointer-arithmetic.rst

+cppcoreguidelines-pro-bounds-pointer-arithmetic
+===============================================
+
+This check flags all usage of pointer arithmetic, because it could lead to an invalid pointer.
+Subtraction of two pointers is not flagged by this check.
+
+Pointers should only refer to single objects, and pointer arithmetic is fragile and easy to get wrong. array_view is a bounds-checked, safe type for accessing arrays of data.
+
+This rule is part of the "Bounds safety" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-bounds1-dont-use-pointer-arithmetic-use-array_view-instead

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

 List of clang-tidy Checks
 =========================
 
 .. toctree::
    cert-setlongjmp
    cert-variadic-function-def
+   cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-explicit-constructor
    google-global-names-in-headers

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-pointer-arithmetic.cpp

+// RUN: %python %S/check_clang_tidy.py %s cppcoreguidelines-pro-bounds-pointer-arithmetic %t
+
+enum E {
+  ENUM_LITERAL = 1
+};
+
+int i = 4;
+int j = 1;
+int *p = 0;
+int *q = 0;
+
+void fail() {
+  q = p + 4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
+  p = q + i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic
+  p = q + ENUM_LITERAL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic
+
+  q = p - 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic
+  p = q - i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic
+  p = q - ENUM_LITERAL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic
+
+  p += 4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic
+  p += i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic
+  p += ENUM_LITERAL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic
+
+  q -= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic
+  q -= i;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic
+  q -= ENUM_LITERAL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic
+
+  p++;
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: do not use pointer arithmetic
+  ++p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use pointer arithmetic
+
+  p--;
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: do not use pointer arithmetic
+  --p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use pointer arithmetic
+
+  i = p[1];
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use pointer arithmetic
+}
+
+struct S {
+  operator int() const;
+};
+
+void f(S &s) {
+  int *i;
+  i = i + s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic
+}
+
+void f2(int i[]) {
+  i[1] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use pointer arithmetic
+}
+
+void okay() {
+  int a[3];
+  i = a[2]; // OK, access to array
+
+  p = q;
+  p = &i;
+
+  i++;
+  ++i;
+  i--;
+  --i;
+  i += 1;
+  i -= 1;
+  i = j + 1;
+  i = j - 1;
+
+  auto diff = p - q; // OK, result is arithmetic
+}