[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-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
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
+  PointerArithmeticCheck.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/misc/MiscTidyModule.cpp

 #include "InaccurateEraseCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
+#include "PointerArithmeticCheck.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
+    CheckFactories.registerCheck<PointerArithmeticCheck>(
+        "misc-pointer-arithmetic");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(

clang-tidy/misc/PointerArithmeticCheck.h

+//===--- PointerArithmeticCheck.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_POINTER_ARITHMETIC_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_POINTER_ARITHMETIC_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Flags all kinds of pointer arithmetic
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-pointer-arithmetic.html
+class PointerArithmeticCheck : public ClangTidyCheck {
+public:
+  PointerArithmeticCheck(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_MISC_POINTER_ARITHMETIC_H
+

clang-tidy/misc/PointerArithmeticCheck.cpp

+//===--- PointerArithmeticCheck.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 "PointerArithmeticCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void PointerArithmeticCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  
+  // According to C++ standard, if one operand of '+' is a pointer, the other one must be an integer
+  Finder->addMatcher(binaryOperator(hasOperatorName("+"), 
+                                    hasEitherOperand(hasType(pointerType()))
+                                   ).bind("binOp"), this);
+  
+  // We exclude the case where one subtracts two pointers, because that cannot yield an invalid pointer
+  //FIXME: Use anyOf
+  Finder->addMatcher(binaryOperator(hasOperatorName("-"),
+                                    hasEitherOperand(hasType(pointerType())),
+                                    hasEitherOperand(hasType(isInteger()))
+                                   ).bind("binOp"), this);
+  
+  Finder->addMatcher(binaryOperator(hasOperatorName("+="),
+                                    hasEitherOperand(hasType(pointerType())),
+                                    hasEitherOperand(hasType(isInteger()))
+                                   ).bind("binOp"), this);
+    
+  Finder->addMatcher(binaryOperator(hasOperatorName("-="),
+                                    hasEitherOperand(hasType(pointerType())),
+                                    hasEitherOperand(hasType(isInteger()))
+                                   ).bind("binOp"), this);
+}
+
+void PointerArithmeticCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedOp = Result.Nodes.getNodeAs<BinaryOperator>("binOp");
+
+  diag(MatchedOp->getExprLoc(), "do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1)");
+}
+
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-inaccurate-erase
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
+   misc-pointer-arithmetic
    misc-sizeof-container
    misc-static-assert
    misc-swapped-arguments
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii

docs/clang-tidy/checks/misc-pointer-arithmetic.rst

+misc-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
 No newline at end of file

test/clang-tidy/misc-pointer-arithmetic.cpp

+// RUN: %python %S/check_clang_tidy.py %s misc-pointer-arithmetic %t
+
+
+enum E {
+  ENUM_LITERAL = 1
+};
+
+void f() {
+  int i = 4;
+  int j = i + 1; //OK, integral arithmetic
+  int* p = 0;
+  int* q = p + 4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  p += 4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  q -= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  q = p - 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  i += p;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  p = q - ENUM_LITERAL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  p = q + ENUM_LITERAL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not use pointer arithmetic (C++ Core Guidelines, rule Bounds.1) [misc-pointer-arithmetic]
+  
+  int a = p - q; //OK, result is arithmetic
+}
 No newline at end of file