Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression #94356

Merged
merged 7 commits into from
Jun 11, 2024
Merged

[clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression #94356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c94feff
[clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression
NagyDonat May 22, 2024
4de254d
Update the documentation and release notes
NagyDonat Jun 6, 2024
b3a7f8b
Use single back-ticks for option name
NagyDonat Jun 6, 2024
ce3a376
Generalize the suppression to all `sizeof(array[0])` expressions
NagyDonat Jun 7, 2024
d8e6228
Merge branch 'SizeofPointer-move-to-tidy' of github_via_443:Ericsson/…
NagyDonat Jun 7, 2024
ae8123a
Satisfy git-clang-format
NagyDonat Jun 7, 2024
59bd1b8
Extend `GenericFunctionTest`, document a class of FPs with a FIXME
NagyDonat Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Generalize the suppression to all sizeof(array[0]) expressions 
...instead of just the RHS of the `sizeof(array) / sizeof(array[0])`
expression. This simplifies the code *and* will suppress many false
positives in various open source projects.
  • Loading branch information
@NagyDonat
NagyDonat committed Jun 7, 2024
commit ce3a37610228ceb9a9e60575f87886bf8e183493
31 changes: 10 additions & 21 deletions clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,11 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
// than other sizeof(ptr) expressions because they can appear as distorted
// forms of the common sizeof(aggregate) expressions.)
//
// To avoid false positives, some idiomatic constructs are accepted:
// + the RHS of a 'sizeof(arr) / sizeof(arr[0])' expression;
// + 'sizeof(*pp)' where 'pp' a pointer-to-pointer value, because this is
// a natural solution when dynamical typing is emulated by passing
// arguments as `generic_function(..., (void *)pp, sizeof(*pp))`.
// To avoid false positives, the check doesn't report expressions like
// 'sizeof(pp[0])' and 'sizeof(*pp)' where `pp` is a pointer-to-pointer or
// array of pointers. (This filters out both `sizeof(arr) / sizeof(arr[0])`
// expressions and other cases like `p = realloc(p, newsize * sizeof(*p));`.)
//
// Moreover this generic message is suppressed in cases that are also matched
// by the more concrete matchers 'sizeof-this' and 'sizeof-charp'.
if (WarnOnSizeOfPointerToAggregate || WarnOnSizeOfPointer) {
Expand All @@ -175,31 +175,20 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
: expr(anyOf(ArrayCastExpr, PointerToArrayExpr,
PointerToStructExpr));

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)));
const auto ArrayLengthExprDenom =
expr(hasParent(binaryOperator(hasOperatorName("/"),
hasLHS(ignoringParenImpCasts(sizeOfExpr(
has(ArrayOfPointersExpr)))))),
sizeOfExpr(has(ArrayOfSamePointersZeroSubscriptExpr)));
const auto SubscriptExprWithZeroIndex =
arraySubscriptExpr(
hasIndex(ZeroLiteral));
const auto DerefExpr =
ignoringParenImpCasts(unaryOperator(hasOperatorName("*")));

Finder->addMatcher(
expr(sizeOfExpr(anyOf(has(ignoringParenImpCasts(
expr(PointerToDetectedExpr, unless(DerefExpr),
unless(SubscriptExprWithZeroIndex),
unless(VarWithConstStrLiteralDecl),
unless(cxxThisExpr())))),
has(PointerToStructTypeWithBinding))),
unless(ArrayLengthExprDenom))
has(PointerToStructTypeWithBinding))))
.bind("sizeof-pointer"),
this);
}
Expand Down
5 changes: 2 additions & 3 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,11 @@ int Test5() {
sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(A10) / sizeof(PtrArray[0]);
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(PC) / sizeof(PtrArray[0]);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
// CHECK-MESSAGES: :[[@LINE-2]]:21: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
// CHECK-MESSAGES: :[[@LINE-3]]:23: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer

sum += sizeof(PChar);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
Expand All @@ -193,6 +190,8 @@ void GenericFunctionTest() {
// a generic function which emulates dynamic typing within C.
some_generic_function(IntPP, sizeof(*IntPP));
some_generic_function(ClassPP, sizeof(*ClassPP));
some_generic_function(IntPP, sizeof(IntPP[0]));
some_generic_function(ClassPP, sizeof(ClassPP[0]));
}

int ValidExpressions() {
Expand Down
5 changes: 2 additions & 3 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,14 +245,11 @@ int Test5() {
sum += sizeof(PtrArray) / sizeof(PtrArray[1]);
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(A10) / sizeof(PtrArray[0]);
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(PC) / sizeof(PtrArray[0]);
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
// CHECK-MESSAGES: :[[@LINE-2]]:21: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
// CHECK-MESSAGES: :[[@LINE-3]]:23: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer
sum += sizeof(ArrayC) / sizeof(PtrArray[0]);
// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; numerator is not a multiple of denominator
// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: suspicious usage of 'sizeof()' on an expression that results in a pointer

// These pointers do not point to aggregate types, so they are not reported in this mode:
sum += sizeof(PChar);
Expand Down Expand Up @@ -312,6 +309,8 @@ void GenericFunctionTest() {
// a generic function which emulates dynamic typing within C.
some_generic_function(IntPP, sizeof(*IntPP));
some_generic_function(ClassPP, sizeof(*ClassPP));
some_generic_function(IntPP, sizeof(IntPP[0]));
some_generic_function(ClassPP, sizeof(ClassPP[0]));
}

int ValidExpressions() {
Expand Down