[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
This revision was automatically updated to reflect the committed changes.
Closed by commit rL296326: [analyzer] clarify 'result is garbage value' when it
is out of bounds (authored by danielmarjamaki).
Changed prior to commit:
https://reviews.llvm.org/D28278?vs=89641=89854#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28278
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
cfe/trunk/test/Analysis/uninit-vals-ps.c
Index: cfe/trunk/test/Analysis/uninit-vals-ps.c
===
--- cfe/trunk/test/Analysis/uninit-vals-ps.c
+++ cfe/trunk/test/Analysis/uninit-vals-ps.c
@@ -57,6 +57,12 @@
return s.x; // no-warning
}
+void f6(int x) {
+ int a[20];
+ if (x == 25) {}
+ if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a
garbage value due to array index out of bounds}}
+}
+
int ret_uninit() {
int i;
int *p =
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -35,6 +35,30 @@
};
} // end anonymous namespace
+static bool isArrayIndexOutOfBounds(CheckerContext , const Expr *Ex) {
+ ProgramStateRef state = C.getState();
+ const LocationContext *LCtx = C.getLocationContext();
+
+ if (!isa(Ex))
+return false;
+
+ SVal Loc = state->getSVal(Ex, LCtx);
+ if (!Loc.isValid())
+return false;
+
+ const MemRegion *MR = Loc.castAs().getRegion();
+ const ElementRegion *ER = dyn_cast(MR);
+ if (!ER)
+return false;
+
+ DefinedOrUnknownSVal Idx = ER->getIndex().castAs();
+ DefinedOrUnknownSVal NumElements = C.getStoreManager().getSizeInElements(
+ state, ER->getSuperRegion(), ER->getValueType());
+ ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true);
+ ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false);
+ return StOutBound && !StInBound;
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext ) const {
ProgramStateRef state = C.getState();
@@ -77,6 +101,8 @@
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
<< "' is a garbage value";
+ if (isArrayIndexOutOfBounds(C, Ex))
+OS << " due to array index out of bounds";
}
else {
// Neither operand was undefined, but the result is undefined.
Index: cfe/trunk/test/Analysis/uninit-vals-ps.c
===
--- cfe/trunk/test/Analysis/uninit-vals-ps.c
+++ cfe/trunk/test/Analysis/uninit-vals-ps.c
@@ -57,6 +57,12 @@
return s.x; // no-warning
}
+void f6(int x) {
+ int a[20];
+ if (x == 25) {}
+ if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a garbage value due to array index out of bounds}}
+}
+
int ret_uninit() {
int i;
int *p =
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -35,6 +35,30 @@
};
} // end anonymous namespace
+static bool isArrayIndexOutOfBounds(CheckerContext , const Expr *Ex) {
+ ProgramStateRef state = C.getState();
+ const LocationContext *LCtx = C.getLocationContext();
+
+ if (!isa(Ex))
+return false;
+
+ SVal Loc = state->getSVal(Ex, LCtx);
+ if (!Loc.isValid())
+return false;
+
+ const MemRegion *MR = Loc.castAs().getRegion();
+ const ElementRegion *ER = dyn_cast(MR);
+ if (!ER)
+return false;
+
+ DefinedOrUnknownSVal Idx = ER->getIndex().castAs();
+ DefinedOrUnknownSVal NumElements = C.getStoreManager().getSizeInElements(
+ state, ER->getSuperRegion(), ER->getValueType());
+ ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true);
+ ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false);
+ return StOutBound && !StInBound;
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext ) const {
ProgramStateRef state = C.getState();
@@ -77,6 +101,8 @@
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
<< "' is a garbage value";
+ if (isArrayIndexOutOfBounds(C, Ex))
+OS << " due to array index out of bounds";
}
else {
// Neither operand was undefined, but the result is undefined.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thank you! Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
danielmarjamaki updated this revision to Diff 89641.
danielmarjamaki added a comment.
Fixed review comment. Broke out function.
Repository:
rL LLVM
https://reviews.llvm.org/D28278
Files:
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
test/Analysis/uninit-vals-ps.c
Index: test/Analysis/uninit-vals-ps.c
===
--- test/Analysis/uninit-vals-ps.c
+++ test/Analysis/uninit-vals-ps.c
@@ -57,6 +57,12 @@
return s.x; // no-warning
}
+void f6(int x) {
+ int a[20];
+ if (x == 25) {}
+ if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a
garbage value due to array index out of bounds}}
+}
+
int ret_uninit() {
int i;
int *p =
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -35,6 +35,30 @@
};
} // end anonymous namespace
+static bool isArrayIndexOutOfBounds(CheckerContext , const Expr *Ex) {
+ ProgramStateRef state = C.getState();
+ const LocationContext *LCtx = C.getLocationContext();
+
+ if (!isa(Ex))
+return false;
+
+ SVal Loc = state->getSVal(Ex, LCtx);
+ if (!Loc.isValid())
+return false;
+
+ const MemRegion *MR = Loc.castAs().getRegion();
+ const ElementRegion *ER = dyn_cast(MR);
+ if (!ER)
+return false;
+
+ DefinedOrUnknownSVal Idx = ER->getIndex().castAs();
+ DefinedOrUnknownSVal NumElements = C.getStoreManager().getSizeInElements(
+ state, ER->getSuperRegion(), ER->getValueType());
+ ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true);
+ ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false);
+ return StOutBound && !StInBound;
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext ) const {
ProgramStateRef state = C.getState();
@@ -77,6 +101,8 @@
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
<< "' is a garbage value";
+ if (isArrayIndexOutOfBounds(C, Ex))
+OS << " due to array index out of bounds";
}
else {
// Neither operand was undefined, but the result is undefined.
Index: test/Analysis/uninit-vals-ps.c
===
--- test/Analysis/uninit-vals-ps.c
+++ test/Analysis/uninit-vals-ps.c
@@ -57,6 +57,12 @@
return s.x; // no-warning
}
+void f6(int x) {
+ int a[20];
+ if (x == 25) {}
+ if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a garbage value due to array index out of bounds}}
+}
+
int ret_uninit() {
int i;
int *p =
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -35,6 +35,30 @@
};
} // end anonymous namespace
+static bool isArrayIndexOutOfBounds(CheckerContext , const Expr *Ex) {
+ ProgramStateRef state = C.getState();
+ const LocationContext *LCtx = C.getLocationContext();
+
+ if (!isa(Ex))
+return false;
+
+ SVal Loc = state->getSVal(Ex, LCtx);
+ if (!Loc.isValid())
+return false;
+
+ const MemRegion *MR = Loc.castAs().getRegion();
+ const ElementRegion *ER = dyn_cast(MR);
+ if (!ER)
+return false;
+
+ DefinedOrUnknownSVal Idx = ER->getIndex().castAs();
+ DefinedOrUnknownSVal NumElements = C.getStoreManager().getSizeInElements(
+ state, ER->getSuperRegion(), ER->getValueType());
+ ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true);
+ ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false);
+ return StOutBound && !StInBound;
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext ) const {
ProgramStateRef state = C.getState();
@@ -77,6 +101,8 @@
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
<< "' is a garbage value";
+ if (isArrayIndexOutOfBounds(C, Ex))
+OS << " due to array index out of bounds";
}
else {
// Neither operand was undefined, but the result is undefined.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:76
if (Ex) {
+ bool ArrayIndexOutOfBounds = false;
+ if (isa(Ex)) {
Please, pull this out into a sub-rutine. Thanks!
https://reviews.llvm.org/D28278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
danielmarjamaki updated this revision to Diff 89540.
danielmarjamaki added a comment.
Making the error message more precise.
https://reviews.llvm.org/D28278
Files:
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
test/Analysis/uninit-vals-ps.c
Index: test/Analysis/uninit-vals-ps.c
===
--- test/Analysis/uninit-vals-ps.c
+++ test/Analysis/uninit-vals-ps.c
@@ -57,6 +57,12 @@
return s.x; // no-warning
}
+void f6(int x) {
+ int a[20];
+ if (x == 25) {}
+ if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a
garbage value due to array index out of bounds}}
+}
+
int ret_uninit() {
int i;
int *p =
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -73,10 +73,31 @@
}
if (Ex) {
+ bool ArrayIndexOutOfBounds = false;
+ if (isa(Ex)) {
+SVal Loc = state->getSVal(Ex,LCtx);
+if (Loc.isValid()) {
+ const MemRegion *MR = Loc.castAs().getRegion();
+ if (const ElementRegion *ER = dyn_cast(MR)) {
+DefinedOrUnknownSVal Idx =
ER->getIndex().castAs();
+DefinedOrUnknownSVal NumElements
+ = C.getStoreManager().getSizeInElements(state,
ER->getSuperRegion(),
+ER->getValueType());
+ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements,
true);
+ProgramStateRef StOutBound = state->assumeInBound(Idx,
NumElements, false);
+if (StOutBound && !StInBound) {
+ ArrayIndexOutOfBounds = true;
+}
+ }
+}
+ }
+
OS << "The " << (isLeft ? "left" : "right")
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
<< "' is a garbage value";
+ if (ArrayIndexOutOfBounds)
+OS << " due to array index out of bounds";
}
else {
// Neither operand was undefined, but the result is undefined.
Index: test/Analysis/uninit-vals-ps.c
===
--- test/Analysis/uninit-vals-ps.c
+++ test/Analysis/uninit-vals-ps.c
@@ -57,6 +57,12 @@
return s.x; // no-warning
}
+void f6(int x) {
+ int a[20];
+ if (x == 25) {}
+ if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a garbage value due to array index out of bounds}}
+}
+
int ret_uninit() {
int i;
int *p =
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -73,10 +73,31 @@
}
if (Ex) {
+ bool ArrayIndexOutOfBounds = false;
+ if (isa(Ex)) {
+SVal Loc = state->getSVal(Ex,LCtx);
+if (Loc.isValid()) {
+ const MemRegion *MR = Loc.castAs().getRegion();
+ if (const ElementRegion *ER = dyn_cast(MR)) {
+DefinedOrUnknownSVal Idx = ER->getIndex().castAs();
+DefinedOrUnknownSVal NumElements
+ = C.getStoreManager().getSizeInElements(state, ER->getSuperRegion(),
+ER->getValueType());
+ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true);
+ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false);
+if (StOutBound && !StInBound) {
+ ArrayIndexOutOfBounds = true;
+}
+ }
+}
+ }
+
OS << "The " << (isLeft ? "left" : "right")
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
<< "' is a garbage value";
+ if (ArrayIndexOutOfBounds)
+OS << " due to array index out of bounds";
}
else {
// Neither operand was undefined, but the result is undefined.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
danielmarjamaki added a comment. In https://reviews.llvm.org/D28278#677905, @zaks.anna wrote: > Does the code you added detects array out of bounds cases without false > positives? Is it an option to just have this checkers produce a more precise > error message in the specific case. > > A lot of work will probably need to be done to implement a proper array out > of bounds checking and no-one is working on that. I don't know.. maybe I can avoid some false positive. Maybe if the left operand seems to be out-of-bounds and the right operand is uninitialized maybe it would be better to complain about the right operand. It is definitely an option for me to have this checker produce more precise error messages. I believe that will solve my problems. Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna added a comment. Does the code you added detects array out of bounds cases without false positives? Is it an option to just have this checkers produce a more precise error message in the specific case. A lot of work will probably need to be done to implement a proper array out of bounds checking and no-one is working on that. Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
danielmarjamaki added a comment. I am not against that the error is shown as long as it's not misleading/wrong. To avoid misleading, in my humble opinion the error message should say "array index out of bounds". Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna added a comment. I think it's more valuable to report a warning here even if the error message is not very precise. Marking something as in bounds when we know it's not does not feel right and could lead to inconsistent states down the road. Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
danielmarjamaki added a comment. In https://reviews.llvm.org/D28278#639710, @xazax.hun wrote: > Did you experience any problems with the array out of bounds check lately? In > case it was stable on large code-bases and did not give too many false > positives, I think it might be worth to move that check out of alpha at the > same time, so users who do not turn on alpha checks will not lose any > functionality. What do you think? I don't have precise statistics. But these array-index-out-of-bounds messages are often false positives. Fixes are needed in the ExprEngine. Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
xazax.hun added a comment. Did you experience any problems with the array out of bounds check lately? In case it was stable on large code-bases and did not give too many false positives, I think it might be worth to move that check out of alpha at the same time, so users who do not turn on alpha checks will not lose any functionality. What do you think? Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
danielmarjamaki created this revision.
danielmarjamaki added reviewers: zaks.anna, dcoughlin.
danielmarjamaki added a subscriber: cfe-commits.
danielmarjamaki set the repository for this revision to rL LLVM.
Example code:
void f1(int x) {
int a[20] = {0};
if (x==25) {}
if (a[x] == 123) {} // <- Warning
}
If I don't enable alpha, only core, then Clang writes this misleading FP:
undef.c:5:12: warning: The left operand of '==' is a garbage value
I say it's a FP because the message is wrong. If the message correctly said
"array index out of bounds" and pointed out a[x] directly, then it would be TP.
This message goes away if alpha is enabled and I believe that is by intention.
Since there is a array-index-out-of-bounds check in alpha I am guessing that
the UndefinedBinaryOperatorResult should not report "array index out of
bounds". Therefore I remove this warning from this check.
This patch is a experimental work in progress. I would like to know if you
think I should modifiy the UndefinedBinaryOperatorResult check or if I should
do something in the ExprEngine? Maybe array index out of bounds should not lead
to Undef SVal?
With this patch, all the existing tests succeed.
Repository:
rL LLVM
https://reviews.llvm.org/D28278
Files:
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -61,7 +61,7 @@
SmallString<256> sbuf;
llvm::raw_svector_ostream OS(sbuf);
const Expr *Ex = nullptr;
-bool isLeft = true;
+bool isLeft;
if (state->getSVal(B->getLHS(), LCtx).isUndef()) {
Ex = B->getLHS()->IgnoreParenCasts();
@@ -73,6 +73,24 @@
}
if (Ex) {
+ if (isa(Ex)) {
+SVal Loc = state->getSVal(Ex,LCtx);
+if (Loc.isValid()) {
+ const MemRegion *MR = Loc.castAs().getRegion();
+ if (const ElementRegion *ER = dyn_cast(MR)) {
+DefinedOrUnknownSVal Idx =
ER->getIndex().castAs();
+DefinedOrUnknownSVal NumElements
+ = C.getStoreManager().getSizeInElements(state,
ER->getSuperRegion(),
+ER->getValueType());
+ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements,
true);
+ProgramStateRef StOutBound = state->assumeInBound(Idx,
NumElements, false);
+if (StOutBound && !StInBound) {
+ return;
+}
+ }
+}
+ }
+
OS << "The " << (isLeft ? "left" : "right")
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -61,7 +61,7 @@
SmallString<256> sbuf;
llvm::raw_svector_ostream OS(sbuf);
const Expr *Ex = nullptr;
-bool isLeft = true;
+bool isLeft;
if (state->getSVal(B->getLHS(), LCtx).isUndef()) {
Ex = B->getLHS()->IgnoreParenCasts();
@@ -73,6 +73,24 @@
}
if (Ex) {
+ if (isa(Ex)) {
+SVal Loc = state->getSVal(Ex,LCtx);
+if (Loc.isValid()) {
+ const MemRegion *MR = Loc.castAs().getRegion();
+ if (const ElementRegion *ER = dyn_cast(MR)) {
+DefinedOrUnknownSVal Idx = ER->getIndex().castAs();
+DefinedOrUnknownSVal NumElements
+ = C.getStoreManager().getSizeInElements(state, ER->getSuperRegion(),
+ER->getValueType());
+ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true);
+ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false);
+if (StOutBound && !StInBound) {
+ return;
+}
+ }
+}
+ }
+
OS << "The " << (isLeft ? "left" : "right")
<< " operand of '"
<< BinaryOperator::getOpcodeStr(B->getOpcode())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
