[clang-tidy] Improving bugprone-sizeof-expr check.
authorBalázs Kéri <1.int32@gmail.com>
Thu, 19 Nov 2020 08:03:22 +0000 (09:03 +0100)
committerBalázs Kéri <1.int32@gmail.com>
Thu, 19 Nov 2020 09:26:33 +0000 (10:26 +0100)
Do not warn for "pointer to aggregate" in a `sizeof(A) / sizeof(A[0])`
expression if `A` is an array of pointers. This is the usual way of
calculating the array length even if the array is of pointers.

Reviewed By: aaron.ballman

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

clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-sizeof-expression.cpp

index 39526da..5790e8f 100644 (file)
@@ -77,6 +77,10 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 }
 
 void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME:
+  // Some of the checks should not match in template code to avoid false
+  // positives if sizeof is applied on template argument.
+
   const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
   const auto ConstantExpr = expr(ignoringParenImpCasts(
       anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
@@ -132,6 +136,7 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                      this);
 
   // Detect sizeof(ptr) where ptr points to an aggregate (i.e. sizeof(&S)).
+  // Do not find it if RHS of a 'sizeof(arr) / sizeof(arr[0])' expression.
   const auto ArrayExpr = expr(ignoringParenImpCasts(
       expr(hasType(qualType(hasCanonicalType(arrayType()))))));
   const auto ArrayCastExpr = expr(anyOf(
@@ -151,13 +156,31 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
       hasType(qualType(hasCanonicalType(PointerToStructType))),
       unless(cxxThisExpr()))));
 
-  Finder->addMatcher(
-      expr(anyOf(sizeOfExpr(has(expr(ignoringParenImpCasts(
-               anyOf(ArrayCastExpr, PointerToArrayExpr, StructAddrOfExpr,
-                     PointerToStructExpr))))),
-                            sizeOfExpr(has(PointerToStructType))))
-          .bind("sizeof-pointer-to-aggregate"),
-      this);
+  const auto ArrayOfPointersExpr = expr(ignoringParenImpCasts(expr(hasType(
+      qualType(hasCanonicalType(arrayType(hasElementType(pointerType()))
+                                    .bind("type-of-array-of-pointers")))))));
+  const auto ArrayOfSamePointersExpr =
+      expr(ignoringParenImpCasts(expr(hasType(qualType(hasCanonicalType(
+          arrayType(equalsBoundNode("type-of-array-of-pointers"))))))));
+  const auto ZeroLiteral =
+      expr(ignoringParenImpCasts(integerLiteral(equals(0))));
+  const auto ArrayOfSamePointersZeroSubscriptExpr =
+      expr(ignoringParenImpCasts(arraySubscriptExpr(
+          hasBase(ArrayOfSamePointersExpr), hasIndex(ZeroLiteral))));
+  const auto ArrayLengthExprDenom =
+      expr(hasParent(expr(ignoringParenImpCasts(
+               binaryOperator(hasOperatorName("/"),
+                              hasLHS(expr(ignoringParenImpCasts(expr(
+                                  sizeOfExpr(has(ArrayOfPointersExpr)))))))))),
+           sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
+
+  Finder->addMatcher(expr(anyOf(sizeOfExpr(has(expr(ignoringParenImpCasts(anyOf(
+                                    ArrayCastExpr, PointerToArrayExpr,
+                                    StructAddrOfExpr, PointerToStructExpr))))),
+                                sizeOfExpr(has(PointerToStructType))),
+                          unless(ArrayLengthExprDenom))
+                         .bind("sizeof-pointer-to-aggregate"),
+                     this);
 
   // Detect expression like: sizeof(epxr) <= k for a suspicious constant 'k'.
   if (WarnOnSizeOfCompareToConstant) {
@@ -178,6 +201,12 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                      this);
 
   // Detect sizeof(...) /sizeof(...));
+  // FIXME:
+  // Re-evaluate what cases to handle by the checker.
+  // Probably any sizeof(A)/sizeof(B) should be error if
+  // 'A' is not an array (type) and 'B' the (type of the) first element of it.
+  // Except if 'A' and 'B' are non-pointers, then use the existing size division
+  // rule.
   const auto ElemType =
       arrayType(hasElementType(recordType().bind("elem-type")));
   const auto ElemPtrType = pointerType(pointee(type().bind("elem-ptr-type")));
index b82becc..d0e6057 100644 (file)
@@ -187,6 +187,7 @@ int Test4(const char A[10]) {
 
 int Test5() {
   typedef int Array10[10];
+  typedef C ArrayC[10];
 
   struct MyStruct {
     Array10 arr;
@@ -203,6 +204,8 @@ int Test5() {
   PMyStruct PS;
   PMyStruct2 PS2;
   Array10 A10;
+  C *PtrArray[10];
+  C *PC;
 
   int sum = 0;
   sum += sizeof(&S.arr);
@@ -239,6 +242,17 @@ int Test5() {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&A10);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(A10) / sizeof(PtrArray[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PC) / sizeof(PtrArray[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+  // CHECK-MESSAGES: :[[@LINE-3]]:23: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+  // CHECK-MESSAGES: :[[@LINE-2]]:27: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
 
   return sum;
 }
@@ -276,6 +290,10 @@ int ValidExpressions() {
   int A[] = {1, 2, 3, 4};
   static const char str[] = "hello";
   static const char* ptr[] { "aaa", "bbb", "ccc" };
+  typedef C *CA10[10];
+  C *PtrArray[10];
+  CA10 PtrArray1;
+
   int sum = 0;
   if (sizeof(A) < 10)
     sum += sizeof(A);
@@ -293,5 +311,10 @@ int ValidExpressions() {
   sum += sizeof(str) / sizeof(str[0]);
   sum += sizeof(ptr) / sizeof(ptr[0]);
   sum += sizeof(ptr) / sizeof(*(ptr));
+  sum += sizeof(PtrArray) / sizeof(PtrArray[0]);
+  // Canonical type of PtrArray1 is same as PtrArray.
+  sum = sizeof(PtrArray) / sizeof(PtrArray1[0]);
+  // There is no warning for 'sizeof(T*)/sizeof(Q)' case.
+  sum += sizeof(PtrArray) / sizeof(A[0]);
   return sum;
 }