[clang-tidy] Add option WarnOnSizeOfPointerToAggregate.

[mbenfield]

Summary

This is now an option under the check bugprone-sizeof-expression.

[mizvekov]

This is OK, within existing practice, but it's unfortunate that we have no tests at all for any of these flags.

[mbenfield]

I'll add a couple simple tests.

[mizvekov]

In D134381#3806929, @mbenfield wrote:

I'll add a couple simple tests.

Thanks!!

[mbenfield]

Add a test.

[mizvekov]

Thank you again.

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

public:
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   const bool WarnOnSizeOfConstant;
   const bool WarnOnSizeOfIntegerExpression;
   const bool WarnOnSizeOfThis;
   const bool WarnOnSizeOfCompareToConstant;
+  const bool WarnOnSizeOfPointerToAggregate;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIZEOFEXPRESSIONCHECK_H

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

 SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnSizeOfConstant(Options.get("WarnOnSizeOfConstant", true)),
       WarnOnSizeOfIntegerExpression(
           Options.get("WarnOnSizeOfIntegerExpression", false)),
       WarnOnSizeOfThis(Options.get("WarnOnSizeOfThis", true)),
       WarnOnSizeOfCompareToConstant(
-          Options.get("WarnOnSizeOfCompareToConstant", true)) {}
+          Options.get("WarnOnSizeOfCompareToConstant", true)),
+      WarnOnSizeOfPointerToAggregate(
+          Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
 
 void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
   Options.store(Opts, "WarnOnSizeOfIntegerExpression",
                 WarnOnSizeOfIntegerExpression);
   Options.store(Opts, "WarnOnSizeOfThis", WarnOnSizeOfThis);
   Options.store(Opts, "WarnOnSizeOfCompareToConstant",
                 WarnOnSizeOfCompareToConstant);
+  Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
+                WarnOnSizeOfPointerToAggregate);
 }
 
 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());
Finder->addMatcher(
                      expr(hasType(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.
+  if (WarnOnSizeOfPointerToAggregate) {
     const auto ArrayExpr =
         ignoringParenImpCasts(hasType(hasCanonicalType(arrayType())));
     const auto ArrayCastExpr = expr(anyOf(
         unaryOperator(hasUnaryOperand(ArrayExpr), unless(hasOperatorName("*"))),
         binaryOperator(hasEitherOperand(ArrayExpr)),
         castExpr(hasSourceExpression(ArrayExpr))));
     const auto PointerToArrayExpr = ignoringParenImpCasts(
         hasType(hasCanonicalType(pointerType(pointee(arrayType())))));
 
     const auto StructAddrOfExpr = unaryOperator(
         hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
                                   hasType(hasCanonicalType(recordType())))));
     const auto PointerToStructType =
         hasUnqualifiedDesugaredType(pointerType(pointee(recordType())));
     const auto PointerToStructExpr = ignoringParenImpCasts(expr(
         hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr())));
 
     const auto ArrayOfPointersExpr = ignoringParenImpCasts(
         hasType(hasCanonicalType(arrayType(hasElementType(pointerType()))
                                      .bind("type-of-array-of-pointers"))));
     const auto ArrayOfSamePointersExpr =
         ignoringParenImpCasts(hasType(hasCanonicalType(
             arrayType(equalsBoundNode("type-of-array-of-pointers")))));
     const auto ZeroLiteral = ignoringParenImpCasts(integerLiteral(equals(0)));
     const auto ArrayOfSamePointersZeroSubscriptExpr =
-      ignoringParenImpCasts(arraySubscriptExpr(hasBase(ArrayOfSamePointersExpr),
-                                               hasIndex(ZeroLiteral)));
+        ignoringParenImpCasts(arraySubscriptExpr(
+            hasBase(ArrayOfSamePointersExpr), hasIndex(ZeroLiteral)));
     const auto ArrayLengthExprDenom =
         expr(hasParent(expr(ignoringParenImpCasts(binaryOperator(
                  hasOperatorName("/"), hasLHS(ignoringParenImpCasts(sizeOfExpr(
                                            has(ArrayOfPointersExpr)))))))),
              sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
 
     Finder->addMatcher(expr(anyOf(sizeOfExpr(has(ignoringParenImpCasts(anyOf(
                                       ArrayCastExpr, PointerToArrayExpr,
                                       StructAddrOfExpr, PointerToStructExpr)))),
                                   sizeOfExpr(has(PointerToStructType))),
                             unless(ArrayLengthExprDenom))
                            .bind("sizeof-pointer-to-aggregate"),
                        this);
+  }
 
   // Detect expression like: sizeof(expr) <= k for a suspicious constant 'k'.
   if (WarnOnSizeOfCompareToConstant) {
     Finder->addMatcher(
         binaryOperator(matchers::isRelationalOperator(),
                        hasOperands(ignoringParenImpCasts(SizeOfExpr),
                                    ignoringParenImpCasts(integerLiteral(anyOf(
                                        equals(0), isBiggerThan(0x80000))))))

clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-warn-on-sizeof-pointer-to-aggregate.cpp

+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -config="{CheckOptions: [{key: bugprone-sizeof-expression.WarnOnSizeOfPointerToAggregate, value: false}]}" --
+
+class C {
+  int size() { return sizeof(this); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(this)'
+};
+
+#pragma pack(1)
+struct  S { char a, b, c; };
+
+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);
+  // No warning.
+  sum += sizeof(&kGlocalMyStruct.arr);
+  // No warning.
+  sum += sizeof(&kGlocalMyStructPtr->arr);
+  // No warning.
+  sum += sizeof(S.arr + 0);
+  // No warning.
+  sum += sizeof(+ S.arr);
+  // No warning.
+  sum += sizeof((int*)S.arr);
+  // No warning.
+
+  sum += sizeof(S.ptr);
+  // No warning.
+  sum += sizeof(kGlocalMyStruct.ptr);
+  // No warning.
+  sum += sizeof(kGlocalMyStructPtr->ptr);
+  // No warning.
+
+  sum += sizeof(&kGlocalMyStruct);
+  // No warning.
+  sum += sizeof(&S);
+  // No warning.
+  sum += sizeof(MyStruct*);
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // No warning.
+  sum += sizeof(PS2);
+  // No warning.
+  sum += sizeof(&A10);
+  // No warning.
+  sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
+  // No warning.
+  sum += sizeof(A10) / sizeof(PtrArray[0]);
+  // No warning.
+  sum += sizeof(PC) / sizeof(PtrArray[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+  sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
+
+  return sum;
+}