[PATCH] D42192: [analyzer] Assume that the allocated value is non-null before construction, not after.
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL323370: [analyzer] Assume that the allocated value is
non-null before construction. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D42192?vs=130335=131326#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42192
Files:
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
cfe/trunk/test/Analysis/new-ctor-conservative.cpp
cfe/trunk/test/Analysis/new-ctor-null-throw.cpp
cfe/trunk/test/Analysis/new-ctor-null.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -500,9 +500,24 @@
// is breaking too much to evaluate the no-op symbolic cast over it, so we
// skip it for now.
ProgramStateRef State = I->getState();
-ValueBldr.generateNode(
-CNE, I,
-setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
+SVal RetVal = State->getSVal(CNE, LCtx);
+
+// If this allocation function is not declared as non-throwing, failures
+// /must/ be signalled by exceptions, and thus the return value will never
+// be NULL. -fno-exceptions does not influence this semantics.
+// FIXME: GCC has a -fcheck-new option, which forces it to consider the case
+// where new can return NULL. If we end up supporting that option, we can
+// consider adding a check for it here.
+// C++11 [basic.stc.dynamic.allocation]p3.
+if (const FunctionDecl *FD = CNE->getOperatorNew()) {
+ QualType Ty = FD->getType();
+ if (const auto *ProtoType = Ty->getAs())
+if (!ProtoType->isNothrow(getContext()))
+ State = State->assume(RetVal.castAs(), true);
+}
+
+ValueBldr.generateNode(CNE, I,
+ setCXXNewAllocatorValue(State, CNE, LCtx, RetVal));
}
ExplodedNodeSet DstPostPostCallCallback;
@@ -559,21 +574,21 @@
State = Call->invalidateRegions(blockCount);
if (!State)
return;
- }
- // If this allocation function is not declared as non-throwing, failures
- // /must/ be signalled by exceptions, and thus the return value will never be
- // NULL. -fno-exceptions does not influence this semantics.
- // FIXME: GCC has a -fcheck-new option, which forces it to consider the case
- // where new can return NULL. If we end up supporting that option, we can
- // consider adding a check for it here.
- // C++11 [basic.stc.dynamic.allocation]p3.
- if (FD) {
-QualType Ty = FD->getType();
-if (const FunctionProtoType *ProtoType = Ty->getAs())
- if (!ProtoType->isNothrow(getContext()))
-if (auto dSymVal = symVal.getAs())
- State = State->assume(*dSymVal, true);
+// If this allocation function is not declared as non-throwing, failures
+// /must/ be signalled by exceptions, and thus the return value will never
+// be NULL. -fno-exceptions does not influence this semantics.
+// FIXME: GCC has a -fcheck-new option, which forces it to consider the case
+// where new can return NULL. If we end up supporting that option, we can
+// consider adding a check for it here.
+// C++11 [basic.stc.dynamic.allocation]p3.
+if (FD) {
+ QualType Ty = FD->getType();
+ if (const auto *ProtoType = Ty->getAs())
+if (!ProtoType->isNothrow(getContext()))
+ if (auto dSymVal = symVal.getAs())
+State = State->assume(*dSymVal, true);
+}
}
StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
Index: cfe/trunk/test/Analysis/new-ctor-null.cpp
===
--- cfe/trunk/test/Analysis/new-ctor-null.cpp
+++ cfe/trunk/test/Analysis/new-ctor-null.cpp
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
typedef __typeof__(sizeof(int)) size_t;
@@ -13,11 +14,23 @@
struct S {
int x;
- S() : x(1) {}
+ S() : x(1) {
+// FIXME: Constructor should not be called with null this, even if it was
+// returned by operator new().
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+ }
~S() {}
};
void testArrays() {
S *s = new S[10]; // no-crash
s[0].x = 2; // expected-warning{{Dereference of null pointer}}
}
+
+int global;
+void testInvalidationOnConstructionIntoNull() {
+ global = 0;
+ S *s = new S();
+ // FIXME: Should be FALSE - we should not invalidate globals.
+ clang_analyzer_eval(global); // expected-warning{{UNKNOWN}}
+}
Index: cfe/trunk/test/Analysis/new-ctor-null-throw.cpp
[PATCH] D42192: [analyzer] Assume that the allocated value is non-null before construction, not after.
NoQ updated this revision to Diff 130335.
NoQ added a comment.
Use `auto`.
https://reviews.llvm.org/D42192
Files:
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/new-ctor-conservative.cpp
test/Analysis/new-ctor-null-throw.cpp
test/Analysis/new-ctor-null.cpp
Index: test/Analysis/new-ctor-null.cpp
===
--- test/Analysis/new-ctor-null.cpp
+++ test/Analysis/new-ctor-null.cpp
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
typedef __typeof__(sizeof(int)) size_t;
@@ -13,11 +14,23 @@
struct S {
int x;
- S() : x(1) {}
+ S() : x(1) {
+// FIXME: Constructor should not be called with null this, even if it was
+// returned by operator new().
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+ }
~S() {}
};
void testArrays() {
S *s = new S[10]; // no-crash
s[0].x = 2; // expected-warning{{Dereference of null pointer}}
}
+
+int global;
+void testInvalidationOnConstructionIntoNull() {
+ global = 0;
+ S *s = new S();
+ // FIXME: Should be FALSE - we should not invalidate globals.
+ clang_analyzer_eval(global); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/new-ctor-null-throw.cpp
===
--- /dev/null
+++ test/Analysis/new-ctor-null-throw.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+
+// These are ill-formed. One cannot return nullptr from a throwing version of an
+// operator new.
+void *operator new(size_t size) {
+ return nullptr;
+}
+void *operator new[](size_t size) {
+ return nullptr;
+}
+
+struct S {
+ int x;
+ S() : x(1) {}
+ ~S() {}
+};
+
+void testArrays() {
+ S *s = new S[10]; // no-crash
+ s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}
Index: test/Analysis/new-ctor-conservative.cpp
===
--- test/Analysis/new-ctor-conservative.cpp
+++ test/Analysis/new-ctor-conservative.cpp
@@ -1,6 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
struct S {
int x;
@@ -27,3 +28,19 @@
// FIXME: Should be true once we inline array constructors.
clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
}
+
+struct NullS {
+ NullS() {
+if (this) {}
+ }
+ NullS(int x) {
+if (!this) {
+ clang_analyzer_warnIfReached(); // no-warning
+}
+ }
+};
+
+void checkNullThis() {
+ NullS *nulls = new NullS(); // no-crash
+ NullS *nulls2 = new NullS(0);
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -500,9 +500,24 @@
// is breaking too much to evaluate the no-op symbolic cast over it, so we
// skip it for now.
ProgramStateRef State = I->getState();
-ValueBldr.generateNode(
-CNE, I,
-setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
+SVal RetVal = State->getSVal(CNE, LCtx);
+
+// If this allocation function is not declared as non-throwing, failures
+// /must/ be signalled by exceptions, and thus the return value will never
+// be NULL. -fno-exceptions does not influence this semantics.
+// FIXME: GCC has a -fcheck-new option, which forces it to consider the case
+// where new can return NULL. If we end up supporting that option, we can
+// consider adding a check for it here.
+// C++11 [basic.stc.dynamic.allocation]p3.
+if (const FunctionDecl *FD = CNE->getOperatorNew()) {
+ QualType Ty = FD->getType();
+ if (const auto *ProtoType = Ty->getAs())
+if (!ProtoType->isNothrow(getContext()))
+ State = State->assume(RetVal.castAs(), true);
+}
+
+ValueBldr.generateNode(CNE, I,
+ setCXXNewAllocatorValue(State, CNE, LCtx, RetVal));
}
ExplodedNodeSet DstPostPostCallCallback;
@@ -559,21 +574,21 @@
State = Call->invalidateRegions(blockCount);
if (!State)
return;
- }
- // If this allocation function is not declared as non-throwing, failures
- // /must/ be signalled by exceptions, and thus the return value will never be
- // NULL. -fno-exceptions does not influence this semantics.
- // FIXME: GCC has
[PATCH] D42192: [analyzer] Assume that the allocated value is non-null before construction, not after.
NoQ marked an inline comment as done. NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:516 +if (!ProtoType->isNothrow(getContext())) + State = State->assume(RetVal.castAs(), true); +} george.karpenkov wrote: > This is neither here nor there, but for this and many other cases I think we > could be considerably more readable by defining helpers `State->assumeIsTrue` > and `State->assumeIsFalse` (or `assumeNonNull`, or whatever is most > descriptive) Dunno, `assume(x, true)` sounds descriptive enough to me, and also feels more like a natural spoken language. https://reviews.llvm.org/D42192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42192: [analyzer] Assume that the allocated value is non-null before construction, not after.
NoQ added a comment. Forgot to mention that this patch follows the tradition of duplicating code from `VisitCXXNewExpr()` to `VisitCXXNewAllocatorCall()` - until we enable `c++-allocator-inlining` by default and clean away the old mode support from `VisitCXXNewExpr()` (which would become pretty trivial). Repository: rC Clang https://reviews.llvm.org/D42192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42192: [analyzer] Assume that the allocated value is non-null before construction, not after.
george.karpenkov added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:514 + QualType Ty = FD->getType(); + if (const FunctionProtoType *ProtoType = Ty->getAs()) +if (!ProtoType->isNothrow(getContext())) auto? Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:516 +if (!ProtoType->isNothrow(getContext())) + State = State->assume(RetVal.castAs(), true); +} This is neither here nor there, but for this and many other cases I think we could be considerably more readable by defining helpers `State->assumeIsTrue` and `State->assumeIsFalse` (or `assumeNonNull`, or whatever is most descriptive) Repository: rC Clang https://reviews.llvm.org/D42192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42192: [analyzer] Assume that the allocated value is non-null before construction, not after.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
In the `c++-allocator-inlining=true` mode, we need to make the assumption that
the conservatively evaluated operator new() has returned a non-null value.
Previously we did this on `CXXNewExpr`, but now we have to do that before
calling the constructor, because some clever constructors are sometimes
assuming that their `this` is null and doing weird stuff. We would also crash
upon evaluating `CXXNewExpr` when the allocator was inlined and returned null
and had a throw specification; this is UB even for custom allocators, but we
still need not to crash.
Added more FIXME tests to ensure that eventually we fix calling the constructor
for null return values.
Repository:
rC Clang
https://reviews.llvm.org/D42192
Files:
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/new-ctor-conservative.cpp
test/Analysis/new-ctor-null-throw.cpp
test/Analysis/new-ctor-null.cpp
Index: test/Analysis/new-ctor-null.cpp
===
--- test/Analysis/new-ctor-null.cpp
+++ test/Analysis/new-ctor-null.cpp
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
typedef __typeof__(sizeof(int)) size_t;
@@ -13,11 +14,23 @@
struct S {
int x;
- S() : x(1) {}
+ S() : x(1) {
+// FIXME: Constructor should not be called with null this, even if it was
+// returned by operator new().
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+ }
~S() {}
};
void testArrays() {
S *s = new S[10]; // no-crash
s[0].x = 2; // expected-warning{{Dereference of null pointer}}
}
+
+int global;
+void testInvalidationOnConstructionIntoNull() {
+ global = 0;
+ S *s = new S();
+ // FIXME: Should be FALSE - we should not invalidate globals.
+ clang_analyzer_eval(global); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/new-ctor-null-throw.cpp
===
--- /dev/null
+++ test/Analysis/new-ctor-null-throw.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+
+// These are ill-formed. One cannot return nullptr from a throwing version of an
+// operator new.
+void *operator new(size_t size) {
+ return nullptr;
+}
+void *operator new[](size_t size) {
+ return nullptr;
+}
+
+struct S {
+ int x;
+ S() : x(1) {}
+ ~S() {}
+};
+
+void testArrays() {
+ S *s = new S[10]; // no-crash
+ s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+}
Index: test/Analysis/new-ctor-conservative.cpp
===
--- test/Analysis/new-ctor-conservative.cpp
+++ test/Analysis/new-ctor-conservative.cpp
@@ -1,6 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
struct S {
int x;
@@ -27,3 +28,19 @@
// FIXME: Should be true once we inline array constructors.
clang_analyzer_eval(s[0].x == 1); // expected-warning{{UNKNOWN}}
}
+
+struct NullS {
+ NullS() {
+if (this) {}
+ }
+ NullS(int x) {
+if (!this) {
+ clang_analyzer_warnIfReached(); // no-warning
+}
+ }
+};
+
+void checkNullThis() {
+ NullS *nulls = new NullS(); // no-crash
+ NullS *nulls2 = new NullS(0);
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -500,9 +500,24 @@
// is breaking too much to evaluate the no-op symbolic cast over it, so we
// skip it for now.
ProgramStateRef State = I->getState();
-ValueBldr.generateNode(
-CNE, I,
-setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
+SVal RetVal = State->getSVal(CNE, LCtx);
+
+// If this allocation function is not declared as non-throwing, failures
+// /must/ be signalled by exceptions, and thus the return value will never
+// be NULL. -fno-exceptions does not influence this semantics.
+// FIXME: GCC has a -fcheck-new option, which forces it to consider the case
+// where new can return NULL. If we end up supporting that option, we can
+// consider adding a check for it here.
+// C++11 [basic.stc.dynamic.allocation]p3.
+
