[clang] [clang][analyzer] Improve documentation of checker 'cplusplus.Move' (NFC) (PR #96295)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/96295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve documentation of checker 'cplusplus.Move' (NFC) (PR #96295)
https://github.com/NagyDonat approved this pull request. LGTM, thanks for updating the docs! I only have one minor inline remark about formatting. https://github.com/llvm/llvm-project/pull/96295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve documentation of checker 'cplusplus.Move' (NFC) (PR #96295)
@@ -420,21 +420,52 @@ around, such as ``std::string_view``.
cplusplus.Move (C++)
-Method calls on a moved-from object and copying a moved-from object will be
reported.
-
+Find use-after-move bugs in C++. This includes method calls on moved-from
+objects, assignment of a moved-from object, and repeated move of a moved-from
+object.
.. code-block:: cpp
- struct A {
+ struct A {
void foo() {}
};
- void f() {
+ void f1() {
A a;
A b = std::move(a); // note: 'a' became 'moved-from' here
a.foo();// warn: method call on a 'moved-from' object 'a'
}
+ void f2() {
+ A a;
+ A b = std::move(a);
+ A c(std::move(a)); // warn: move of an already moved-from object
+ }
+
+ void f3() {
+ A a;
+ A b = std::move(a);
+ b = a; // warn: copy of moved-from object
+ }
+
+The checker option ``WarnOn`` controls on what objects the use-after-move is
+checked. The most strict value is ``KnownsOnly``, in this mode only objects are
+checked whose type is known to be move-unsafe. These include most STL objects
+(but excluding move-safe ones) and smart pointers. With option value
+``KnownsAndLocals`` local variables (of any type) are additionally checked. The
+idea behind this is that local variables are usually not tempting to be re-used
+so an use after move is more likely a bug than with member variables. With
+option value ``All`` any use-after move condition is checked on all kinds of
+variables, excluding global variables and known move-safe cases. Default value
+is ``KnownsAndLocals``.
NagyDonat wrote:
```suggestion
The checker option ``WarnOn`` controls on what objects the use-after-move is
checked.
* The most strict value is ``KnownsOnly``, in this mode only objects are
checked whose type is known to be move-unsafe. These include most STL objects
(but excluding move-safe ones) and smart pointers.
* With option value ``KnownsAndLocals`` local variables (of any type) are
additionally checked. The idea behind this is that local variables are
usually not tempting to be re-used so an use after move is more likely a bug
than with member variables.
* With option value ``All`` any use-after move condition is checked on all
kinds of variables, excluding global variables and known move-safe cases.
Default value is ``KnownsAndLocals``.
```
This paragraph was very long, re-flow it into a bullet point list.
https://github.com/llvm/llvm-project/pull/96295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improve documentation of checker 'cplusplus.Move' (NFC) (PR #96295)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/96295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
" numerator is not a multiple of denominator")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (ElementSize > CharUnits::Zero() &&
DenominatorSize > CharUnits::Zero() &&
ElementSize != DenominatorSize) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
-" numerator is not a multiple of denominator")
+ diag(E->getOperatorLoc(),
+ "suspicious usage of 'sizeof(array)/sizeof(...)':"
NagyDonat wrote:
It would be slightly better in isolation, but all the other diagnostics look
like e.g. `suspicious usage of 'sizeof(..., ...)'` or `suspicious usage of
'sizeof(this)'; did you mean 'sizeof(*this)'` so for the sake of consistency
I'd like to leave this aspect of the messages unchanged.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/95550
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH 1/3] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index c25ee42d0899a..3afa0e5c39882 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -296,7 +296,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("sizeof-integer-call")) {
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
- "that results in an integer")
+ "of integer type")
<< E->getSourceRange();
} else if (const auto *E = Result.Nodes.getNodeAs("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< E->getSourceRange();
} else {
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
- "that results in a pointer")
+ "of pointer type")
<< E->getSourceRange();
}
} else if (const auto *E = Result.Nodes.getNodeAs(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
" numerator is not a multiple of denominator")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (ElementSize > CharUnits::Zero() &&
DenominatorSize > CharUnits::Zero() &&
ElementSize != DenominatorSize) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
-" numerator is not a multiple of denominator")
+ diag(E->getOperatorLoc(),
+ "suspicious usage of 'sizeof(array)/sizeof(...)':"
+ " denominator differs from the size of array elements")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy == DenomTy) {
- // FIXME: This message is wrong, it should not refer to sizeof "pointer"
- // usage (and by the way, it would be to clarify all the messages).
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have the same type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (!WarnOnSizeOfPointer) {
// When 'WarnOnSizeOfPointer' is enabled, these messages become
redundant:
if (PointedTy && DenomTy && PointedTy == DenomTy) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': size of pointer "
+ "is divided by size of pointed type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
DenomTy->isPointerType()) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have pointer types")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
}
}
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
index aef930f2c8fda..b898071a56613 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -34,24 +34,24 @@ int Test5() {
int sum = 0;
sum += sizeof();
- //
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/95550
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH 1/2] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index c25ee42d0899a..3afa0e5c39882 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -296,7 +296,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("sizeof-integer-call")) {
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
- "that results in an integer")
+ "of integer type")
<< E->getSourceRange();
} else if (const auto *E = Result.Nodes.getNodeAs("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< E->getSourceRange();
} else {
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
- "that results in a pointer")
+ "of pointer type")
<< E->getSourceRange();
}
} else if (const auto *E = Result.Nodes.getNodeAs(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
" numerator is not a multiple of denominator")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (ElementSize > CharUnits::Zero() &&
DenominatorSize > CharUnits::Zero() &&
ElementSize != DenominatorSize) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
-" numerator is not a multiple of denominator")
+ diag(E->getOperatorLoc(),
+ "suspicious usage of 'sizeof(array)/sizeof(...)':"
+ " denominator differs from the size of array elements")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy == DenomTy) {
- // FIXME: This message is wrong, it should not refer to sizeof "pointer"
- // usage (and by the way, it would be to clarify all the messages).
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have the same type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (!WarnOnSizeOfPointer) {
// When 'WarnOnSizeOfPointer' is enabled, these messages become
redundant:
if (PointedTy && DenomTy && PointedTy == DenomTy) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': size of pointer "
+ "is divided by size of pointed type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
DenomTy->isPointerType()) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have pointer types")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
}
}
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
index aef930f2c8fda..b898071a56613 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -34,24 +34,24 @@ int Test5() {
int sum = 0;
sum += sizeof();
- //
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: @PiotrZSL I see, thanks for the explanation, the "colons may confuse parsers" question is a good point. I think I'll return to using semicolons, because that's what was used before my commit. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
NagyDonat wrote:
I kept "usage of" and the apostrophes to be consistent with the rest of the
messages emitted by this checker. If you think that this is important, it would
be possible to extend the scope of this commit to apply these changes in all
the other messages.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
" numerator is not a multiple of denominator")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (ElementSize > CharUnits::Zero() &&
DenominatorSize > CharUnits::Zero() &&
ElementSize != DenominatorSize) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
-" numerator is not a multiple of denominator")
+ diag(E->getOperatorLoc(),
+ "suspicious usage of 'sizeof(array)/sizeof(...)':"
+ " denominator differs from the size of array elements")
NagyDonat wrote:
Yes, this is only triggered when
`arrayType(hasElementType(recordType().bind("elem-type")))` matches, so we have
an array type whose elements are records.
https://github.com/llvm/llvm-project/pull/95550
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -461,7 +446,7 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext , if (!ER) return State; - const TypedValueRegion *Orig = getOriginRegion(ER); + const auto *Orig = ER->getSuperRegion()->getAs(); NagyDonat wrote: Nitpick: also change the name "`Orig`". https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,162 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SVB.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SVB.makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SVB.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin region (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ return dyn_cast_or_null(Ret);
+}
+
+// Basically 1 -> 1st, 12 -> 12th, etc.
+static void printIdxWithOrdinalSuffix(llvm::raw_ostream , unsigned Idx) {
+ Os << Idx << llvm::getOrdinalSuffix(Idx);
NagyDonat wrote:
```suggestion
switch (Idx) {
case 1:
Os << "first";
break;
case 2:
Os << "second";
break;
case 3:
Os << "third";
break;
default:
Os << Idx << llvm::getOrdinalSuffix(Idx);
}
```
Consider printing "first", "second" and "third" instead of "1st", "2nd" and
"3rd" because IMO it makes the diagnostics easier to read -- and this function
will be used for argument indices in builtin functions, so these cases will be
very common.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,162 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SVB.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SVB.makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SVB.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin region (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ return dyn_cast_or_null(Ret);
+}
NagyDonat wrote:
I thought several hours about this function, and my TLDR conclusion is that (1)
this logic is wrong (2) `SymExpr::getOriginRegion()` is a red flag and (3) just
use a plain `getSuperRegion()` call instead of this function. (Note that the
branch where SymExpr::getOriginRegion() is used is not exercised in any
testcase.)
More precisely, the problem is that `SymExpr::getOriginRegion()` is an
extremely unnatural operation, which extracts a part of the "opaque unique tag"
of the symbol. The result of `S->getOriginRegion()` is:
- the region _R_ when `S` is a `SymbolRegionValue` which represents "the value
stored in the memory region _R_ at the start of the analysis of this function"
- the region _R_ when `S` is a `SymbolDerived` which represents "the value
stored in the memory region _R_ after a certain invalidation"
- nullpointer in other cases (e.g. `SymbolConjured`).
As a concrete example, if you have a function like
```c++
void save_to_logs(int *src, int begin, int count) {
memcpy(global_log_array, src + begin, count * sizeof(int));
}
```
then the element region representing `src + begin` would be passed to
`getOriginRegion()`, which would first strip the `ElementRegion` layer to get a
`SymbolicRegion`, then unwrap that to get a `SymbolRegionValue` and finally
return the `ParamVarRegion` which represents the pointer-sized memory area of
the variable `src`.
This won't lead to a crash because this `ParamVarRegion` is immediately
discarded by the next check, which requires an array type:
```
if (!Orig->getValueType()->isArrayType())
return State;
```
However, there are no situations where `getOriginRegion()` = "region where a
pointer to this symbolic region was stored at some earlier step" is actually
useful, so you should just remove the branch using it.
By the way, I see that you need a `TypedValueRegion` after this point to
continue the calculations -- but if the SuperRegion of your ElementRegion is a
SymbolicRegion, then the right thing to do is just discarding it. A
`SymbolicRegion` is not necessarily a real "region", it may just represent a
pointer that points into the middle of some opaque unknown memory.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat requested changes to this pull request. Thanks for the updates! Unfortunately, as I dug deeper into your function `getOriginRegion()`, I realized that it is logically incorrect (see inline comment for explanation). (I was a bit suspicious during the earlier reviews as well, but this is a very complex area and earlier I didn't spend enough time on it.) https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ if (const auto *element = MR->getAs()) {
+Ret = element->getBaseRegion();
+ }
+ return dyn_cast_or_null(Ret);
+}
+
+// Basically 1 -> 1st, 12 -> 12th, etc.
+static void printIdxWithOrdinalSuffix(llvm::raw_ostream , unsigned Idx) {
+ Os << Idx << llvm::getOrdinalSuffix(Idx);
+}
+
+ProgramStateRef CStringChecker::checkInit(CheckerContext ,
+ ProgramStateRef State,
+ AnyArgExpr Buffer, SVal Element,
+ SVal Size) const {
+
+ // If a previous check has failed, propagate the failure.
+ if (!State)
+return nullptr;
+
+ const MemRegion *R = Element.getAsRegion();
+ if (!R)
+return State;
+
+ const auto *ER = dyn_cast(R);
+ if (!ER)
+return State;
+
+ const TypedValueRegion *Orig = getOriginRegion(ER);
+ if (!Orig)
+return State;
+
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ // FIXME: We ought to able to check objects as well. Maybe
+ // UninitializedObjectChecker could help?
+ if (!Orig->getValueType()->isArrayType())
+return State;
+
+ const QualType ElemTy = Ctx.getBaseElementType(Orig->getValueType());
+ const NonLoc Zero = SValBuilder.makeZeroArrayIndex();
+
+ SVal FirstElementVal =
+ State->getLValue(ElemTy, Zero, loc::MemRegionVal(Orig)).castAs();
+ if (!isa(FirstElementVal))
+return State;
+
+ // Ensure that we wouldn't read uninitialized value.
+ if (Filter.CheckCStringUninitializedRead &&
+ State->getSVal(FirstElementVal.castAs()).isUndef()) {
+llvm::SmallString<258> Buf;
+llvm::raw_svector_ostream OS(Buf);
+OS << "The first element of the ";
+printIdxWithOrdinalSuffix(OS, Buffer.ArgumentIndex + 1);
+OS << " argument is undefined";
+emitUninitializedReadBug(C, State, Buffer.Expression, OS.str());
+return nullptr;
+ }
+
+ // We won't check whether the entire region is fully initialized -- lets just
+ // check that the first and the last element is. So, onto checking the last
+ // element:
+ const QualType IdxTy = SValBuilder.getArrayIndexType();
+
+ NonLoc ElemSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(ElemTy).getQuantity(), IdxTy)
+ .castAs();
+
+ // FIXME: Check that the size arg to the cstring function is divisible by
+ // size of the actual element type?
+
+ // The type of the argument to the cstring function is either char or wchar,
+ // but thats not the type of the original array (or memory region).
+ // Suppose the following:
+ // int t[5];
+ // memcpy(dst, t, sizeof(t) / sizeof(t[0]));
+ // When checking whether t is fully initialized, we see it as char array of
+ // size sizeof(int)*5. If we check the last element as a character, we read
+ // the last byte of an integer, which will be undefined. But just because
+ // that value is undefined, it doesn't mean that the element is
uninitialized!
+ // For this reason, we need to retrieve the actual last element with the
+ // correct type.
+
+ // Divide the size argument to the cstring function by the actual element
+ // type. This value will be size of the array, or the index to the
+ // past-the-end element.
+ std::optional Offset =
+ SValBuilder
+ .evalBinOpNN(State, clang::BO_Div, Size.castAs(), ElemSize,
+ IdxTy)
+ .getAs();
+
+ // Retrieve the index of the last element.
+ const NonLoc One = SValBuilder.makeIntVal(1, IdxTy).castAs();
+ SVal LastIdx = SValBuilder.evalBinOpNN(State, BO_Sub, *Offset, One, IdxTy);
+
+ if
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
@@ -32,3 +32,20 @@ void different_2() {
int d = p2 - p1; // expected-warning{{Subtraction of two pointers that do
not point into the same array is undefined behavior}} \
// expected-note{{Subtraction of two pointers that do not
point into the same array is undefined behavior}}
}
+
+int different_3() {
+ struct {
+int array[5];
+ } a, b;
+ return [3] - [2]; // expected-warning{{Subtraction of two
pointers that do not point into the same array is undefined behavior}} \
+// expected-note{{Subtraction of two
pointers that do not point into the same array is undefined behavior}}
+}
+
+int different_5() {
NagyDonat wrote:
```suggestion
int different_4() {
```
Very minor nitpick: `different_3` was followed by `different_5`.
https://github.com/llvm/llvm-project/pull/95899
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
@@ -154,12 +154,14 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator
*B,
auto R =
std::make_unique(BT, Msg_MemRegionDifferent,
N);
R->addRange(B->getSourceRange());
-if (DiffDeclL)
- R->addNote("Array at the left-hand side of subtraction",
- {DiffDeclL, C.getSourceManager()});
-if (DiffDeclR)
- R->addNote("Array at the right-hand side of subtraction",
- {DiffDeclR, C.getSourceManager()});
+if (DiffDeclL != DiffDeclR) {
NagyDonat wrote:
```suggestion
if (DiffDeclL != DiffDeclR) {
// The declarations may be identical even if the regions are different,
// if they are field regions within different objects:
// struct { int array[10]; } a, b;
// do_something_with(a.array[5] - b.array[5]);
// In this case the notes would be confusing, so don't emit them.
```
Add a comment that explains this `if (DiffDeclL != DiffDeclR)` check because
otherwise it would be very surprising.
https://github.com/llvm/llvm-project/pull/95899
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
https://github.com/NagyDonat approved this pull request. LGTM, thanks for the update. I added two minor edit suggestions, but after that the commit can be merged. https://github.com/llvm/llvm-project/pull/95899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: If there is no feedback on the separator mark bikeshedding (which is understandable -- it's a very minor question), I'll merge this commit on Thursday. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
@@ -144,9 +144,24 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator
*B,
// Allow arithmetic on different symbolic regions.
if (isa(SuperLR) || isa(SuperRR))
return;
+if (const auto *SuperDLR = dyn_cast(SuperLR))
+ DiffDeclL = SuperDLR->getDecl();
+if (const auto *SuperDRR = dyn_cast(SuperRR))
+ DiffDeclR = SuperDRR->getDecl();
NagyDonat wrote:
Note that `FieldRegion`s are `DeclRegion`s, so these declarations may be data
member declarations within a `struct` or `class`. This is usually not a
problem, but there is a corner case where `SuperLR != SuperRR`, but the
corresponding declarations are identical:
```
struct {
int array[5];
} a, b;
int func(void) {
return [3] - [2];
}
```
In this case the current code would place both notes onto the declaration of
`field`, which would be confusing for the user.
Consider adding some code that handles this situation explicitly. (Either
simply skip note creation when `DiffDeclL == DiffDeclR`, or create a
specialized note for this case.)
https://github.com/llvm/llvm-project/pull/95899
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
https://github.com/NagyDonat commented: Looks good overall, I have one remark about a rare case that would require some specialized code. https://github.com/llvm/llvm-project/pull/95899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Add notes to PointerSubChecker (PR #95899)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95899 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ if (const auto *element = MR->getAs()) {
+Ret = element->getBaseRegion();
+ }
+ return dyn_cast_or_null(Ret);
+}
+
+// Basically 1 -> 1st, 12 -> 12th, etc.
+static void printIdxWithOrdinalSuffix(llvm::raw_ostream , unsigned Idx) {
+ Os << Idx << llvm::getOrdinalSuffix(Idx);
+}
+
+ProgramStateRef CStringChecker::checkInit(CheckerContext ,
+ ProgramStateRef State,
+ AnyArgExpr Buffer, SVal Element,
+ SVal Size) const {
+
+ // If a previous check has failed, propagate the failure.
+ if (!State)
+return nullptr;
+
+ const MemRegion *R = Element.getAsRegion();
+ if (!R)
+return State;
+
+ const auto *ER = dyn_cast(R);
+ if (!ER)
+return State;
+
+ const TypedValueRegion *Orig = getOriginRegion(ER);
+ if (!Orig)
+return State;
+
+ SValBuilder = State->getStateManager().getSValBuilder();
NagyDonat wrote:
I agree with both suggestions, `SVB` is the _right_ name mandated by our _holy
traditions_.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] Reland [clang][Sema, Lex, Parse] Preprocessor embed in C and C++ (PR #95802)
@@ -441,6 +441,7 @@ tok::PPKeywordKind IdentifierInfo::getPPKeywordID() const {
CASE( 4, 'e', 's', else);
CASE( 4, 'l', 'n', line);
CASE( 4, 's', 'c', sccs);
+ CASE(5, 'e', 'b', embed);
NagyDonat wrote:
```suggestion
CASE( 5, 'e', 'b', embed);
```
Just drive-by bikeshedding -- consider aligning this like the lines around it.
https://github.com/llvm/llvm-project/pull/95802
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] use unqualified canonical type during merging equivalence class (PR #95729)
https://github.com/NagyDonat approved this pull request. Nice catch, thanks for fixing this bug! https://github.com/llvm/llvm-project/pull/95729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: > Consider replacing ":" separator with "," I switched to ":" (instead of the ";" that was used previously) to emphasize that the second part of the message is an explanation/reason for the first part (and not a second, separate issue). However I can switch to "," or ";" if that option turns out to be more popular. @ rewiewers: Which symbol would you prefer in these messages? (My preferences are dot > keep the semicolon ≳ comma.) https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
https://github.com/NagyDonat created
https://github.com/llvm/llvm-project/pull/95550
...becasue they were strangely worded and in a few cases outright incorrect.
From 06adc063c2388ea534537f5a417751fdf64b22cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Fri, 14 Jun 2024 15:16:34 +0200
Subject: [PATCH] [clang-tidy] Clarify diagnostics of
bugprone-sizeof-expression
...becasue they were strangely worded and in a few cases outright
incorrect.
---
.../bugprone/SizeofExpressionCheck.cpp| 22 ++---
.../checkers/bugprone/sizeof-expression-2.c | 12 +--
.../sizeof-expression-any-pointer.cpp | 86 +--
...on-warn-on-sizeof-pointer-to-aggregate.cpp | 4 +-
.../checkers/bugprone/sizeof-expression.cpp | 84 +-
5 files changed, 105 insertions(+), 103 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index c25ee42d0899a..3afa0e5c39882 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -296,7 +296,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
} else if (const auto *E =
Result.Nodes.getNodeAs("sizeof-integer-call")) {
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
- "that results in an integer")
+ "of integer type")
<< E->getSourceRange();
} else if (const auto *E = Result.Nodes.getNodeAs("sizeof-this")) {
diag(E->getBeginLoc(),
@@ -314,7 +314,7 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
<< E->getSourceRange();
} else {
diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
- "that results in a pointer")
+ "of pointer type")
<< E->getSourceRange();
}
} else if (const auto *E = Result.Nodes.getNodeAs(
@@ -342,31 +342,33 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
if (DenominatorSize > CharUnits::Zero() &&
!NumeratorSize.isMultipleOf(DenominatorSize)) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
+ diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)':"
" numerator is not a multiple of denominator")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (ElementSize > CharUnits::Zero() &&
DenominatorSize > CharUnits::Zero() &&
ElementSize != DenominatorSize) {
- diag(E->getOperatorLoc(), "suspicious usage of
'sizeof(...)/sizeof(...)';"
-" numerator is not a multiple of denominator")
+ diag(E->getOperatorLoc(),
+ "suspicious usage of 'sizeof(array)/sizeof(...)':"
+ " denominator differs from the size of array elements")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy == DenomTy) {
- // FIXME: This message is wrong, it should not refer to sizeof "pointer"
- // usage (and by the way, it would be to clarify all the messages).
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have the same type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (!WarnOnSizeOfPointer) {
// When 'WarnOnSizeOfPointer' is enabled, these messages become
redundant:
if (PointedTy && DenomTy && PointedTy == DenomTy) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': size of pointer "
+ "is divided by size of pointed type")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
} else if (NumTy && DenomTy && NumTy->isPointerType() &&
DenomTy->isPointerType()) {
diag(E->getOperatorLoc(),
- "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'")
+ "suspicious usage of 'sizeof(...)/sizeof(...)': both expressions "
+ "have pointer types")
<< E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
}
}
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
index aef930f2c8fda..b898071a56613 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@
[clang-tools-extra] [clang-tidy] Clarify diagnostics of bugprone-sizeof-expression (PR #95550)
NagyDonat wrote: This commit is motivated by the discussion on https://github.com/llvm/llvm-project/pull/94356 . Feel free to bikeshed the messages, I'm not too attached to the new choices, I'm just unsatisfied with the old ones. https://github.com/llvm/llvm-project/pull/95550 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
@@ -26,17 +28,10 @@ namespace {
class Z3Config {
NagyDonat wrote:
If I understand it correctly, rule of three/five dictates that the copy/move
assignment/construction of this class needs to be handled (or deleted).
https://github.com/llvm/llvm-project/pull/95128
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
https://github.com/NagyDonat commented: I tried to review the resource management code thoroughly, but I admit that I'm not skilled in this area -- I'm used to working in modern C++ and Python where these footguns are less relevant. I found two classed that (if I understand correctly) violate the "rule of three", but I didn't find any acute issues. https://github.com/llvm/llvm-project/pull/95128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
@@ -50,16 +45,17 @@ void Z3ErrorHandler(Z3_context Context, Z3_error_code
Error) {
/// Wrapper for Z3 context
class Z3Context {
NagyDonat wrote:
Another class that manages a resource in a destructor, but does not handle
copy/move assignment/construction.
https://github.com/llvm/llvm-project/pull/95128
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -413,38 +588,18 @@ ProgramStateRef
CStringChecker::CheckLocation(CheckerContext ,
if (!ER)
return state;
- SValBuilder = C.getSValBuilder();
- ASTContext = svalBuilder.getContext();
-
// Get the index of the accessed element.
- NonLoc Idx = ER->getIndex();
-
- if (CK == CharKind::Regular) {
-if (ER->getValueType() != Ctx.CharTy)
- return state;
- } else {
-if (ER->getValueType() != Ctx.WideCharTy)
- return state;
-
-QualType SizeTy = Ctx.getSizeType();
-NonLoc WideSize =
-svalBuilder
-.makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
-SizeTy)
-.castAs();
-SVal Offset = svalBuilder.evalBinOpNN(state, BO_Mul, Idx, WideSize,
SizeTy);
-if (Offset.isUnknown())
- return state;
-Idx = Offset.castAs();
- }
+ std::optional Idx = getIndex(state, ER, CK);
+ if (!Idx)
+return state;
NagyDonat wrote:
Consider capitalizing `state` in this function to be consistent with the coding
guidelines and other functions.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat commented: I read the rest of the commit and only found two very minor observations. https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -26,16 +50,12 @@ void top(char *dst) {
void *mempcpy(void *restrict s1, const void *restrict s2, size_t n);
-void mempcpy14() {
+void mempcpy13() {
NagyDonat wrote:
Why are you renaming this to `memcpy13`? I see only two `memcpy` tests after
this and they have index 15 and 16, which is very random, but at least follows
14 directly.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -413,38 +588,18 @@ ProgramStateRef
CStringChecker::CheckLocation(CheckerContext ,
if (!ER)
return state;
- SValBuilder = C.getSValBuilder();
- ASTContext = svalBuilder.getContext();
-
// Get the index of the accessed element.
- NonLoc Idx = ER->getIndex();
-
- if (CK == CharKind::Regular) {
-if (ER->getValueType() != Ctx.CharTy)
- return state;
- } else {
-if (ER->getValueType() != Ctx.WideCharTy)
- return state;
-
-QualType SizeTy = Ctx.getSizeType();
-NonLoc WideSize =
-svalBuilder
-.makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
-SizeTy)
-.castAs();
-SVal Offset = svalBuilder.evalBinOpNN(state, BO_Mul, Idx, WideSize,
SizeTy);
-if (Offset.isUnknown())
- return state;
-Idx = Offset.castAs();
- }
+ std::optional Idx = getIndex(state, ER, CK);
+ if (!Idx)
+return state;
// Get the size of the array.
const auto *superReg = cast(ER->getSuperRegion());
DefinedOrUnknownSVal Size =
getDynamicExtent(state, superReg, C.getSValBuilder());
ProgramStateRef StInBound, StOutBound;
- std::tie(StInBound, StOutBound) = state->assumeInBoundDual(Idx, Size);
+ std::tie(StInBound, StOutBound) = state->assumeInBoundDual(*Idx, Size);
NagyDonat wrote:
Perhaps apply a [structured
binding](https://en.cppreference.com/w/cpp/language/structured_binding) instead
of `std::tie`?
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ if (const auto *element = MR->getAs()) {
+Ret = element->getBaseRegion();
+ }
+ return dyn_cast_or_null(Ret);
+}
+
+// Basically 1 -> 1st, 12 -> 12th, etc.
+static void printIdxWithOrdinalSuffix(llvm::raw_ostream , unsigned Idx) {
+ Os << Idx << llvm::getOrdinalSuffix(Idx);
+}
+
+ProgramStateRef CStringChecker::checkInit(CheckerContext ,
+ ProgramStateRef State,
+ AnyArgExpr Buffer, SVal Element,
+ SVal Size) const {
+
+ // If a previous check has failed, propagate the failure.
+ if (!State)
+return nullptr;
+
+ const MemRegion *R = Element.getAsRegion();
+ if (!R)
+return State;
+
+ const auto *ER = dyn_cast(R);
+ if (!ER)
+return State;
+
+ const TypedValueRegion *Orig = getOriginRegion(ER);
+ if (!Orig)
+return State;
+
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ // FIXME: We ought to able to check objects as well. Maybe
+ // UninitializedObjectChecker could help?
+ if (!Orig->getValueType()->isArrayType())
+return State;
NagyDonat wrote:
Perhaps move this above the definition of `SValBuilder` and `Ctx` to keep it
together with all the other early return statements.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
NagyDonat wrote: > Please have a second look at the llvm-related changes. Z3 has a really > dangerous C API. I shot myself in the foot literally 3 times while working on > this PR. > And btw notice that I removed the proof=false configuration as that is the > default AFAIK, so that's why that is missing. OK, I'll re-check the footgun handling either today or tomorrow. https://github.com/llvm/llvm-project/pull/95128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ if (const auto *element = MR->getAs()) {
+Ret = element->getBaseRegion();
NagyDonat wrote:
``
Please follow the convention that variable names start with a capital letter.
``
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
NagyDonat wrote:
Normal person: "2"
Programmer: "`Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity()`"
;-)
(No action expected, we're programmers...)
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ if (const auto *element = MR->getAs()) {
+Ret = element->getBaseRegion();
+ }
+ return dyn_cast_or_null(Ret);
+}
+
+// Basically 1 -> 1st, 12 -> 12th, etc.
+static void printIdxWithOrdinalSuffix(llvm::raw_ostream , unsigned Idx) {
+ Os << Idx << llvm::getOrdinalSuffix(Idx);
+}
+
+ProgramStateRef CStringChecker::checkInit(CheckerContext ,
+ ProgramStateRef State,
+ AnyArgExpr Buffer, SVal Element,
+ SVal Size) const {
+
+ // If a previous check has failed, propagate the failure.
+ if (!State)
+return nullptr;
+
+ const MemRegion *R = Element.getAsRegion();
+ if (!R)
+return State;
+
+ const auto *ER = dyn_cast(R);
+ if (!ER)
+return State;
+
+ const TypedValueRegion *Orig = getOriginRegion(ER);
+ if (!Orig)
+return State;
+
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ // FIXME: We ought to able to check objects as well. Maybe
+ // UninitializedObjectChecker could help?
+ if (!Orig->getValueType()->isArrayType())
+return State;
+
+ const QualType ElemTy = Ctx.getBaseElementType(Orig->getValueType());
+ const NonLoc Zero = SValBuilder.makeZeroArrayIndex();
+
+ SVal FirstElementVal =
+ State->getLValue(ElemTy, Zero, loc::MemRegionVal(Orig)).castAs();
+ if (!isa(FirstElementVal))
+return State;
+
+ // Ensure that we wouldn't read uninitialized value.
+ if (Filter.CheckCStringUninitializedRead &&
+ State->getSVal(FirstElementVal.castAs()).isUndef()) {
+llvm::SmallString<258> Buf;
+llvm::raw_svector_ostream OS(Buf);
+OS << "The first element of the ";
+printIdxWithOrdinalSuffix(OS, Buffer.ArgumentIndex + 1);
+OS << " argument is undefined";
+emitUninitializedReadBug(C, State, Buffer.Expression, OS.str());
+return nullptr;
+ }
+
+ // We won't check whether the entire region is fully initialized -- lets just
+ // check that the first and the last element is. So, onto checking the last
+ // element:
+ const QualType IdxTy = SValBuilder.getArrayIndexType();
+
+ NonLoc ElemSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(ElemTy).getQuantity(), IdxTy)
+ .castAs();
+
+ // FIXME: Check that the size arg to the cstring function is divisible by
+ // size of the actual element type?
+
+ // The type of the argument to the cstring function is either char or wchar,
+ // but thats not the type of the original array (or memory region).
+ // Suppose the following:
+ // int t[5];
+ // memcpy(dst, t, sizeof(t) / sizeof(t[0]));
+ // When checking whether t is fully initialized, we see it as char array of
+ // size sizeof(int)*5. If we check the last element as a character, we read
+ // the last byte of an integer, which will be undefined. But just because
+ // that value is undefined, it doesn't mean that the element is
uninitialized!
+ // For this reason, we need to retrieve the actual last element with the
+ // correct type.
+
+ // Divide the size argument to the cstring function by the actual element
+ // type. This value will be size of the array, or the index to the
+ // past-the-end element.
+ std::optional Offset =
+ SValBuilder
+ .evalBinOpNN(State, clang::BO_Div, Size.castAs(), ElemSize,
+ IdxTy)
+ .getAs();
+
+ // Retrieve the index of the last element.
+ const NonLoc One = SValBuilder.makeIntVal(1, IdxTy).castAs();
+ SVal LastIdx = SValBuilder.evalBinOpNN(State, BO_Sub, *Offset, One, IdxTy);
+
+ if
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -413,38 +588,18 @@ ProgramStateRef
CStringChecker::CheckLocation(CheckerContext ,
if (!ER)
return state;
- SValBuilder = C.getSValBuilder();
- ASTContext = svalBuilder.getContext();
-
// Get the index of the accessed element.
- NonLoc Idx = ER->getIndex();
-
- if (CK == CharKind::Regular) {
-if (ER->getValueType() != Ctx.CharTy)
- return state;
- } else {
-if (ER->getValueType() != Ctx.WideCharTy)
- return state;
-
-QualType SizeTy = Ctx.getSizeType();
-NonLoc WideSize =
-svalBuilder
-.makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
-SizeTy)
-.castAs();
-SVal Offset = svalBuilder.evalBinOpNN(state, BO_Mul, Idx, WideSize,
SizeTy);
-if (Offset.isUnknown())
- return state;
-Idx = Offset.castAs();
- }
+ std::optional Idx = getIndex(state, ER, CK);
+ if (!Idx)
+return state;
// Get the size of the array.
const auto *superReg = cast(ER->getSuperRegion());
DefinedOrUnknownSVal Size =
getDynamicExtent(state, superReg, C.getSValBuilder());
ProgramStateRef StInBound, StOutBound;
- std::tie(StInBound, StOutBound) = state->assumeInBoundDual(Idx, Size);
+ std::tie(StInBound, StOutBound) = state->assumeInBoundDual(*Idx, Size);
NagyDonat wrote:
Consider using a structured binding instead of `std::tie`.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
+// element region).
+static const TypedValueRegion *getOriginRegion(const ElementRegion *ER) {
+ const MemRegion *MR = ER->getSuperRegion();
+ const MemRegion *Ret = MR;
+ assert(MR);
+ if (const auto *sym = MR->getAs()) {
+SymbolRef sym2 = sym->getSymbol();
+if (!sym2)
+ return nullptr;
+Ret = sym2->getOriginRegion();
+ }
+ if (const auto *element = MR->getAs()) {
+Ret = element->getBaseRegion();
NagyDonat wrote:
This `getBaseRegion()` call could produce some surprising and inconsistent
results: e.g. if you have
```
struct S {
int field[20][20];
} array[10];
```
then calling `getOriginRegion` on the ElementRegion representing
`array[5].field[3]` would return the FieldRegion representing `array[5].field`,
but (if I understand this correctly) calling it on `array[5].field[3][6]` would
return the NonParamVarRegion representing `array` because `getBaseRegion()`
strips the FieldRegion layer.
Consider writing an explicit loop that only strips ElementRegion layers.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -393,6 +401,173 @@ ProgramStateRef
CStringChecker::checkNonNull(CheckerContext ,
return stateNonNull;
}
+static std::optional getIndex(ProgramStateRef State,
+ const ElementRegion *ER, CharKind CK) {
+ SValBuilder = State->getStateManager().getSValBuilder();
+ ASTContext = SValBuilder.getContext();
+
+ if (CK == CharKind::Regular) {
+if (ER->getValueType() != Ctx.CharTy)
+ return {};
+return ER->getIndex();
+ }
+
+ if (ER->getValueType() != Ctx.WideCharTy)
+return {};
+
+ QualType SizeTy = Ctx.getSizeType();
+ NonLoc WideSize =
+ SValBuilder
+ .makeIntVal(Ctx.getTypeSizeInChars(Ctx.WideCharTy).getQuantity(),
+ SizeTy)
+ .castAs();
+ SVal Offset =
+ SValBuilder.evalBinOpNN(State, BO_Mul, ER->getIndex(), WideSize, SizeTy);
+ if (Offset.isUnknown())
+return {};
+ return Offset.castAs();
+}
+
+// Try to get hold of the origin regin (e.g. the actual array region from an
NagyDonat wrote:
```suggestion
// Try to get hold of the origin region (e.g. the actual array region from an
```
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat commented: Overall I'm satisfied with this commit, but I have some minor remarks. I'm publishing some review comments now, but I'll continue the review tomorrow. https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
@@ -26,16 +50,12 @@ void top(char *dst) {
void *mempcpy(void *restrict s1, const void *restrict s2, size_t n);
-void mempcpy14() {
+void mempcpy13() {
int src[] = {1, 2, 3, 4};
int dst[5] = {0};
int *p;
- p = mempcpy(dst, src, 4 * sizeof(int)); // expected-warning{{Bytes string
function accesses uninitialized/garbage values}}
- // FIXME: This behaviour is actually surprising and needs to be fixed,
- // mempcpy seems to consider the very last byte of the src buffer
uninitialized
- // and returning undef unfortunately. It should have returned unknown or a
conjured value instead.
-
+ p = mempcpy(dst, src, 4 * sizeof(int));
clang_analyzer_eval(p == [4]); // no-warning (above is fatal)
NagyDonat wrote:
Delete or update the comment on this line.
https://github.com/llvm/llvm-project/pull/95408
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Check the correct first and last elements in cstring.UninitializedRead (PR #95408)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
@@ -434,6 +447,12 @@ class SMTSolver {
virtual bool isFPSupported() = 0;
virtual void print(raw_ostream ) const = 0;
+
+ /// Sets the requested option.
+ virtual void setBoolParam(StringRef Key, bool Value) = 0;
+ virtual void setUnsignedParam(StringRef Key, unsigned Value) = 0;
+
+ virtual std::unique_ptr getStatistics() const = 0;
NagyDonat wrote:
> It's not used in this PR, and I find it difficult to add test for.
Ok, that's completely fine, it doesn't cause any problems here.
https://github.com/llvm/llvm-project/pull/95128
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
@@ -434,6 +447,12 @@ class SMTSolver {
virtual bool isFPSupported() = 0;
virtual void print(raw_ostream ) const = 0;
+
+ /// Sets the requested option.
+ virtual void setBoolParam(StringRef Key, bool Value) = 0;
+ virtual void setUnsignedParam(StringRef Key, unsigned Value) = 0;
+
+ virtual std::unique_ptr getStatistics() const = 0;
NagyDonat wrote:
Is this method called anywhere?
https://github.com/llvm/llvm-project/pull/95128
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/95128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer][NFC] Reorganize Z3 report refutation (PR #95128)
https://github.com/NagyDonat approved this pull request. LGTM. I didn't deeply analyze all the small details of the commit, but it is clearly NFC (builds better infrastructure for follow-up commits), the implementation is clear and elegant (slightly better quality than what I can write) and the tests demonstrate that it's working. IMO feel free to merge this and continue with the next commit. https://github.com/llvm/llvm-project/pull/95128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Finish moving alpha.core.SizeofPtr to clang-tidy (PR #95118)
https://github.com/NagyDonat closed https://github.com/llvm/llvm-project/pull/95118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Finish moving alpha.core.SizeofPtr to clang-tidy (PR #95118)
NagyDonat wrote: > We should remember to mention this transfer explicitly in the release notes > one day. Yes, I'll try to remember it. By the way, it is already mentioned in the clang-tidy release notes (apparently there the release notes are kept in sync with active development), but it's definitely important to mention this on the analyzer side as well. (This was just an alpha checker, so I suppose that there weren't too many users, but still.) https://github.com/llvm/llvm-project/pull/95118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: Minor correction: in the commit message I accidentally wrote `alpha.core.SizeofPointer` instead of `alpha.core.SizeofPtr`. (Fortunately this difference is mostly academical, because my followup commit will remove that checker.) https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Finish moving alpha.core.SizeofPtr to clang-tidy (PR #95118)
https://github.com/NagyDonat created
https://github.com/llvm/llvm-project/pull/95118
The checker `alpha.core.SizeofPtr` was a very simple checker that did not rely
on path sensitive analysis and was very similar to the (more complex and
refined) clang-tidy check `bugprone-sizeof-expression`.
As there is no reason to maintain two separate implementations for the same
goal (and clang-tidy is more lightweight and accessible than the Analyzer) I
decided to move this functionality from the Static Analyzer to clang-tidy.
Recently my commit 546c816a529835a4cf89deecff957ea336a94fa2 reimplemented the
advantageous parts of `alpha.core.SizeofPtr` within clang-tidy; now this commit
finishes the transfer by deleting `alpha.core.SizeofPtr`.
From a168ee43267c83c2f59e3d8e8db746ecf62eb2ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Tue, 11 Jun 2024 15:42:27 +0200
Subject: [PATCH] [analyzer] Finish moving alpha.core.SizeofPtr to clang-tidy
The checker `alpha.core.SizeofPtr` was a very simple checker that did
not rely on path sensitive analysis and was very similar to the (more
complex and refined) clang-tidy check `bugprone-sizeof-expression`.
As there is no reason to maintain two separate implementations for the
same goal (and clang-tidy is more lightweight and accessible than the
Analyzer) I decided to move this functionality from the Static Analyzer
to clang-tidy.
Recently my commit 546c816a529835a4cf89deecff957ea336a94fa2
reimplemented the advantageous parts of `alpha.core.SizeofPtr` within
clang-tidy; now this commit finishes the transfer by deleting
`alpha.core.SizeofPtr`.
---
clang/docs/analyzer/checkers.rst | 15 ---
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../StaticAnalyzer/Checkers/CMakeLists.txt| 1 -
.../Checkers/CheckSizeofPointer.cpp | 96 ---
clang/test/Analysis/sizeofpointer.c | 8 --
clang/www/analyzer/alpha_checks.html | 16
6 files changed, 140 deletions(-)
delete mode 100644 clang/lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
delete mode 100644 clang/test/Analysis/sizeofpointer.c
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index f53dd545df5a9..d76ee241da797 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2452,21 +2452,6 @@ Check for pointer subtractions on two pointers pointing
to different memory chun
int d = - // warn
}
-.. _alpha-core-SizeofPtr:
-
-alpha.core.SizeofPtr (C)
-
-Warn about unintended use of ``sizeof()`` on pointer expressions.
-
-.. code-block:: c
-
- struct s {};
-
- int test(struct s *p) {
- return sizeof(p);
- // warn: sizeof(ptr) can produce an unexpected result
- }
-
.. _alpha-core-StackAddressAsyncEscape:
alpha.core.StackAddressAsyncEscape (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index a7f62ef7f2d07..429c334a0b24b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -296,10 +296,6 @@ def PointerSubChecker : Checker<"PointerSub">,
"different memory chunks">,
Documentation;
-def SizeofPointerChecker : Checker<"SizeofPtr">,
- HelpText<"Warn about unintended use of sizeof() on pointer expressions">,
- Documentation;
-
def TestAfterDivZeroChecker : Checker<"TestAfterDivZero">,
HelpText<"Check for division by variable that is later compared against 0. "
"Either the comparison is useless or there is division by zero.">,
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 68e829cace495..682cfa01bec96 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -24,7 +24,6 @@ add_clang_library(clangStaticAnalyzerCheckers
CheckObjCInstMethSignature.cpp
CheckPlacementNew.cpp
CheckSecuritySyntaxOnly.cpp
- CheckSizeofPointer.cpp
CheckerDocumentation.cpp
ChrootChecker.cpp
CloneChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
b/clang/lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
deleted file mode 100644
index 0d2551f11583e..0
--- a/clang/lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
+++ /dev/null
@@ -1,96 +0,0 @@
-//==- CheckSizeofPointer.cpp - Check for sizeof on pointers --*- C++
-*-==//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-//
-// This file defines a check for unintended use of sizeof() on pointer
-// expressions.
-//
[clang-tools-extra] [clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat closed https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Improve sizeof(pointer) handling in bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Remove a redundant check in Mangle. NFC (PR #95071)
https://github.com/NagyDonat approved this pull request. https://github.com/llvm/llvm-project/pull/95071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: I evaluated the effects of the additional change [Generalize the suppression to all sizeof(array[0]) expressions](https://github.com/llvm/llvm-project/pull/94356/commits/ce3a37610228ceb9a9e60575f87886bf8e183493) compared to the earlier version of this PR, and it seems that this tweak eliminates a large amount of false positives: | Project | New Reports | Resolved Reports | |-|-|--| | memcached | No reports | No reports | tmux | No reports | No reports | curl | No reports | [3 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_previous_sizeofexpression_after_most_changes=curl_curl-7_66_0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | twin | No reports | No reports | vim | No reports | No reports | openssl | No reports | [8 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_previous_sizeofexpression_after_most_changes=openssl_openssl-3.0.0-alpha7_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | sqlite | No reports | [12 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_previous_sizeofexpression_after_most_changes=sqlite_version-3.33.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | ffmpeg | No reports | [9 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_previous_sizeofexpression_after_most_changes=ffmpeg_n4.3.1_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | postgres | No reports | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_previous_sizeofexpression_after_most_changes=postgres_REL_13_0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | tinyxml2 | No reports | No reports | libwebm | No reports | No reports | xerces | No reports | [10 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_previous_sizeofexpression_after_most_changes=xerces_v3.2.3_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | bitcoin | No reports | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_previous_sizeofexpression_after_most_changes=bitcoin_v0.20.1_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | protobuf | No reports | [4 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=protobuf_v3.13.0_previous_sizeofexpression_after_most_changes=protobuf_v3.13.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | qtbase | No reports | [7 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=qtbase_v6.2.0_previous_sizeofexpression_after_most_changes=qtbase_v6.2.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) | contour | No reports | No reports | openrct2 | No reports | No reports | llvm-project | No reports | [6 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=llvm-project_llvmorg-12.0.0_previous_sizeofexpression_after_most_changes=llvm-project_llvmorg-12.0.0_new_sizeofexpression_with_full_arrayindexzero_suppression=Resolved) I randomly checked ~20% of the resolved reports, and they all were false positives (that is, situations where `sizeof(pointer)` is the idiomatic and correct solution), so I'm satisfied with this additional change. At this point I'm satisfied with the behavior of this check on the real-world code and I'm not planning to add more features within this PR. (There is one known issue which I highlighted in a FIXME in commit [Extend GenericFunctionTest, document a class of FPs with a FIXME](https://github.com/llvm/llvm-project/pull/94356/commits/59bd1b83e4fa89b371f4d1a96c51fc7a1b4ad170) -- but that's significantly less severe than the issues before this PR, so I think it's enough to handle that later.) I also answered all the review suggestions, so I'd be grateful for another round of reviews from @PiotrZSL @EugeneZelenko or anybody else who has time for it. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Replace X && isa(X) with isa_and_nonnull(X). NFC (PR #94987)
@@ -8510,7 +8510,8 @@ class MappableExprsHandler {
assert(VDecl == VD && "We got information for the wrong
declaration??");
assert(!Components.empty() &&
"Not expecting declaration with no component lists.");
-if (VD && E && VD->getType()->isAnyPointerType() &&
isa(E))
+if (VD && VD->getType()->isAnyPointerType() &&
+isa_and_nonnull(E))
HasMapBasePtr = true;
if (VD && E && VD->getType()->isAnyPointerType() &&
(isa(E) || isa(E)))
NagyDonat wrote:
Consider preserving the analogy between this and the next `if` with two
analogous `isa<>()` calls.
https://github.com/llvm/llvm-project/pull/94987
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Replace X && isa(X) with isa_and_nonnull(X). NFC (PR #94987)
@@ -2772,7 +2772,7 @@ fixVariable(const VarDecl *VD, FixitStrategy::Kind K, // also covers call-operator of lamdas isa(FD) || // skip when the function body is a try-block -(FD->hasBody() && isa(FD->getBody())) || +isa_and_nonnull(FD->getBody()) || NagyDonat wrote: The comment before the definition of `getBody()` says that ``` /// NOTE: For checking if there is a body, use hasBody() instead, to avoid /// unnecessary AST de-serialization of the body. ``` so perhaps it would be better to keep `hasBody()`. (Although I don't know whether performance is relevant at this point -- if we're handling an unusual case, then the runtime doesn't matter.) https://github.com/llvm/llvm-project/pull/94987 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Replace X && isa(X) with isa_and_nonnull(X). NFC (PR #94987)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94987 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Replace X && isa(X) with isa_and_nonnull(X). NFC (PR #94987)
https://github.com/NagyDonat approved this pull request. LGTM as well, with two optional remarks. https://github.com/llvm/llvm-project/pull/94987 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Replace X && isa(X) with isa_and_nonnull(X). NFC (PR #94987)
@@ -302,7 +302,7 @@ void MangleContext::mangleBlock(const DeclContext *DC, const BlockDecl *BD, assert((isa(DC) || isa(DC)) && "expected a NamedDecl or BlockDecl"); if (isa(DC)) - for (; DC && isa(DC); DC = DC->getParent()) + for (; isa_and_nonnull(DC); DC = DC->getParent()) (void) getBlockId(cast(DC), true); NagyDonat wrote: Could you remove the line `if (isa(DC))` as well? It's completely redundant with the loop condition. Although I see that this is a big automatic change, so perhaps it should be left for a separate change. https://github.com/llvm/llvm-project/pull/94987 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Add an ownership change visitor to StreamChecker (PR #94957)
@@ -168,8 +193,9 @@ REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef,
StreamState)
namespace {
class StreamChecker;
-using FnCheck = std::function;
+using FnCheckTy = void(const StreamChecker *, const FnDescription *,
+ const CallEvent &, CheckerContext &);
+using FnCheck = std::function;
NagyDonat wrote:
What is the goal of this change?
https://github.com/llvm/llvm-project/pull/94957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Add an ownership change visitor to StreamChecker (PR #94957)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94957 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Add an ownership change visitor to StreamChecker (PR #94957)
https://github.com/NagyDonat approved this pull request. Nice improvement, LGTM overall. I only have one phrasing nitpick and one very minor question about a tangential change. https://github.com/llvm/llvm-project/pull/94957 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Add an ownership change visitor to StreamChecker (PR #94957)
@@ -696,6 +732,69 @@ struct StreamOperationEvaluator {
} // end anonymous namespace
+//===--===//
+// Definition of NoStreamStateChangeVisitor.
+//===--===//
+
+namespace {
+class NoStreamStateChangeVisitor final : public NoOwnershipChangeVisitor {
+protected:
+ /// Syntactically checks whether the callee is a freeing function. Since
+ /// we have no path-sensitive information on this call (we would need a
+ /// CallEvent instead of a CallExpr for that), its possible that a
+ /// freeing function was called indirectly through a function pointer,
+ /// but we are not able to tell, so this is a best effort analysis.
+ bool isFreeingCallAsWritten(const CallExpr ) const {
+const auto *StreamChk = static_cast();
+if (StreamChk->FCloseDesc.matchesAsWritten(Call))
+ return true;
+
+return false;
+ }
+
+ bool doesFnIntendToHandleOwnership(const Decl *Callee,
+ ASTContext ) override {
+using namespace clang::ast_matchers;
+const FunctionDecl *FD = dyn_cast(Callee);
+
+auto Matches =
+match(findAll(callExpr().bind("call")), *FD->getBody(), ACtx);
+for (BoundNodes Match : Matches) {
+ if (const auto *Call = Match.getNodeAs("call"))
+if (isFreeingCallAsWritten(*Call))
+ return true;
+}
+// TODO: Ownership might change with an attempt to store stream object, not
+// only through freeing it. Check for attempted stores as well.
+return false;
+ }
+
+ virtual bool hasResourceStateChanged(ProgramStateRef CallEnterState,
+ ProgramStateRef CallExitEndState) final
{
+return CallEnterState->get(Sym) !=
+ CallExitEndState->get(Sym);
+ }
+
+ PathDiagnosticPieceRef emitNote(const ExplodedNode *N) override {
+PathDiagnosticLocation L = PathDiagnosticLocation::create(
+N->getLocation(),
+N->getState()->getStateManager().getContext().getSourceManager());
+return std::make_shared(
+L, "Returning without freeing stream object or storing the pointer for
"
NagyDonat wrote:
```suggestion
L, "Returning without freeing stream object or storing it for "
```
The current phrasing is technically correct, because the stream object is a
`FILE *` _pointer_, but IMO "it" would be both shorter and easier to understand
https://github.com/llvm/llvm-project/pull/94957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/6] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/5] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: I re-ran the open source evaluation, and here is the clean diff that I promised (italicized notes are just copied from the old table): | Project | New Reports | Resolved Reports | Notes |-|-|--|--| | memcached | No reports | No reports | – | tmux | No reports | [23 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_old_sizeofexpressions_with_new_messages=tmux_2.6_new_sizeofexpressions_rerun=Resolved) | _reports seem to be FPs, including several ones that [use `qsort` in a clear and straightforward way](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_old_sizeofexpressions=tmux_2.6_new_sizeofexpressions=Resolved=5493278=e1dd82bffcf68169ff8fe7181ca44f16=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Ftmux%2Fwindow-buffer.c)_ | curl | [3 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions_with_new_messages=curl_curl-7_66_0_new_sizeofexpressions_rerun=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions_with_new_messages=curl_curl-7_66_0_new_sizeofexpressions_rerun=Resolved) | _new reports are TPs (all reporting incorrect use of the same data structure), resolved one is FP_ | twin | No reports | No reports | – | vim | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_old_sizeofexpressions_with_new_messages=vim_v8.2.1920_new_sizeofexpressions_rerun=New) | No reports | _true positive_ | openssl | [23 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions_rerun_openssl_ffmpeg=New) | [22 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions_rerun_openssl_ffmpeg=Resolved) | resolved reports are FPs, new reports are mostly TPs or "works, but ugly and dodgy" code with a few FPs that look like `generic_function(, sizeof(arg))` or `get_memory(length*sizeof(array[0]))` | sqlite | [11 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions_with_new_messages=sqlite_version-3.33.0_new_sizeofexpressions_rerun=New) | No reports | _among the new results there are many FPs ([(1)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493379=f411835e93b1711c2889d4bef2889db9=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fshell.c), [(2)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493385=d9e3d0a984913130c821b7c18c2cc8d2=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fsqlite3.c)) that do things like `char **mem; realloc(mem, numElements*sizeof(mem[0]))`_ | ffmpeg | [22 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=ffmpeg_n4.3.1_new_sizeofexpressions_rerun_openssl_ffmpeg=New) | [109 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions_with_new_messages_openssl_ffmpeg=ffmpeg_n4.3.1_new_sizeofexpressions_rerun_openssl_ffmpeg=Resolved) | postgres | No reports | [5 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions_with_new_messages=postgres_REL_13_0_new_sizeofexpressions_rerun=Resolved) | _resolved reports are FPs_ | tinyxml2 | No reports | No reports | – | libwebm | No reports | No reports | – | xerces | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=xerces_v3.2.3_old_sizeofexpressions_with_new_messages=xerces_v3.2.3_new_sizeofexpressions_rerun=New) | No reports | true positive, seems to be an ugly bug | bitcoin | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=bitcoin_v0.20.1_old_sizeofexpressions_with_new_messages=bitcoin_v0.20.1_new_sizeofexpressions_rerun=New) | No reports | false positive `hasher.Write((const unsigned char*), sizeof(ptr));` | protobuf | [5 new
[clang] [analyzer][NFC] Factor out NoOwnershipChangeVisitor (PR #94357)
https://github.com/NagyDonat approved this pull request. https://github.com/llvm/llvm-project/pull/94357 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -190,6 +190,15 @@ Options .. option:: WarnOnSizeOfPointerToAggregate - When `true`, the check will warn on an expression like - ``sizeof(expr)`` where the expression is a pointer - to aggregate. Default is `true`. + When `true`, the check will warn when the argument of ``sizeof`` is either a + pointer-to-aggregate type, an expression returning a pointer-to-aggregate + value or an expression that returns a pointer from an array-to-pointer + conversion (that may be implicit or explicit, for example ``array + 2`` or + ``(int *)array``). Default is `true`. NagyDonat wrote: I also added a proper description for `WarnOnSizeOfPointerToAggregate`, because the old one was very vague and didn't explain the (somewhat complex) effects that are handled here. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/3] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat updated
https://github.com/llvm/llvm-project/pull/94356
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH 1/2] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
index 5e64d23874ec1..05ef666d4f01e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -67,7 +67,8 @@ SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
WarnOnSizeOfCompareToConstant(
Options.get("WarnOnSizeOfCompareToConstant", true)),
WarnOnSizeOfPointerToAggregate(
- Options.get("WarnOnSizeOfPointerToAggregate", true)) {}
+ Options.get("WarnOnSizeOfPointerToAggregate", true)),
+ WarnOnSizeOfPointer(Options.get("WarnOnSizeOfPointer", false)) {}
void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
@@ -78,6 +79,7 @@ void
SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap ) {
WarnOnSizeOfCompareToConstant);
Options.store(Opts, "WarnOnSizeOfPointerToAggregate",
WarnOnSizeOfPointerToAggregate);
+ Options.store(Opts, "WarnOnSizeOfPointer", WarnOnSizeOfPointer);
}
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -127,17 +129,30 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder
*Finder) {
const auto ConstStrLiteralDecl =
varDecl(isDefinition(), hasType(hasCanonicalType(CharPtrType)),
hasInitializer(ignoringParenImpCasts(stringLiteral(;
+ const auto VarWithConstStrLiteralDecl = expr(
+ hasType(hasCanonicalType(CharPtrType)),
+ ignoringParenImpCasts(declRefExpr(hasDeclaration(ConstStrLiteralDecl;
Finder->addMatcher(
- sizeOfExpr(has(ignoringParenImpCasts(
- expr(hasType(hasCanonicalType(CharPtrType)),
- ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ConstStrLiteralDecl)))
+ sizeOfExpr(has(ignoringParenImpCasts(VarWithConstStrLiteralDecl)))
.bind("sizeof-charp"),
this);
- // Detect sizeof(ptr)
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -292,11 +318,17 @@ void SizeofExpressionCheck::check(const
MatchFinder::MatchResult ) {
diag(E->getBeginLoc(),
"suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?")
<< E->getSourceRange();
- } else if (const auto *E =
- Result.Nodes.getNodeAs("sizeof-pointer-to-aggregate")) {
-diag(E->getBeginLoc(),
- "suspicious usage of 'sizeof(A*)'; pointer to aggregate")
-<< E->getSourceRange();
+ } else if (const auto *E = Result.Nodes.getNodeAs("sizeof-pointer")) {
+if (Result.Nodes.getNodeAs("struct-type")) {
+ diag(E->getBeginLoc(),
+ "suspicious usage of 'sizeof(A*)' on pointer-to-aggregate type; did
"
+ "you mean 'sizeof(A)'?")
+ << E->getSourceRange();
+} else {
+ diag(E->getBeginLoc(), "suspicious usage of 'sizeof()' on an expression "
+ "that results in a pointer")
NagyDonat wrote:
I used this phrasing to be consistent with the analogous message "suspicious
usage of 'sizeof()' on an expression that results in an integer". I would also
prefer "suspicious use of 'sizeof()' on an expression of pointer type", but
then it would be good to update the message for integers, and then it would be
good to update all the other awkward messages printed by this check...
I'd prefer to keep the current consistent and (IMO) barely acceptable message
in this commit; and leave a general cleanup of all the messages of this checker
for a separate followup commit. (I'm not very enthusiastic for doing this
cleanup, but I can do it if the community feels that it would be important.)
https://github.com/llvm/llvm-project/pull/94356
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Factor out NoOwnershipChangeVisitor (PR #94357)
https://github.com/NagyDonat approved this pull request. Perfectly normal NFC change, feel free to merge it (assuming that the tests pass). https://github.com/llvm/llvm-project/pull/94357 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: I analyzed several open source project with this check to observe the effects of my commit and I was (pleasantly?) surprised to see that it detected some ugly errors (despite the fact that the inputs are stable open source projects...). The following table compares the "old" state before this commit and the "new" state where this commit is active and the freshly added option `WarnOnSizeOfPointer` is set to true (instead of the default "false"): | Project | New Reports | Resolved Reports | Notes | |-|-|--|---| | memcached | No reports | No reports | – | tmux | No reports | [23 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_2.6_old_sizeofexpressions=tmux_2.6_new_sizeofexpressions=Resolved) | reports seem to be FPs, including several ones that [use `qsort` in a clear and straightforward way](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_old_sizeofexpressions=tmux_2.6_new_sizeofexpressions=Resolved=5493278=e1dd82bffcf68169ff8fe7181ca44f16=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Ftmux%2Fwindow-buffer.c) | curl | [3 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions=curl_curl-7_66_0_new_sizeofexpressions=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=curl_curl-7_66_0_old_sizeofexpressions=curl_curl-7_66_0_new_sizeofexpressions=Resolved) | new reports are TPs (all reporting incorrect use of the same data structure), resolved one is FP | twin | No reports | No reports | – | vim | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=vim_v8.2.1920_old_sizeofexpressions=vim_v8.2.1920_new_sizeofexpressions=New) | No reports | true positive | openssl | [33 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions=New) | [32 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=openssl_openssl-3.0.0-alpha7_old_sizeofexpressions=openssl_openssl-3.0.0-alpha7_new_sizeofexpressions=Resolved) | ... | sqlite | [19 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New) | [8 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=Resolved) | among the new results there are many FPs ([(1)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493379=f411835e93b1711c2889d4bef2889db9=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fshell.c), [(2)](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=sqlite_version-3.33.0_old_sizeofexpressions=sqlite_version-3.33.0_new_sizeofexpressions=New=5493385=d9e3d0a984913130c821b7c18c2cc8d2=%2Flocal%2Fpersistent_docker%2FCSA-measurements-driver-2894%2Fmeasurements_workspace%2Fsqlite%2Fsqlite3.c)) that do things like `char **mem; realloc(mem, numElements*sizeof(mem[0]))`; the resolved reports are FPs and they reappear among the new reports with a different message | ffmpeg | [31 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions=ffmpeg_n4.3.1_new_sizeofexpressions=New) | [118 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=ffmpeg_n4.3.1_old_sizeofexpressions=ffmpeg_n4.3.1_new_sizeofexpressions=Resolved) | ... | postgres | [2 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions=postgres_REL_13_0_new_sizeofexpressions=New) | [7 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=postgres_REL_13_0_old_sizeofexpressions=postgres_REL_13_0_new_sizeofexpressions=Resolved) | resolved reports are mostly FPs, two of them (one FP + one "technically works, but stupid" code) reappear as "new" reports with the new message | tinyxml2 | [1 new reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_old_sizeofexpressions=tinyxml2_8.0.0_new_sizeofexpressions=New) | [1 resolved reports](https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tinyxml2_8.0.0_old_sizeofexpressions=tinyxml2_8.0.0_new_sizeofexpressions=Resolved) | same FP is reported with the new message | libwebm | No reports | No reports | – | xerces | [16 new
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
NagyDonat wrote: @EugeneZelenko Thanks for inviting some additional reviewers. I'll add an entry in the release notes. https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/94356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
@@ -124,8 +124,6 @@ int Test1(const char* ptr) {
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious usage of sizeof
pointer 'sizeof(P*)/sizeof(Q*)'
sum += sizeof(ptr) / sizeof(char);
// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious usage of sizeof
pointer 'sizeof(T*)/sizeof(T)'
- sum += sizeof(ptr) / sizeof(ptr[0]);
- // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: suspicious usage of sizeof
pointer 'sizeof(T*)/sizeof(T)'
NagyDonat wrote:
I'm deleting this test line, because the same expression also appears on line
117.
https://github.com/llvm/llvm-project/pull/94356
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add WarnOnSizeOfPointer mode to bugprone-sizeof-expression (PR #94356)
https://github.com/NagyDonat created
https://github.com/llvm/llvm-project/pull/94356
This commit reimplements the functionality of the Clang Static Analyzer checker
`alpha.core.SizeofPointer` within clang-tidy by adding a new (off-by-default)
option to bugprone-sizeof-expression which activates reporting all the
`sizeof(ptr)` expressions (where ptr is an expression that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer` was an
AST-based checker, which did not rely on the path sensitive capabilities of the
Static Analyzer, so there was no reason to keep it in the Static Analyzer
instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression, because
that check already provided several heuristics that reported various especially
suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't surprise the
existing users; but it can provide a more through coverage for the
vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than the existing
partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is a
pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped to avoid
several false positives on open-source code.
This commit also replaces the old message "suspicious usage of 'sizeof(A*)';
pointer to aggregate" with two more concrete messages; but I feel that this
tidy check would deserve a through cleanup of all the diagnostic messages that
it can produce. (I added a FIXME to mark one outright misleading message.)
From c94feff726b48e7e3b5a46d5028cc5a6d0ac9beb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 22 May 2024 11:57:50 +0200
Subject: [PATCH] [clang-tidy] Add WarnOnSizeOfPointer mode to
bugprone-sizeof-expression
This commit reimplements the functionality of the Clang Static Analyzer
checker `alpha.core.SizeofPointer` within clang-tidy by adding a new
(off-by-default) option to bugprone-sizeof-expression which activates
reporting all the `sizeof(ptr)` expressions (where ptr is an expression
that produces a pointer).
The main motivation for this change is that `alpha.core.SizeofPointer`
was an AST-based checker, which did not rely on the path sensitive
capabilities of the Static Analyzer, so there was no reason to keep it
in the Static Analyzer instead of the more lightweight clang-tidy.
After this commit I'm planning to create a separate commit that deletes
`alpha.core.SizeofPointer` from Clang Static Analyzer.
It was natural to place this moved logic in bugprone-sizeof-expression,
because that check already provided several heuristics that reported
various especially suspicious classes of `sizeof(ptr)` expressions.
The new mode `WarnOnSizeOfPointer` is off-by-default, so it won't
surprise the existing users; but it can provide a more through coverage
for the vulnerability CWE-467 ("Use of sizeof() on a Pointer Type") than
the existing partial heuristics.
I preserved the exception that the RHS of an expression that looks like
`sizeof(array) / sizeof(array[0])` is not reported; and I added another
exception which ensures that `sizeof(*pp)` is not reported when `pp` is
a pointer-to-pointer expression.
This second exception (which I also apply in the "old" on-by-default mode
`WarnOnSizeOfPointerToAggregate`) was present in the CSA checker
`alpha.core.SizeofPoionter` and I decided to copy it because it helped
to avoid several false positives on open-source code.
This commit also replaces the old message "suspicious usage of
'sizeof(A*)'; pointer to aggregate" with two more concrete messages; but
I feel that this tidy check would deserve a through cleanup of all the
diagnostic messages that it can produce. (I added a FIXME to mark one
outright misleading message.)
---
.../bugprone/SizeofExpressionCheck.cpp| 95 ---
.../bugprone/SizeofExpressionCheck.h | 1 +
.../checks/bugprone/sizeof-expression.rst | 5 +
.../checkers/bugprone/sizeof-expression-2.c | 12 +-
.../sizeof-expression-any-pointer.cpp | 231 ++
.../checkers/bugprone/sizeof-expression.cpp | 63 +++--
6 files changed, 351 insertions(+), 56 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-any-pointer.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
[clang] [analyzer] New optin.taint.TaintAlloc checker for catching unbounded memory allocation calls (PR #92420)
https://github.com/NagyDonat approved this pull request. The change LGTM. I agree with @steakhal that `TaintedAlloc` would be a slightly better name, but the current one is also acceptable. https://github.com/llvm/llvm-project/pull/92420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improved PointerSubChecker (PR #93676)
NagyDonat wrote:
> With the current version I have the following observations:
>
> * There is a warning for `( + 1) - ` and `( - 1) - `. Should this
> be fixed?
The expression `( + 1) - ` is valid and should not produce a warning. It
could appear e.g. in code that's structured like
```
void log_observations(double *begin, double *end, /*...*/) {
log_observation_count(end - begin);
// also log the actual observations
}
void do_observations(/*...*/) {
double array[1024], *p = array;
//...
while (can_observe()) {
*p++ = /* ... */
}
log_observations(array, p);
}
void do_single_observation(/*...*/) {
if (!can_observe())
return;
double result = /* ... */
// ...
log_observations(, + 1);
}
```
On the other hand `( - 1) - ` is not standard-compliant, because the
standard guarantees an "after-the-end" pointer (which is valid in calculations
but must not be dereferenced) but it doesn't accept a "before-the-begin"
pointer.
> * The code `(int *)((char *)([4]) + sizeof(int)) - [4]` produces no
> warning but `(int *)((char *)([4]) + 1) - [4]` produces warning.
That's very nice, even a slightly less accurate solution would be acceptable.
> For 2-dimensional arrays there is warning for all of these cases (lines 44-47
> in the test file). Is this possible to fix (to get warning in all cases), or
> no warning is needed here?
I'd say that the current behavior (warning on all of 44-47) is OK here -- this
is very unusual trickery and deserves a highlight. However I could also accept
a situation where there was no warning for these complex multidimensional cases.
https://github.com/llvm/llvm-project/pull/93676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improved PointerSubChecker (PR #93676)
@@ -0,0 +1,74 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.PointerSub -verify %s
+
+void f1(void) {
+ int x, y, z[10];
+ int d = - // expected-warning{{Subtraction of two pointers that do
not point into the same array is undefined behavior}}
+ d = z - // expected-warning{{Subtraction of two pointers that do not
point into the same array is undefined behavior}}
+ d = - // expected-warning{{Subtraction of two pointers that do not
point into the same array is undefined behavior}}
+ d = (long*) - (long*)
+}
+
+void f2(void) {
+ int a[10], b[10], c;
+ int *p = [2];
+ int *q = [8];
+ int d = q - p; // no-warning
+
+ q = [3];
+ d = q - p; // expected-warning{{Subtraction of two pointers that}}
+
+ q = a + 10;
+ d = q - p; // no warning (use of pointer to one after the end is allowed)
+ d = [4] - a; // no warning
+
+ q = a + 11;
+ d = q - a; // ?
+
+ d = - p; // expected-warning{{Subtraction of two pointers that}}
+}
+
+void f3(void) {
+ int a[3][4];
+ int d;
+
+ d = &(a[2]) - &(a[1]);
+ d = a[2] - a[1]; // expected-warning{{Subtraction of two pointers that}}
+ d = a[1] - a[1];
+ d = &(a[1][2]) - &(a[1][0]);
+ d = &(a[1][2]) - &(a[0][0]); // expected-warning{{Subtraction of two
pointers that}}
+}
+
+void f4(void) {
+ int n = 4, m = 3;
+ int a[n][m];
+ int (*p)[m] = a; // p == [0]
+ p += 1; // p == [1]
+ int d = p - a; // d == 1 // expected-warning{{subtraction of pointers to
type 'int[m]' of zero size has undefined behavior}}
+
+ d = &(a[2]) - &(a[1]); // expected-warning{{subtraction of pointers to type
'int[m]' of zero size has undefined behavior}}
+ d = a[2] - a[1]; // expected-warning{{Subtraction of two pointers that}}
NagyDonat wrote:
Could you add a FIXME comment which states that these "type 'int[m]' of zero
size" warnings are false positives emitted by checker X (and they should be
eliminated eventually)?
https://github.com/llvm/llvm-project/pull/93676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Fix comparison to True/False (PR #94038)
https://github.com/NagyDonat commented: This change follows both the letter and the spirit of PEP8. (Note that `True` and `False` are not "singletons like `None`" because the type `bool` has two possible values -- but different rules still claim that the original code was bad and the new code is good.) https://github.com/llvm/llvm-project/pull/94038 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer][NFC] Improve docs of alpha.unix.BlockInCriticalSection (PR #93812)
NagyDonat wrote: > My intention was to ease the review as much as possible by cutting them into > atomic parts. This is a good idea in general, but I feel that these two changes are so trivial, that they'd be very easy to review even together. Feel free to merge them both in whatever combination you would like. https://github.com/llvm/llvm-project/pull/93812 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer][NFC] Improve docs of alpha.unix.BlockInCriticalSection (PR #93812)
https://github.com/NagyDonat approved this pull request. What's the relationship between this PR and https://github.com/llvm/llvm-project/pull/93799 ? Otherwise, the change LGTM, but you might want to either unify these two NFC changes into a single commit or ensure that they're independent. https://github.com/llvm/llvm-project/pull/93812 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improved PointerSubChecker (PR #93676)
@@ -0,0 +1,74 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.PointerSub -verify %s
+
+void f1(void) {
+ int x, y, z[10];
+ int d = - // expected-warning{{Subtraction of two pointers that do
not point into the same array is undefined behavior}}
+ d = z - // expected-warning{{Subtraction of two pointers that do not
point into the same array is undefined behavior}}
+ d = - // expected-warning{{Subtraction of two pointers that do not
point into the same array is undefined behavior}}
NagyDonat wrote:
> Wrt. ` - 1`, that should be valid as well, given that the resulting pointer
> is not dereferenced.
The standard explicitly disallows this, see [[expr.add] part
4.2](http://eel.is/c++draft/expr.add#4.2) and 4.3 (in the most recent C++ draft
standard, other versions may be different).
https://github.com/llvm/llvm-project/pull/93676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Improved PointerSubChecker (PR #93676)
@@ -44,24 +44,30 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B, const MemRegion *LR = LV.getAsRegion(); const MemRegion *RR = RV.getAsRegion(); - - if (!(LR && RR)) -return; - - const MemRegion *BaseLR = LR->getBaseRegion(); - const MemRegion *BaseRR = RR->getBaseRegion(); - - if (BaseLR == BaseRR) + if (!LR || !RR) return; - // Allow arithmetic on different symbolic regions. - if (isa(BaseLR) || isa(BaseRR)) -return; + const auto *ElemLR = dyn_cast(LR); + const auto *ElemRR = dyn_cast(RR); + // FIXME: We want to verify that these are elements of an array. + // Because behavior of ElementRegion it may be confused with a cast. + // There is not a simple way to distinguish it from array element (check the + // types?). Because this missing check a warning is missing in the rare case + // when two casted pointers to the same region (variable) are subtracted. NagyDonat wrote: I discussed your examples and questions with @whisperity and Zoltán Porkoláb, and I'm trying to summarize what we determined. Note that I'm mostly working from the most recent C++ draft standard (http://eel.is/c++draft/), so some the conclusions may be invalid in C or older versions of C++ (but I tried to highlight the differences that I know about). (1) In your first example the step `int *b = a` is invalid because the `int[3][3]` array `a` decays to an `int (*)[3]` (pointer to array of 3 `int`s) and that type is not interconvertible with a plain `int *` (see [ [basic.compound] note 5](http://eel.is/c++draft/basic.compound#note-5) in the C++ draft standard). (And if you define `b` as `int (*b)[3] = a`, then the pointer subtraction will become invalid.) (2) In the second example using a `long *` to initialize `char *` or `short *` variables is _usually_ an error, it's accepted under old C, but in C++ you need an explicit cast and e.g. modern GCC also produces an error (`-Wincompatible-pointer-types` a warning that's by default an error) when it compiles this. I'd guess that the actual pointer arithmetic _is_ standard-compliant, but the relevant parts of the standard ([[expr.reinterpret.cast] 7](http://eel.is/c++draft/expr.post#expr.reinterpret.cast-7), [[expr.static.cast] 14](http://eel.is/c++draft/expr.post#expr.static.cast-14))are too vague to give a confident answer. Note that under C++ _accessing_ the element `short b[1]` would be a violation of [[basic.lval] part 11](http://eel.is/c++draft/basic.lval#def:type-accessible), but based on [[defns.access]](http://eel.is/c++draft/defns.access) I'd say that the expression `[1]` is _not_ an access of `b[1]`. (3) In your third example `[2][2] - [1][1]` is clearly working in practically all implementations, but we're fairly sure that it's not compliant with the C++ standard, because [ [expr.add] part 5.2](https://www.eel.is/c++draft/expr.add#5.2) speaks about "array elements _i_ and _j_ of the same array object _x_" (and later "_i_ - _j_" which shows that "_i_" and "_j_" are scalar numbers) -- while in your example `a[2][2]` and `a[1][1]` are not (numerically indexed) elements within the same array. (This is coherent with [[dcl.array]](http://eel.is/c++draft/dcl.array) where an "array" implicitly means a one-dimensional array structure (whose elements may be other arrays).) Nevertheless, it may be reasonable to avoid emitting warnings on this kind of code, because that could be better for the users. (4) The definition of "_what exactly an "array object" is_" and "what is an _element_ of an array" primarily appears at [[dcl.array] part 6](http://eel.is/c++draft/dcl.array#6): > An object of type “array of N U” consists of a contiguously allocated > non-empty set of N subobjects of type U, known as the _elements_ of the > array, and numbered 0 to N-1. (This clearly shows that the _element of an element of_ an array is not an _element of_ the array. The notion of "multi-dimensional arrays" only appears in the _Example_ [[dcl.array] Example 4](http://eel.is/c++draft/dcl.array#example-4) with word choices that suggests that this is just a mathematical/intuitive notion and not something that's well-defined in the standard.) This general definition is augmented by [[basic.compound] part 3, sentence 11](http://eel.is/c++draft/basic.compound#3.sentence-11) which states that: > For purposes of pointer arithmetic > ([[expr.add]](http://eel.is/c++draft/expr.add)) and comparison > ([[expr.rel]](http://eel.is/c++draft/expr.rel), > [[expr.eq]](http://eel.is/c++draft/expr.eq)), a pointer past the end of the > last element of an array x of n elements is considered to be equivalent to a > pointer to a hypothetical array element n of x and an object of type T that > is not an array element is considered to belong to an array with one element > of type T. (5) Yes, the sub-arrays of a multidimensional array are separate "array objects" that have their own
[clang] [clang][analyzer][NFC] Add test for a limitation of alpha.unix.Bloc… (PR #93799)
https://github.com/NagyDonat approved this pull request. It's good to document this, the commit LGTM. Are you planning to fix this soon? https://github.com/llvm/llvm-project/pull/93799 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
