[PATCH] D42876: [analyzer] Support returning objects by value.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Committed this but put a wrong phabricator link in the commit message, sorry >_< https://reviews.llvm.org/rC325202 https://reviews.llvm.org/D42876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42876: [analyzer] Support returning objects by value.
NoQ updated this revision to Diff 134346.
NoQ added a comment.
Update the comment with some thoughts from
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html
https://reviews.llvm.org/D42876
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1,9 +1,10 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s -std=c++11
extern bool clang_analyzer_eval(bool);
extern bool clang_analyzer_warnIfReached();
+void clang_analyzer_checkInlined(bool);
struct Trivial {
Trivial(int x) : value(x) {}
@@ -422,6 +423,10 @@
struct CtorWithNoReturnDtor {
CtorWithNoReturnDtor() = default;
+CtorWithNoReturnDtor(int x) {
+ clang_analyzer_checkInlined(false); // no-warning
+}
+
~CtorWithNoReturnDtor() __attribute__((noreturn));
};
@@ -439,6 +444,12 @@
clang_analyzer_warnIfReached(); // no-warning
}
+#if __cplusplus >= 201103L
+ CtorWithNoReturnDtor returnNoReturnDtor() {
+return {1}; // no-crash
+ }
+#endif
+
#endif // TEMPORARY_DTORS
}
@@ -530,3 +541,152 @@
Sub(i).m();
}
}
+
+namespace test_return_temporary {
+class C {
+ int x, y;
+
+public:
+ C(int x, int y) : x(x), y(y) {}
+ int getX() const { return x; }
+ int getY() const { return y; }
+ ~C() {}
+};
+
+class D: public C {
+public:
+ D() : C(1, 2) {}
+ D(const D ): C(d.getX(), d.getY()) {}
+};
+
+C returnTemporaryWithVariable() { C c(1, 2); return c; }
+C returnTemporaryWithAnotherFunctionWithVariable() {
+ return returnTemporaryWithVariable();
+}
+C returnTemporaryWithCopyConstructionWithVariable() {
+ return C(returnTemporaryWithVariable());
+}
+
+C returnTemporaryWithConstruction() { return C(1, 2); }
+C returnTemporaryWithAnotherFunctionWithConstruction() {
+ return returnTemporaryWithConstruction();
+}
+C returnTemporaryWithCopyConstructionWithConstruction() {
+ return C(returnTemporaryWithConstruction());
+}
+
+D returnTemporaryWithVariableAndNonTrivialCopy() { D d; return d; }
+D returnTemporaryWithAnotherFunctionWithVariableAndNonTrivialCopy() {
+ return returnTemporaryWithVariableAndNonTrivialCopy();
+}
+D returnTemporaryWithCopyConstructionWithVariableAndNonTrivialCopy() {
+ return D(returnTemporaryWithVariableAndNonTrivialCopy());
+}
+
+#if __cplusplus >= 201103L
+C returnTemporaryWithBraces() { return {1, 2}; }
+C returnTemporaryWithAnotherFunctionWithBraces() {
+ return returnTemporaryWithBraces();
+}
+C returnTemporaryWithCopyConstructionWithBraces() {
+ return C(returnTemporaryWithBraces());
+}
+#endif // C++11
+
+void test() {
+ C c1 = returnTemporaryWithVariable();
+ clang_analyzer_eval(c1.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c1.getY() == 2); // expected-warning{{TRUE}}
+
+ C c2 = returnTemporaryWithAnotherFunctionWithVariable();
+ clang_analyzer_eval(c2.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c2.getY() == 2); // expected-warning{{TRUE}}
+
+ C c3 = returnTemporaryWithCopyConstructionWithVariable();
+ clang_analyzer_eval(c3.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c3.getY() == 2); // expected-warning{{TRUE}}
+
+ C c4 = returnTemporaryWithConstruction();
+ // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+ // in the inlined function is supported.
+ clang_analyzer_eval(c4.getX() == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(c4.getY() == 2); // expected-warning{{UNKNOWN}}
+
+ C c5 = returnTemporaryWithAnotherFunctionWithConstruction();
+ // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+ // in the inlined function is supported.
+ clang_analyzer_eval(c5.getX() == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(c5.getY() == 2); // expected-warning{{UNKNOWN}}
+
+ C c6 = returnTemporaryWithCopyConstructionWithConstruction();
+ // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+ // in the inlined function is supported.
+ clang_analyzer_eval(c5.getX() == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(c5.getY() == 2); // expected-warning{{UNKNOWN}}
+
+#if __cplusplus >= 201103L
+
+ C c7 =
[PATCH] D42876: [analyzer] Support returning objects by value.
NoQ updated this revision to Diff 133933.
NoQ added a comment.
Well, it still seems to be a reasonable improvement given how all temporary
materialization works now, and it's under the flag (`-analyzer-config
cfg-temporary-dtors=true`), so i guess i'll mark it as TODO and commit, and
i'll keep an eye on that when looking at real-world code analysis results.
Destructor fixes and tests are also coming in https://reviews.llvm.org/D43104.
https://reviews.llvm.org/D42876
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1,9 +1,10 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s -std=c++11
extern bool clang_analyzer_eval(bool);
extern bool clang_analyzer_warnIfReached();
+void clang_analyzer_checkInlined(bool);
struct Trivial {
Trivial(int x) : value(x) {}
@@ -422,6 +423,10 @@
struct CtorWithNoReturnDtor {
CtorWithNoReturnDtor() = default;
+CtorWithNoReturnDtor(int x) {
+ clang_analyzer_checkInlined(false); // no-warning
+}
+
~CtorWithNoReturnDtor() __attribute__((noreturn));
};
@@ -439,6 +444,12 @@
clang_analyzer_warnIfReached(); // no-warning
}
+#if __cplusplus >= 201103L
+ CtorWithNoReturnDtor returnNoReturnDtor() {
+return {1}; // no-crash
+ }
+#endif
+
#endif // TEMPORARY_DTORS
}
@@ -530,3 +541,152 @@
Sub(i).m();
}
}
+
+namespace test_return_temporary {
+class C {
+ int x, y;
+
+public:
+ C(int x, int y) : x(x), y(y) {}
+ int getX() const { return x; }
+ int getY() const { return y; }
+ ~C() {}
+};
+
+class D: public C {
+public:
+ D() : C(1, 2) {}
+ D(const D ): C(d.getX(), d.getY()) {}
+};
+
+C returnTemporaryWithVariable() { C c(1, 2); return c; }
+C returnTemporaryWithAnotherFunctionWithVariable() {
+ return returnTemporaryWithVariable();
+}
+C returnTemporaryWithCopyConstructionWithVariable() {
+ return C(returnTemporaryWithVariable());
+}
+
+C returnTemporaryWithConstruction() { return C(1, 2); }
+C returnTemporaryWithAnotherFunctionWithConstruction() {
+ return returnTemporaryWithConstruction();
+}
+C returnTemporaryWithCopyConstructionWithConstruction() {
+ return C(returnTemporaryWithConstruction());
+}
+
+D returnTemporaryWithVariableAndNonTrivialCopy() { D d; return d; }
+D returnTemporaryWithAnotherFunctionWithVariableAndNonTrivialCopy() {
+ return returnTemporaryWithVariableAndNonTrivialCopy();
+}
+D returnTemporaryWithCopyConstructionWithVariableAndNonTrivialCopy() {
+ return D(returnTemporaryWithVariableAndNonTrivialCopy());
+}
+
+#if __cplusplus >= 201103L
+C returnTemporaryWithBraces() { return {1, 2}; }
+C returnTemporaryWithAnotherFunctionWithBraces() {
+ return returnTemporaryWithBraces();
+}
+C returnTemporaryWithCopyConstructionWithBraces() {
+ return C(returnTemporaryWithBraces());
+}
+#endif // C++11
+
+void test() {
+ C c1 = returnTemporaryWithVariable();
+ clang_analyzer_eval(c1.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c1.getY() == 2); // expected-warning{{TRUE}}
+
+ C c2 = returnTemporaryWithAnotherFunctionWithVariable();
+ clang_analyzer_eval(c2.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c2.getY() == 2); // expected-warning{{TRUE}}
+
+ C c3 = returnTemporaryWithCopyConstructionWithVariable();
+ clang_analyzer_eval(c3.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c3.getY() == 2); // expected-warning{{TRUE}}
+
+ C c4 = returnTemporaryWithConstruction();
+ // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+ // in the inlined function is supported.
+ clang_analyzer_eval(c4.getX() == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(c4.getY() == 2); // expected-warning{{UNKNOWN}}
+
+ C c5 = returnTemporaryWithAnotherFunctionWithConstruction();
+ // Should be TRUE under TEMPORARY_DTORS once this sort of construction
+ // in the inlined function is supported.
+ clang_analyzer_eval(c5.getX() == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(c5.getY() == 2); // expected-warning{{UNKNOWN}}
+
+ C c6 = returnTemporaryWithCopyConstructionWithConstruction();
+ // Should be TRUE under TEMPORARY_DTORS
[PATCH] D42876: [analyzer] Support returning objects by value.
NoQ planned changes to this revision.
NoQ added a comment.
Destructor for the returned temporary is still evaluated conservatively:
class C {
public:
int ,
C(int &_x, int &_y) : x(_x), y(_y) { ++x; }
C(const C ) : x(c.x), y(c.y) { ++x; }
~C() { ++y; }
};
C make(int , int ) {
return { x, y };
}
void test() {
int x = 0, y = 0;
{
C c = make(x, y);
}
clang_analyzer_dump(x); // 2 S32b
clang_analyzer_dump(y); // 1 S32b
}
Also there's a weird rebound at the call site - from the
callee-context-temporary to the caller-context-temporary - which is currently
implemented as a trivial copy and not a full-featured constructor call. I guess
there shouldn't be a copy here (because there is no constructor or assignment
operator), so i suspect we should directly construct into the caller context's
temporary for the call-expression.
These issues are known, i'm just pointing out that we'd need to tackle them to
make at least this simple example work correctly.
Repository:
rC Clang
https://reviews.llvm.org/D42876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42876: [analyzer] Support returning objects by value.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
NoQ added dependencies: D42875: [analyzer] Add construction context for
ReturnStmt., D42779: [analyzer] NFC: Make sure we don't ever inline the
constructor for which the temporary destructor is noreturn and missing..
NoQ added a dependency: D42721: [analyzer] NFC: Use construction contexts for
finding the target region for the construction..
Now that we've added the necessary information into the CFG in
https://reviews.llvm.org/D42875, we can use it to support returning objects by
value.
Unfortunately, we still do not support the constructions into temporaries which
are later copied via copy-constructors, which is common in statements like
`return Class()`. But we can still support objects constructed in a different
manner, say `return getObject()` or `return { 1, 2 }`. Also, we've always
supported `return Obj;` when `Obj` has a trivial copy constructor, but now we
support it even when the constructor is non-trivial.
This patch adds a facility to prevent the new approach from being activated
when temporary destructors are disabled, because we have a mechanism for
suppressing hilarious false positives that wouldn't work in this case - see
https://reviews.llvm.org/D42779. Namely, i'm adding a new `EvalCallOption` that
will indicate that we're constructing a temporary object.
Repository:
rC Clang
https://reviews.llvm.org/D42876
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1,9 +1,10 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true %s -std=c++11
extern bool clang_analyzer_eval(bool);
extern bool clang_analyzer_warnIfReached();
+void clang_analyzer_checkInlined(bool);
struct Trivial {
Trivial(int x) : value(x) {}
@@ -422,6 +423,10 @@
struct CtorWithNoReturnDtor {
CtorWithNoReturnDtor() = default;
+CtorWithNoReturnDtor(int x) {
+ clang_analyzer_checkInlined(false); // no-warning
+}
+
~CtorWithNoReturnDtor() __attribute__((noreturn));
};
@@ -439,6 +444,12 @@
clang_analyzer_warnIfReached(); // no-warning
}
+#if __cplusplus >= 201103L
+ CtorWithNoReturnDtor returnNoReturnDtor() {
+return {1}; // no-crash
+ }
+#endif
+
#endif // TEMPORARY_DTORS
}
@@ -530,3 +541,152 @@
Sub(i).m();
}
}
+
+namespace test_return_temporary {
+class C {
+ int x, y;
+
+public:
+ C(int x, int y) : x(x), y(y) {}
+ int getX() const { return x; }
+ int getY() const { return y; }
+ ~C() {}
+};
+
+class D: public C {
+public:
+ D() : C(1, 2) {}
+ D(const D ): C(d.getX(), d.getY()) {}
+};
+
+C returnTemporaryWithVariable() { C c(1, 2); return c; }
+C returnTemporaryWithAnotherFunctionWithVariable() {
+ return returnTemporaryWithVariable();
+}
+C returnTemporaryWithCopyConstructionWithVariable() {
+ return C(returnTemporaryWithVariable());
+}
+
+C returnTemporaryWithConstruction() { return C(1, 2); }
+C returnTemporaryWithAnotherFunctionWithConstruction() {
+ return returnTemporaryWithConstruction();
+}
+C returnTemporaryWithCopyConstructionWithConstruction() {
+ return C(returnTemporaryWithConstruction());
+}
+
+D returnTemporaryWithVariableAndNonTrivialCopy() { D d; return d; }
+D returnTemporaryWithAnotherFunctionWithVariableAndNonTrivialCopy() {
+ return returnTemporaryWithVariableAndNonTrivialCopy();
+}
+D returnTemporaryWithCopyConstructionWithVariableAndNonTrivialCopy() {
+ return D(returnTemporaryWithVariableAndNonTrivialCopy());
+}
+
+#if __cplusplus >= 201103L
+C returnTemporaryWithBraces() { return {1, 2}; }
+C returnTemporaryWithAnotherFunctionWithBraces() {
+ return returnTemporaryWithBraces();
+}
+C returnTemporaryWithCopyConstructionWithBraces() {
+ return C(returnTemporaryWithBraces());
+}
+#endif // C++11
+
+void test() {
+ C c1 = returnTemporaryWithVariable();
+ clang_analyzer_eval(c1.getX() == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(c1.getY() == 2); // expected-warning{{TRUE}}
+
+ C c2 = returnTemporaryWithAnotherFunctionWithVariable();
+
