[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL301224: [analyzer] Improve suppression for inlined defensive
checks before operator &. (authored by dergachev).
Changed prior to commit:
https://reviews.llvm.org/D31982?vs=95890=96446#toc
Repository:
rL LLVM
https://reviews.llvm.org/D31982
Files:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp
cfe/trunk/test/Analysis/inlining/inline-defensive-checks.c
cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp
cfe/trunk/test/Analysis/null-deref-offsets.c
cfe/trunk/test/Analysis/uninit-const.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -961,6 +961,26 @@
const Expr *Inner = nullptr;
if (const Expr *Ex = dyn_cast(S)) {
Ex = Ex->IgnoreParenCasts();
+
+// Performing operator `&' on an lvalue expression is essentially a no-op.
+// Then, if we are taking addresses of fields or elements, these are also
+// unlikely to matter.
+// FIXME: There's a hack in our Store implementation that always computes
+// field offsets around null pointers as if they are always equal to 0.
+// The idea here is to report accesses to fields as null dereferences
+// even though the pointer value that's being dereferenced is actually
+// the offset of the field rather than exactly 0.
+// See the FIXME in StoreManager's getLValueFieldOrIvar() method.
+// This code interacts heavily with this hack; otherwise the value
+// would not be null at all for most fields, so we'd be unable to track it.
+if (const auto *Op = dyn_cast(Ex))
+ if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue()) {
+Ex = Op->getSubExpr()->IgnoreParenCasts();
+while (const auto *ME = dyn_cast(Ex)) {
+ Ex = ME->getBase()->IgnoreParenCasts();
+}
+ }
+
if (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex))
Inner = Ex;
}
Index: cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp
@@ -404,9 +404,15 @@
case loc::ConcreteIntKind:
// While these seem funny, this can happen through casts.
-// FIXME: What we should return is the field offset. For example,
-// add the field offset to the integer value. That way funny things
+// FIXME: What we should return is the field offset, not base. For example,
+// add the field offset to the integer value. That way things
// like this work properly: &(((struct foo *) 0xa)->f)
+// However, that's not easy to fix without reducing our abilities
+// to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
+// is a null dereference even though we're dereferencing offset of f
+// rather than null. Coming up with an approach that computes offsets
+// over null pointers properly while still being able to catch null
+// dereferences might be worth it.
return Base;
default:
@@ -431,7 +437,7 @@
// If the base is an unknown or undefined value, just return it back.
// FIXME: For absolute pointer addresses, we just return that value back as
// well, although in reality we should return the offset added to that
- // value.
+ // value. See also the similar FIXME in getLValueFieldOrIvar().
if (Base.isUnknownOrUndef() || Base.getAs())
return Base;
Index: cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp
===
--- cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp
+++ cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp
@@ -70,4 +70,17 @@
void test(int *p1, int *p2) {
idc(p1);
Foo f(p1);
-}
\ No newline at end of file
+}
+
+struct Bar {
+ int x;
+};
+void idcBar(Bar *b) {
+ if (b)
+;
+}
+void testRefToField(Bar *b) {
+ idcBar(b);
+ int = b->x; // no-warning
+ x = 5;
+}
Index: cfe/trunk/test/Analysis/inlining/inline-defensive-checks.c
===
--- cfe/trunk/test/Analysis/inlining/inline-defensive-checks.c
+++ cfe/trunk/test/Analysis/inlining/inline-defensive-checks.c
@@ -1,7 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-inlined-defensive-checks=true -verify %s
// Perform inline defensive checks.
-void idc(int *p) {
+void idc(void *p) {
if (p)
;
}
@@ -139,3 +139,42 @@
int z = y;
idcTriggerZeroValueThroughCall(z);
}
+
+struct S {
+ int f1;
+ int f2;
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperators(struct S *s) {
+ idc(s);
+ *(&(s->f1)) = 7;
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/Store.cpp:440 // well, although in reality we should return the offset added to that - // value. + // value. See also the similar FIXME in getLValueFieldOrIvar(). if (Base.isUnknownOrUndef() || Base.getAs()) NoQ wrote: > Note that this code doesn't really trigger; we return `UnknownVal()` > somewhere above, as shown on the newly added tests. I suspect we may be > missing valid null dereferences because of that; will have a look. This is also addressed by D32291. https://reviews.llvm.org/D31982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:977
+ Ex = Op->getSubExpr()->IgnoreParenCasts();
+ while (true) {
+if (const auto *ME = dyn_cast(Ex)) {
zaks.anna wrote:
> Why do we need the "while (true)"? Can we just use "dyn_cast(Ex)"
> as the loop condition?
>
> Take a look at the getDerefExpr(const Stmt *S) and see if that would be a
> better place to add this code. Maybe not..
>
Accidentally clicked "Done" and forgot about the `getDerefExpr()` part.
I put it into a follow-up patch though: D32291, because it's quite a cascade of
changes already.
https://reviews.llvm.org/D31982
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965 + +// Performing operator `&' on an lvalue expression is essentially a no-op. +// Then, if we are taking addresses of fields or elements, these are also zaks.anna wrote: > NoQ wrote: > > alexshap wrote: > > > "Address-of" operator can be overloaded, > > > just wondering - doest this code work correctly in that case ? > > In this case we'd see a `CXXOperatorCallExpr` instead of `UnaryOperator` > > (all hail clang AST!). > Adding a test case for that would be good. Not sure. There are so many things that work differently in this scenario that i'm having troubles coming up with a test that tests exactly that and doesn't throw or not throw a warning for a dozen of other reasons. I'm even having troubles understanding what particular overload are we interested in. Did you have anything specific in mind? Comment at: lib/StaticAnalyzer/Core/Store.cpp:440 // well, although in reality we should return the offset added to that - // value. + // value. See also the similar FIXME in getLValueFieldOrIvar(). if (Base.isUnknownOrUndef() || Base.getAs()) Note that this code doesn't really trigger; we return `UnknownVal()` somewhere above, as shown on the newly added tests. I suspect we may be missing valid null dereferences because of that; will have a look. https://reviews.llvm.org/D31982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ updated this revision to Diff 95890.
NoQ marked 5 inline comments as done.
NoQ added a comment.
Fix comments!
https://reviews.llvm.org/D31982
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/explain-svals.cpp
test/Analysis/inlining/inline-defensive-checks.c
test/Analysis/inlining/inline-defensive-checks.cpp
test/Analysis/null-deref-offsets.c
test/Analysis/uninit-const.cpp
Index: test/Analysis/uninit-const.cpp
===
--- test/Analysis/uninit-const.cpp
+++ test/Analysis/uninit-const.cpp
@@ -122,7 +122,7 @@
}
void f_uninit(void) {
- int x;
+ int x; // expected-note {{'x' declared without an initial value}}
doStuff_uninit(); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
Index: test/Analysis/null-deref-offsets.c
===
--- /dev/null
+++ test/Analysis/null-deref-offsets.c
@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 -w -triple i386-apple-darwin10 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+struct S {
+ int x, y;
+ int z[2];
+};
+
+void testOffsets(struct S *s) {
+ if (s != 0)
+return;
+
+ // FIXME: Here we are testing the hack that computes offsets to null pointers
+ // as 0 in order to find null dereferences of not-exactly-null pointers,
+ // such as &(s->y) below, which is equal to 4 rather than 0 in run-time.
+
+ // These are indeed null.
+ clang_analyzer_eval(s == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(&(s->x) == 0); // expected-warning{{TRUE}}
+
+ // FIXME: These should ideally be true.
+ clang_analyzer_eval(&(s->y) == 4); // expected-warning{{FALSE}}
+ clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{UNKNOWN}}
+
+ // FIXME: These should ideally be false.
+ clang_analyzer_eval(&(s->y) == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{UNKNOWN}}
+
+ // But this should still be a null dereference.
+ s->y = 5; // expected-warning{{Access to field 'y' results in a dereference of a null pointer (loaded from variable 's')}}
+}
Index: test/Analysis/inlining/inline-defensive-checks.cpp
===
--- test/Analysis/inlining/inline-defensive-checks.cpp
+++ test/Analysis/inlining/inline-defensive-checks.cpp
@@ -70,4 +70,17 @@
void test(int *p1, int *p2) {
idc(p1);
Foo f(p1);
-}
\ No newline at end of file
+}
+
+struct Bar {
+ int x;
+};
+void idcBar(Bar *b) {
+ if (b)
+;
+}
+void testRefToField(Bar *b) {
+ idcBar(b);
+ int = b->x; // no-warning
+ x = 5;
+}
Index: test/Analysis/inlining/inline-defensive-checks.c
===
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -1,7 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-inlined-defensive-checks=true -verify %s
// Perform inline defensive checks.
-void idc(int *p) {
+void idc(void *p) {
if (p)
;
}
@@ -139,3 +139,42 @@
int z = y;
idcTriggerZeroValueThroughCall(z);
}
+
+struct S {
+ int f1;
+ int f2;
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperators(struct S *s) {
+ idc(s);
+ *(&(s->f1)) = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset1(struct S *s) {
+ idc(s);
+ int *x = &(s->f2);
+ *x = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset2(struct S *s) {
+ idc(s);
+ int *x = &(s->f2) - 1;
+ // FIXME: Should not warn.
+ *x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithAssignment(struct S *s) {
+ idc(s);
+ int *x = &(s->f1);
+ *x = 7; // no-warning
+}
+
+
+struct S2 {
+ int a[1];
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField(struct S2 *s) {
+ idc(s);
+ *(&(s->a[0])) = 7; // no-warning
+}
Index: test/Analysis/explain-svals.cpp
===
--- test/Analysis/explain-svals.cpp
+++ test/Analysis/explain-svals.cpp
@@ -81,9 +81,10 @@
namespace {
class C {
+public:
int x[10];
+ int y;
-public:
void test_5(int i) {
clang_analyzer_explain(this); // expected-warning-re^pointer to 'this' object$
clang_analyzer_explain([i]); // expected-warning-re^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$
Index: lib/StaticAnalyzer/Core/Store.cpp
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
zaks.anna added a comment.
Thanks!!!
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965
+
+// Performing operator `&' on an lvalue expression is essentially a no-op.
+// Then, if we are taking addresses of fields or elements, these are also
NoQ wrote:
> alexshap wrote:
> > "Address-of" operator can be overloaded,
> > just wondering - doest this code work correctly in that case ?
> In this case we'd see a `CXXOperatorCallExpr` instead of `UnaryOperator` (all
> hail clang AST!).
Adding a test case for that would be good.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:968
+// unlikely to matter.
+// FIXME: Currently offsets of fields are computed incorrectly,
+// being always equal to 0. See the FIXME in StoreManager's
Incorrect implies that there is a better "correct" model and invites a fix. Do
we know what better model would be? If so, we could add that to the comment. If
not, I'd prefer explaining why it works this way (similarly to how you did in
the comment below). Maybe adding an example of what does not work. And you
could add a FIXME to say that it's worth investigating if there is a better way
of handling it. (The majority of this info should probably go to Store.cpp)
Also, maybe it's just the choice of words here. "Incorrect" sounds like
something that needs to be corrected. Whereas you could use something like "is
modeled imprecisely with respect to what happens at execution time", which
could still mean that this is how we do want to model it going forward.
It seems that the problem with modeling this way is demonstrated with a test
case in explain-svals.cpp. So the benefits of the current modeling seem to be
worth it.
Can we add a note along the path saying that "p" in "p->f" is null? This would
address the user confusion with imprecise modeling.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:977
+ Ex = Op->getSubExpr()->IgnoreParenCasts();
+ while (true) {
+if (const auto *ME = dyn_cast(Ex)) {
Why do we need the "while (true)"? Can we just use "dyn_cast(Ex)"
as the loop condition?
Take a look at the getDerefExpr(const Stmt *S) and see if that would be a
better place to add this code. Maybe not..
Comment at: lib/StaticAnalyzer/Core/Store.cpp:405
case loc::ConcreteIntKind:
// While these seem funny, this can happen through casts.
// FIXME: What we should return is the field offset. For example,
Could you rephrase this existing comment while you are here? Using word "funny"
seems content-free and a bit strange.
Comment at: lib/StaticAnalyzer/Core/Store.cpp:409
// like this work properly: &(((struct foo *) 0xa)->f)
+// However, that's not easy to fix without reducing our abilities
+// to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
Thanks for adding the explanation!
Can you think of other cases where the same would apply? (Ex: array index)
https://reviews.llvm.org/D31982
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Yay! This is going to fix some really confusing false positives.LGTM with Gabor's comments addressed. One note. I am not a big fan of using clang_analyzer_explain() in tests for functionality. It is great for debugging, but for testing it exposes too much of the implementation details of the region hierarchy and store. Exposing this kind of state is fine in tests specific to the store/region store, but in my opinion it should be another clang_analyzer_ entry point and should be limited to regions. Similarly we should have a clang_analyzer_ entry point for testing symbols but that doesn't expose the guts of MemRegions. And (maybe?) another one for SymExprs. Comment at: test/Analysis/explain-svals.cpp:103 + C *c = 0; + // FIXME: we need to be explaining '40' rather than '0' here; not explainer bug. + clang_analyzer_explain(>y); // expected-warning-re^concrete memory address '0'$ It would be good to mention that this is a modeling bug and point to where it could be fixed. Otherwise the FIXME is not super helpful to future maintainers. https://reviews.llvm.org/D31982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:975
+ if (Op->getOpcode() == UO_AddrOf)
+if (Op->getSubExpr()->isLValue()) {
+ Ex = Op->getSubExpr()->IgnoreParenCasts();
Maybe you could move this condition one level up with &&?
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:977
+ Ex = Op->getSubExpr()->IgnoreParenCasts();
+ while (true) {
+if (const auto *ME = dyn_cast(Ex)) {
Can't you writhe this like:
```
while (const auto *ME = dyn_cast(Ex)) {
Ex = ME->getBase()->IgnoreParenCasts();
}
```
?
https://reviews.llvm.org/D31982
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ updated this revision to Diff 95082.
NoQ added a comment.
Remove accidentally added braces in unrelated code.
https://reviews.llvm.org/D31982
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/explain-svals.cpp
test/Analysis/inlining/inline-defensive-checks.c
test/Analysis/inlining/inline-defensive-checks.cpp
test/Analysis/uninit-const.cpp
Index: test/Analysis/uninit-const.cpp
===
--- test/Analysis/uninit-const.cpp
+++ test/Analysis/uninit-const.cpp
@@ -122,7 +122,7 @@
}
void f_uninit(void) {
- int x;
+ int x; // expected-note {{'x' declared without an initial value}}
doStuff_uninit(); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
Index: test/Analysis/inlining/inline-defensive-checks.cpp
===
--- test/Analysis/inlining/inline-defensive-checks.cpp
+++ test/Analysis/inlining/inline-defensive-checks.cpp
@@ -70,4 +70,17 @@
void test(int *p1, int *p2) {
idc(p1);
Foo f(p1);
-}
\ No newline at end of file
+}
+
+struct Bar {
+ int x;
+};
+void idcBar(Bar *b) {
+ if (b)
+;
+}
+void testRefToField(Bar *b) {
+ idcBar(b);
+ int = b->x; // no-warning
+ x = 5;
+}
Index: test/Analysis/inlining/inline-defensive-checks.c
===
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -1,7 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-inlined-defensive-checks=true -verify %s
// Perform inline defensive checks.
-void idc(int *p) {
+void idc(void *p) {
if (p)
;
}
@@ -139,3 +139,42 @@
int z = y;
idcTriggerZeroValueThroughCall(z);
}
+
+struct S {
+ int f1;
+ int f2;
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperators(struct S *s) {
+ idc(s);
+ *(&(s->f1)) = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset1(struct S *s) {
+ idc(s);
+ int *x = &(s->f2);
+ *x = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset2(struct S *s) {
+ idc(s);
+ int *x = &(s->f2) - 1;
+ // FIXME: Should not warn.
+ *x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithAssignment(struct S *s) {
+ idc(s);
+ int *x = &(s->f1);
+ *x = 7; // no-warning
+}
+
+
+struct S2 {
+ int a[1];
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField(struct S2 *s) {
+ idc(s);
+ *(&(s->a[0])) = 7; // no-warning
+}
Index: test/Analysis/explain-svals.cpp
===
--- test/Analysis/explain-svals.cpp
+++ test/Analysis/explain-svals.cpp
@@ -81,9 +81,10 @@
namespace {
class C {
+public:
int x[10];
+ int y;
-public:
void test_5(int i) {
clang_analyzer_explain(this); // expected-warning-re^pointer to 'this' object$
clang_analyzer_explain([i]); // expected-warning-re^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$
@@ -96,3 +97,9 @@
clang_analyzer_explain(conjure_S()); // expected-warning-re^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$
clang_analyzer_explain(conjure_S().z); // expected-warning-re^value derived from \(symbol of type 'struct S' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$
}
+
+void test_7() {
+ C *c = 0;
+ // FIXME: we need to be explaining '40' rather than '0' here; not explainer bug.
+ clang_analyzer_explain(>y); // expected-warning-re^concrete memory address '0'$
+}
Index: lib/StaticAnalyzer/Core/Store.cpp
===
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -406,6 +406,10 @@
// FIXME: What we should return is the field offset. For example,
// add the field offset to the integer value. That way funny things
// like this work properly: &(((struct foo *) 0xa)->f)
+// However, that's not easy to fix without reducing our abilities
+// to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
+// is a null dereference even though we're dereferencing offset of f
+// rather than null.
return Base;
default:
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -961,6 +961,28 @@
const Expr *Inner = nullptr;
if (const Expr
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965 + +// Performing operator `&' on an lvalue expression is essentially a no-op. +// Then, if we are taking addresses of fields or elements, these are also alexshap wrote: > "Address-of" operator can be overloaded, > just wondering - doest this code work correctly in that case ? In this case we'd see a `CXXOperatorCallExpr` instead of `UnaryOperator` (all hail clang AST!). https://reviews.llvm.org/D31982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
alexshap added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965 + +// Performing operator `&' on an lvalue expression is essentially a no-op. +// Then, if we are taking addresses of fields or elements, these are also "Address-of" operator can be overloaded, just wondering - doest this code work correctly in that case ? https://reviews.llvm.org/D31982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
NoQ created this revision.
In code
void foo(int *p) {
if (p) {}
}
void bar(int *p) {
foo(p);
*p = 5;
}
we suppress the null dereference warning in `*p = 5` because the state split
within the inlined function is essentially irrelevant, as this is merely a
defensive check that does not necessarily trigger in this caller context.
The suppression works by tracking the null pointer symbol in a bug report
visitor and seeing if it was constrained to 0 inside a callee context. The fact
that we have to rely on such manual suppressions is a downside of the
inlining-based approach to interprocedural analysis.
The suppression gets confused when the null dereference becomes more complex,
eg:
struct S {
int x;
}
void foo(struct S *s) {
if (s) {}
}
void bar(struct S *s) {
foo(s);
*(&(s->x)) = 5;
}
In this case `s` is `{reg_$0}`, and `reg_$0` is constrained to
`[0, 0]`, similarly to the above. However, while computing `s->x` and then
`&(s->x)`, the symbol is collapsed to a constant `0 (Loc)`, making it harder to
track the symbol. The visitor is improved to handle this case.
While the fact that //offset of field `x` in struct `S` in our example is equal
to 0// is purely accidental, with any other offset it'd still be a null
dereference, worth suppressing. How does the analyzer handle the non-zero
offset case and still see a null dereference? Simply and powerfully: it
mis-computes the offset, incorrectly believing that all offsets are equal to 0
(woohoo!). I add a test case to document this behavior; even though it's
trivial to fix, the positive side effects of this bug are for now too useful to
discard.
https://reviews.llvm.org/D31982
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/explain-svals.cpp
test/Analysis/inlining/inline-defensive-checks.c
test/Analysis/inlining/inline-defensive-checks.cpp
test/Analysis/uninit-const.cpp
Index: test/Analysis/uninit-const.cpp
===
--- test/Analysis/uninit-const.cpp
+++ test/Analysis/uninit-const.cpp
@@ -122,7 +122,7 @@
}
void f_uninit(void) {
- int x;
+ int x; // expected-note {{'x' declared without an initial value}}
doStuff_uninit(); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
Index: test/Analysis/inlining/inline-defensive-checks.cpp
===
--- test/Analysis/inlining/inline-defensive-checks.cpp
+++ test/Analysis/inlining/inline-defensive-checks.cpp
@@ -70,4 +70,17 @@
void test(int *p1, int *p2) {
idc(p1);
Foo f(p1);
-}
\ No newline at end of file
+}
+
+struct Bar {
+ int x;
+};
+void idcBar(Bar *b) {
+ if (b)
+;
+}
+void testRefToField(Bar *b) {
+ idcBar(b);
+ int = b->x; // no-warning
+ x = 5;
+}
Index: test/Analysis/inlining/inline-defensive-checks.c
===
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -1,7 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-inlined-defensive-checks=true -verify %s
// Perform inline defensive checks.
-void idc(int *p) {
+void idc(void *p) {
if (p)
;
}
@@ -139,3 +139,42 @@
int z = y;
idcTriggerZeroValueThroughCall(z);
}
+
+struct S {
+ int f1;
+ int f2;
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperators(struct S *s) {
+ idc(s);
+ *(&(s->f1)) = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset1(struct S *s) {
+ idc(s);
+ int *x = &(s->f2);
+ *x = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset2(struct S *s) {
+ idc(s);
+ int *x = &(s->f2) - 1;
+ // FIXME: Should not warn.
+ *x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithAssignment(struct S *s) {
+ idc(s);
+ int *x = &(s->f1);
+ *x = 7; // no-warning
+}
+
+
+struct S2 {
+ int a[1];
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField(struct S2 *s) {
+ idc(s);
+ *(&(s->a[0])) = 7; // no-warning
+}
Index: test/Analysis/explain-svals.cpp
===
--- test/Analysis/explain-svals.cpp
+++ test/Analysis/explain-svals.cpp
@@ -81,9 +81,10 @@
namespace {
class C {
+public:
int x[10];
+ int y;
-public:
void test_5(int i) {
clang_analyzer_explain(this); // expected-warning-re^pointer to 'this' object$
clang_analyzer_explain([i]); // expected-warning-re^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$
@@ -96,3 +97,9 @@
