[clang-tidy] Improving bugprone-sizeof-expr check.

[balazske]

Summary

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.

[aaron.ballman]

LGTM aside from a testing request.

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

315

I think this is missing some test coverage for cases like:

sum += sizeof(PtrArray) / sizeof(PtrArray[1]); // Bad array index
sum += sizeof(PtrArray) / sizeof(A[0]); // Different array types

Also, if you can add a comment to the sum += sizeof(PtrArray) / sizeof(PtrArray1[0]); test case about why this passes (same canonical types), that'd be appreciated since the test case is a bit subtle.

[balazske]

Improving tests.

[balazske]

This checker has multiple weaknesses. There are more cases when the warnings should not appear (probably if the argument of sizeof is a template parameter), or more than one warning is generated for a code construct. The test code could be further improved too (but there are many cases to handle). This fix addresses only a single problematic case.

[aaron.ballman]

In D91543#2400051, @balazske wrote:

This checker has multiple weaknesses. There are more cases when the warnings should not appear (probably if the argument of sizeof is a template parameter), or more than one warning is generated for a code construct. The test code could be further improved too (but there are many cases to handle). This fix addresses only a single problematic case.

Sorry about being unclear -- I was intending to ensure we had test coverage for the changes in the patch. Specifically, the ZeroLiteral and ArrayOfSamePointersExpr matchers did not appear to have negative tests showing that we aren't diagnosing that code. I don't expect this patch to actually handle those cases (or for you to be on the hook for the fixes), it's more about documenting expectations.

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

317

Thanks for the new test case, can you also add one for array[1] (with the incorrect index) and add a FIXME to the comment so it's clear that we know we don't handle these cases currently but might like to someday?

[balazske]

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

317

The test for array[1] is not the same that is inserted on line 245?

[aaron.ballman]

LGTM!

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

317

Weirdly, I didn't see the test cases added around line 245 until this review. Huh, very odd -- but yes, that test covers it nicely.

[balazske]

Adding FIXME comments.

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

void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfIntegerExpression",
                 WarnOnSizeOfIntegerExpression);
   Options.store(Opts, "WarnOnSizeOfThis", WarnOnSizeOfThis);
   Options.store(Opts, "WarnOnSizeOfCompareToConstant",
                 WarnOnSizeOfCompareToConstant);
 }
 
 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)),
             binaryOperator(hasLHS(IntegerExpr), hasRHS(IntegerExpr)))));
   const auto IntegerCallExpr = expr(ignoringParenImpCasts(
       callExpr(anyOf(hasType(isInteger()), hasType(enumType())),
                unless(isInTemplateInstantiation()))));
   const auto SizeOfExpr = expr(anyOf(
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(expr(sizeOfExpr(has(ignoringParenImpCasts(expr(
                               hasType(qualType(hasCanonicalType(CharPtrType))),
                               ignoringParenImpCasts(declRefExpr(
                                   hasDeclaration(ConstStrLiteralDecl))))))))
                          .bind("sizeof-charp"),
                      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(
       unaryOperator(hasUnaryOperand(ArrayExpr), unless(hasOperatorName("*"))),
       binaryOperator(hasEitherOperand(ArrayExpr)),
       castExpr(hasSourceExpression(ArrayExpr))));
   const auto PointerToArrayExpr = expr(ignoringParenImpCasts(expr(
       hasType(qualType(hasCanonicalType(pointerType(pointee(arrayType()))))))));
 
   const auto StructAddrOfExpr =
       unaryOperator(hasOperatorName("&"),
                     hasUnaryOperand(ignoringParenImpCasts(expr(
                         hasType(qualType(hasCanonicalType(recordType())))))));
   const auto PointerToStructType = type(hasUnqualifiedDesugaredType(
       pointerType(pointee(recordType()))));
   const auto PointerToStructExpr = expr(ignoringParenImpCasts(expr(
       hasType(qualType(hasCanonicalType(PointerToStructType))),
       unless(cxxThisExpr()))));
 
-  Finder->addMatcher(
-      expr(anyOf(sizeOfExpr(has(expr(ignoringParenImpCasts(
-               anyOf(ArrayCastExpr, PointerToArrayExpr, StructAddrOfExpr,
-                     PointerToStructExpr))))),
-                            sizeOfExpr(has(PointerToStructType))))
+  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) {
     Finder->addMatcher(
         binaryOperator(matchers::isRelationalOperator(),
                        hasOperands(ignoringParenImpCasts(SizeOfExpr),
                                    ignoringParenImpCasts(anyOf(
                                        integerLiteral(equals(0)),
                                        integerLiteral(isBiggerThan(0x80000))))))
             .bind("sizeof-compare-constant"),
         this);
   }
 
   // Detect expression like: sizeof(expr, expr); most likely an error.
   Finder->addMatcher(expr(sizeOfExpr(has(expr(ignoringParenImpCasts(
                               binaryOperator(hasOperatorName(",")))))))
                          .bind("sizeof-comma-expr"),
                      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")));
   const auto NumType = qualType(hasCanonicalType(
       type(anyOf(ElemType, ElemPtrType, type())).bind("num-type")));
   const auto DenomType = qualType(hasCanonicalType(type().bind("denom-type")));
 
   Finder->addMatcher(

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

int Test4(const char A[10]) {
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(char*)'
   sum += sizeof(kDEF);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(char*)'
   return sum;
 }
 
 int Test5() {
   typedef int Array10[10];
+  typedef C ArrayC[10];
 
   struct MyStruct {
     Array10 arr;
     Array10* ptr;
   };
   typedef const MyStruct TMyStruct;
   typedef const MyStruct *PMyStruct;
   typedef TMyStruct *PMyStruct2;
 
   static TMyStruct kGlocalMyStruct = {};
   static TMyStruct volatile * kGlocalMyStructPtr = &kGlocalMyStruct;
 
   MyStruct S;
   PMyStruct PS;
   PMyStruct2 PS2;
   Array10 A10;
+  C *PtrArray[10];
+  C *PC;
 
   int sum = 0;
   sum += sizeof(&S.arr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&kGlocalMyStruct.arr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(&kGlocalMyStructPtr->arr);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
int Test5() {
   sum += sizeof(PMyStruct);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
   // 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;
 }
 
 int Test6() {
   int sum = 0;
 
   struct S A = AsStruct(), B = AsStruct();
int Test6() {
 
   return sum;
 }
 
 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);
   sum += sizeof(int);
   sum += sizeof(AsStruct());
   sum += sizeof(M{}.AsStruct());
   sum += sizeof(A[sizeof(A) / sizeof(int)]);
   sum += sizeof(&A[sizeof(A) / sizeof(int)]);
   sum += sizeof(sizeof(0));  // Special case: sizeof size_t.
   sum += sizeof(void*);
   sum += sizeof(void const *);
   sum += sizeof(void const *) / 4;
   sum += sizeof(str);
   sum += sizeof(str) / sizeof(char);
   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;
 }