diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp index f3d4c2255d86e..f178faa194463 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp @@ -72,7 +72,9 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name, Options.get("WarnOnSizeOfPointerToAggregate", true)), WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)), WarnOnOffsetDividedBySizeOf( - Options.get("WarnOnOffsetDividedBySizeOf", true)) {} + Options.get("WarnOnOffsetDividedBySizeOf", true)), + WarnOnSizeOfInLoopTermination( + Options.get("WarnOnSizeOfInLoopTermination", true)) {} void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant); @@ -86,6 +88,8 @@ void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer); Options.store(Opts, "WarnOnOffsetDividedBySizeOf", WarnOnOffsetDividedBySizeOf); + Options.store(Opts, "WarnOnSizeOfInLoopTermination", + WarnOnSizeOfInLoopTermination); } void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { @@ -93,6 +97,13 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { // Some of the checks should not match in template code to avoid false // positives if sizeof is applied on template argument. + auto LoopCondExpr = + [](const ast_matchers::internal::Matcher &InnerMatcher) { + return stmt(anyOf(forStmt(hasCondition(InnerMatcher)), + whileStmt(hasCondition(InnerMatcher)), + doStmt(hasCondition(InnerMatcher)))); + }; + const auto IntegerExpr = ignoringParenImpCasts(integerLiteral()); const auto ConstantExpr = ignoringParenImpCasts( anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)), @@ -130,6 +141,14 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) { this); } + if (WarnOnSizeOfInLoopTermination) { + auto CondExpr = binaryOperator( + allOf(has(SizeOfExpr.bind("sizeof-expr")), isComparisonOperator())); + Finder->addMatcher(LoopCondExpr(anyOf(CondExpr, hasDescendant(CondExpr))) + .bind("loop-expr"), + this); + } + // Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"'; const auto CharPtrType = pointerType(pointee(isAnyCharacter())); const auto ConstStrLiteralDecl = @@ -353,6 +372,23 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) { diag(E->getBeginLoc(), "suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?") << E->getSourceRange(); + } else if (const auto *E = Result.Nodes.getNodeAs("loop-expr")) { + auto *SizeofArgTy = Result.Nodes.getNodeAs("sizeof-arg-type"); + if (const auto member = dyn_cast(SizeofArgTy)) + SizeofArgTy = member->getPointeeType().getTypePtr(); + + const auto *SzOfExpr = Result.Nodes.getNodeAs("sizeof-expr"); + + if (const auto type = dyn_cast(SizeofArgTy)) { + // check if the array element size is larger than one. If true, + // the size of the array is higher than the number of elements + CharUnits sSize = Ctx.getTypeSizeInChars(type->getElementType()); + if (!sSize.isOne()) { + diag(SzOfExpr->getBeginLoc(), + "suspicious usage of 'sizeof' in the loop") + << SzOfExpr->getSourceRange(); + } + } } else if (const auto *E = Result.Nodes.getNodeAs("sizeof-pointer")) { if (Result.Nodes.getNodeAs("struct-type")) { diag(E->getBeginLoc(), diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h index fbd62cb80fb2d..e979b4723cf2e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.h @@ -32,6 +32,7 @@ class SizeofExpressionCheck : public ClangTidyCheck { const bool WarnOnSizeOfPointerToAggregate; const bool WarnOnSizeOfPointer; const bool WarnOnOffsetDividedBySizeOf; + const bool WarnOnSizeOfInLoopTermination; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 579fca54924d5..76177073d01d6 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -146,6 +146,11 @@ Changes in existing checks ` check to detect conversion in argument of ``std::make_optional``. +- Improved :doc: `bugprone-sizeof-expression + ` check by adding + `WarnOnSizeOfInLoopTermination` option to detect misuses of ``sizeof`` + expression in loop conditions. + - Improved :doc:`bugprone-string-constructor ` check to find suspicious calls of ``std::string`` constructor with char pointer, start position and diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst index 29edb26ed7aa2..04824cc1fe0e4 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/sizeof-expression.rst @@ -316,3 +316,12 @@ Options When `true`, the check will warn on pointer arithmetic where the element count is obtained from a division with ``sizeof(...)``, e.g., ``Ptr + Bytes / sizeof(*T)``. Default is `true`. + +.. option:: WarnOnSizeOfInLoopTermination + + When `true`, the check will warn about incorrect use of sizeof expression + in loop termination condition. The warning triggers if the ``sizeof`` + expression appears to be incorrectly used to determine the number of + array/buffer elements. + e.g, ``long arr[10]; for(int i = 0; i < sizeof(arr); i++) { ... }``. Default + is `true`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp index 5e6f394152e9d..33cf1cbea8377 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp @@ -164,6 +164,69 @@ int Test2(MyConstChar* A) { return sum; } +struct A { + int array[10]; +}; + +struct B { + struct A a; +}; + +void loop_access_elements(int num, struct B b) { + struct A arr[10]; + char buf[20]; + + // CHECK-MESSAGES: :[[@LINE+1]]:22: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] + for(int i = 0; i < sizeof(arr); i++) { + struct A a = arr[i]; + } + + // Loop warning should not trigger here, even though this code is incorrect + // CHECK-MESSAGES: :[[@LINE+2]]:22: warning: suspicious usage of 'sizeof(K)'; did you mean 'K'? [bugprone-sizeof-expression] + // CHECK-MESSAGES: :[[@LINE+1]]:32: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator [bugprone-sizeof-expression] + for(int i = 0; i < sizeof(10)/sizeof(A); i++) { + struct A a = arr[i]; + } + + // Should not warn here + for(int i = 0; i < sizeof(arr)/sizeof(A); i++) {} + + // Should not warn here + for (int i = 0; i < 10; i++) { + if (sizeof(arr) != 0) { + + } + } + + for (int i = 0; i < 10; i++) { + // CHECK-MESSAGES: :[[@LINE+1]]:25: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] + for (int j = 0; j < sizeof(arr); j++) { + } + } + + // CHECK-MESSAGES: :[[@LINE+1]]:22: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] + for(int j = 0; j < sizeof(b.a.array); j++) {} + + // Should not warn here + for(int i = 0; i < sizeof(buf); i++) {} + + // Should not warn here + for(int i = 0; i < (sizeof(arr) << 3); i++) {} + + int i = 0; + // CHECK-MESSAGES: :[[@LINE+1]]:14: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] + while(i <= sizeof(arr)) {i++;} + + i = 0; + do { + i++; + // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] + } while(i <= sizeof(arr)); + + // CHECK-MESSAGES: :[[@LINE+1]]:29: warning: suspicious usage of 'sizeof' in the loop [bugprone-sizeof-expression] + for(int i = 0, j = 0; i < sizeof(arr) && j < sizeof(buf); i++, j++) {} +} + template int Foo() { int A[T]; return sizeof(T); } // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'