[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC328893: [CFG] [analyzer] Avoid modeling C++17 constructors
that arent fully supported. (authored by dergachev, committed by ).
Repository:
rL LLVM
https://reviews.llvm.org/D44854
Files:
lib/Analysis/CFG.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/cxx17-mandatory-elision.cpp
test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -108,6 +108,9 @@
C c = C::get();
}
+// FIXME: Find construction contexts for both branches in C++17.
+// Note that once it gets detected, the test for the get() branch would not
+// fail, because FileCheck allows partial matches.
// CHECK: void simpleVariableWithTernaryOperator(bool coin)
// CHECK:[B1]
// CXX11-NEXT: 1: [B4.2] ? [B2.5] : [B3.6]
@@ -122,15 +125,15 @@
// CXX11-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B2.4])
// CXX11-NEXT: 4: [B2.3]
// CXX11-NEXT: 5: [B2.4] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B1.2])
+// CXX17-NEXT: 3: [B2.2]()
// CHECK:[B3]
// CHECK-NEXT: 1: 0
// CHECK-NEXT: 2: [B3.1] (ImplicitCastExpr, NullToPointer, class C *)
// CXX11-NEXT: 3: [B3.2] (CXXConstructExpr, [B3.5], class C)
// CXX11-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CXX11-NEXT: 5: [B3.4]
// CXX11-NEXT: 6: [B3.5] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B3.2] (CXXConstructExpr, [B1.2], class C)
+// CXX17-NEXT: 3: [B3.2] (CXXConstructExpr, class C)
// CXX17-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CHECK:[B4]
// CHECK-NEXT: 1: coin
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+template struct AddressVector {
+ T *buf[10];
+ int len;
+
+ AddressVector() : len(0) {}
+
+ void push(T *t) {
+buf[len] = t;
+++len;
+ }
+};
+
+class ClassWithoutDestructor {
+ AddressVector
+
+public:
+ ClassWithoutDestructor(AddressVector ) : v(v) {
+v.push(this);
+ }
+
+ ClassWithoutDestructor(ClassWithoutDestructor &) : v(c.v) { v.push(this); }
+ ClassWithoutDestructor(const ClassWithoutDestructor ) : v(c.v) {
+v.push(this);
+ }
+};
+
+ClassWithoutDestructor make1(AddressVector ) {
+ return ClassWithoutDestructor(v);
+}
+ClassWithoutDestructor make2(AddressVector ) {
+ return make1(v);
+}
+ClassWithoutDestructor make3(AddressVector ) {
+ return make2(v);
+}
+
+void testMultipleReturns() {
+ AddressVector v;
+ ClassWithoutDestructor c = make3(v);
+
+#if __cplusplus >= 201703L
+ // FIXME: Both should be TRUE.
+ clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == ); // expected-warning{{FALSE}}
+#else
+ clang_analyzer_eval(v.len == 5); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[4] == ); // expected-warning{{TRUE}}
+#endif
+}
Index: test/Analysis/temp-obj-dtors-cfg-output.cpp
===
--- test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -218,6 +218,9 @@
// Don't try to figure out how to perform construction of the record here.
const C () { return foo1(); } // no-crash
C &() { return foo2(); } // no-crash
+const C (bool coin) {
+ return coin ? foo1() : foo1(); // no-crash
+}
} // end namespace pass_references_through
// CHECK: [B1 (ENTRY)]
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -203,13 +203,24 @@
// TODO: What exactly happens when we are? Does the temporary object live
// long enough in the region store in this case? Would checkers think
// that this object immediately goes out of scope?
- // TODO: We assume that the call site has a temporary object construction
- // context. This is no longer true in C++17 or when copy elision is
-
[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL328893: [CFG] [analyzer] Avoid modeling C++17 constructors
that arent fully supported. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D44854?vs=140168=140469#toc
Repository:
rL LLVM
https://reviews.llvm.org/D44854
Files:
cfe/trunk/lib/Analysis/CFG.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
cfe/trunk/test/Analysis/cxx17-mandatory-elision.cpp
cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -203,13 +203,24 @@
// TODO: What exactly happens when we are? Does the temporary object live
// long enough in the region store in this case? Would checkers think
// that this object immediately goes out of scope?
- // TODO: We assume that the call site has a temporary object construction
- // context. This is no longer true in C++17 or when copy elision is
- // performed. We may need to unwrap multiple stack frames here and we
- // won't necessarily end up with a temporary at the end.
const LocationContext *TempLCtx = LCtx;
- if (const LocationContext *CallerLCtx =
- LCtx->getCurrentStackFrame()->getParent()) {
+ const StackFrameContext *SFC = LCtx->getCurrentStackFrame();
+ if (const LocationContext *CallerLCtx = SFC->getParent()) {
+auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
+ .getAs();
+if (!RTC) {
+ // We were unable to find the correct construction context for the
+ // call in the parent stack frame. This is equivalent to not being
+ // able to find construction context at all.
+ CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+} else if (!isa(
+ RTC->getConstructionContext())) {
+ // FXIME: The return value is constructed directly into a
+ // non-temporary due to C++17 mandatory copy elision. This is not
+ // implemented yet.
+ assert(getContext().getLangOpts().CPlusPlus17);
+ CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+}
TempLCtx = CallerLCtx;
}
CallOpts.IsTemporaryCtorOrDtor = true;
Index: cfe/trunk/lib/Analysis/CFG.cpp
===
--- cfe/trunk/lib/Analysis/CFG.cpp
+++ cfe/trunk/lib/Analysis/CFG.cpp
@@ -1302,6 +1302,15 @@
}
case Stmt::ConditionalOperatorClass: {
auto *CO = cast(Child);
+if (!dyn_cast_or_null(Layer->getTriggerStmt())) {
+ // If the object returned by the conditional operator is not going to be a
+ // temporary object that needs to be immediately materialized, then
+ // it must be C++17 with its mandatory copy elision. Do not yet promise
+ // to support this case.
+ assert(!CO->getType()->getAsCXXRecordDecl() || CO->isGLValue() ||
+ Context->getLangOpts().CPlusPlus17);
+ break;
+}
findConstructionContexts(Layer, CO->getLHS());
findConstructionContexts(Layer, CO->getRHS());
break;
Index: cfe/trunk/test/Analysis/cxx17-mandatory-elision.cpp
===
--- cfe/trunk/test/Analysis/cxx17-mandatory-elision.cpp
+++ cfe/trunk/test/Analysis/cxx17-mandatory-elision.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+template struct AddressVector {
+ T *buf[10];
+ int len;
+
+ AddressVector() : len(0) {}
+
+ void push(T *t) {
+buf[len] = t;
+++len;
+ }
+};
+
+class ClassWithoutDestructor {
+ AddressVector
+
+public:
+ ClassWithoutDestructor(AddressVector ) : v(v) {
+v.push(this);
+ }
+
+ ClassWithoutDestructor(ClassWithoutDestructor &) : v(c.v) { v.push(this); }
+ ClassWithoutDestructor(const ClassWithoutDestructor ) : v(c.v) {
+v.push(this);
+ }
+};
+
+ClassWithoutDestructor make1(AddressVector ) {
+ return ClassWithoutDestructor(v);
+}
+ClassWithoutDestructor make2(AddressVector ) {
+ return make1(v);
+}
+ClassWithoutDestructor make3(AddressVector ) {
+ return make2(v);
+}
+
+void testMultipleReturns() {
+ AddressVector v;
+ ClassWithoutDestructor c = make3(v);
+
+#if __cplusplus >= 201703L
+ // FIXME: Both should be TRUE.
+ clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == ); // expected-warning{{FALSE}}
+#else
+ clang_analyzer_eval(v.len == 5); //
[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:215 + // able to find construction context at all. + CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true; +} else if (!isa( george.karpenkov wrote: > Is this field `false` by default? Also, double negation is harder to read > (e.g. `not modelled properly = false` vs. `modelled property = true`), but I > guess that should have been said earlier. Yeah, flags within `EvalCallOptions` are all "red" flags to warn `evalCall()` that something is fishy about the call. They're all off by default. I'm not sure what's the best way to change that. I don't want each branch to set like 4 different flags, so they should be non-"red" by default. https://reviews.llvm.org/D44854 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
george.karpenkov accepted this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. LGTM provided comments are answered. Field rename would be appreciated, if possible. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:215 + // able to find construction context at all. + CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true; +} else if (!isa( Is this field `false` by default? Also, double negation is harder to read (e.g. `not modelled properly = false` vs. `modelled property = true`), but I guess that should have been said earlier. https://reviews.llvm.org/D44854 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
NoQ updated this revision to Diff 140168.
NoQ added a comment.
Fix a comment in tests.
https://reviews.llvm.org/D44854
Files:
lib/Analysis/CFG.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/cxx17-mandatory-elision.cpp
test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: test/Analysis/temp-obj-dtors-cfg-output.cpp
===
--- test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -218,6 +218,9 @@
// Don't try to figure out how to perform construction of the record here.
const C () { return foo1(); } // no-crash
C &() { return foo2(); } // no-crash
+const C (bool coin) {
+ return coin ? foo1() : foo1(); // no-crash
+}
} // end namespace pass_references_through
// CHECK: [B1 (ENTRY)]
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- /dev/null
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+template struct AddressVector {
+ T *buf[10];
+ int len;
+
+ AddressVector() : len(0) {}
+
+ void push(T *t) {
+buf[len] = t;
+++len;
+ }
+};
+
+class ClassWithoutDestructor {
+ AddressVector
+
+public:
+ ClassWithoutDestructor(AddressVector ) : v(v) {
+v.push(this);
+ }
+
+ ClassWithoutDestructor(ClassWithoutDestructor &) : v(c.v) { v.push(this); }
+ ClassWithoutDestructor(const ClassWithoutDestructor ) : v(c.v) {
+v.push(this);
+ }
+};
+
+ClassWithoutDestructor make1(AddressVector ) {
+ return ClassWithoutDestructor(v);
+}
+ClassWithoutDestructor make2(AddressVector ) {
+ return make1(v);
+}
+ClassWithoutDestructor make3(AddressVector ) {
+ return make2(v);
+}
+
+void testMultipleReturns() {
+ AddressVector v;
+ ClassWithoutDestructor c = make3(v);
+
+#if __cplusplus >= 201703L
+ // FIXME: Both should be TRUE.
+ clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == ); // expected-warning{{FALSE}}
+#else
+ clang_analyzer_eval(v.len == 5); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[4] == ); // expected-warning{{TRUE}}
+#endif
+}
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -108,6 +108,9 @@
C c = C::get();
}
+// FIXME: Find construction contexts for both branches in C++17.
+// Note that once it gets detected, the test for the get() branch would not
+// fail, because FileCheck allows partial matches.
// CHECK: void simpleVariableWithTernaryOperator(bool coin)
// CHECK:[B1]
// CXX11-NEXT: 1: [B4.2] ? [B2.5] : [B3.6]
@@ -122,15 +125,15 @@
// CXX11-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B2.4])
// CXX11-NEXT: 4: [B2.3]
// CXX11-NEXT: 5: [B2.4] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B1.2])
+// CXX17-NEXT: 3: [B2.2]()
// CHECK:[B3]
// CHECK-NEXT: 1: 0
// CHECK-NEXT: 2: [B3.1] (ImplicitCastExpr, NullToPointer, class C *)
// CXX11-NEXT: 3: [B3.2] (CXXConstructExpr, [B3.5], class C)
// CXX11-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CXX11-NEXT: 5: [B3.4]
// CXX11-NEXT: 6: [B3.5] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B3.2] (CXXConstructExpr, [B1.2], class C)
+// CXX17-NEXT: 3: [B3.2] (CXXConstructExpr, class C)
// CXX17-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CHECK:[B4]
// CHECK-NEXT: 1: coin
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -203,13 +203,24 @@
// TODO: What exactly happens when we are? Does the temporary object live
// long enough in the region store in this case? Would checkers think
// that this object immediately goes out of scope?
- // TODO: We assume that the call site has a temporary object construction
- // context. This is no longer true in C++17 or when copy elision is
- // performed. We may need to unwrap multiple stack frames here and we
- // won't necessarily end up with a temporary at the end.
const LocationContext *TempLCtx = LCtx;
-
[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
NoQ updated this revision to Diff 140167.
NoQ added a comment.
Conditional operators, like call-expressions in
https://reviews.llvm.org/D44273, may also be glvalues of record type instead of
simply being reference-typed.
https://reviews.llvm.org/D44854
Files:
lib/Analysis/CFG.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/cxx17-mandatory-elision.cpp
test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: test/Analysis/temp-obj-dtors-cfg-output.cpp
===
--- test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -218,6 +218,10 @@
// Don't try to figure out how to perform construction of the record here.
const C () { return foo1(); } // no-crash
C &() { return foo2(); } // no-crash
+
+const C (bool coin) {
+ return coin ? foo1() : foo1();
+}
} // end namespace pass_references_through
// CHECK: [B1 (ENTRY)]
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- /dev/null
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+template struct AddressVector {
+ T *buf[10];
+ int len;
+
+ AddressVector() : len(0) {}
+
+ void push(T *t) {
+buf[len] = t;
+++len;
+ }
+};
+
+class ClassWithoutDestructor {
+ AddressVector
+
+public:
+ ClassWithoutDestructor(AddressVector ) : v(v) {
+v.push(this);
+ }
+
+ ClassWithoutDestructor(ClassWithoutDestructor &) : v(c.v) { v.push(this); }
+ ClassWithoutDestructor(const ClassWithoutDestructor ) : v(c.v) {
+v.push(this);
+ }
+};
+
+ClassWithoutDestructor make1(AddressVector ) {
+ return ClassWithoutDestructor(v);
+}
+ClassWithoutDestructor make2(AddressVector ) {
+ return make1(v);
+}
+ClassWithoutDestructor make3(AddressVector ) {
+ return make2(v);
+}
+
+void testMultipleReturns() {
+ AddressVector v;
+ ClassWithoutDestructor c = make3(v);
+
+#if __cplusplus >= 201703L
+ // FIXME: Both should be TRUE.
+ clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == ); // expected-warning{{FALSE}}
+#else
+ clang_analyzer_eval(v.len == 5); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[4] == ); // expected-warning{{TRUE}}
+#endif
+}
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -108,6 +108,9 @@
C c = C::get();
}
+// FIXME: Find construction contexts for both branches in C++17.
+// Note that once it gets detected, the test for the get() branch would not
+// fail, because FileCheck allows partial matches.
// CHECK: void simpleVariableWithTernaryOperator(bool coin)
// CHECK:[B1]
// CXX11-NEXT: 1: [B4.2] ? [B2.5] : [B3.6]
@@ -122,15 +125,15 @@
// CXX11-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B2.4])
// CXX11-NEXT: 4: [B2.3]
// CXX11-NEXT: 5: [B2.4] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B1.2])
+// CXX17-NEXT: 3: [B2.2]()
// CHECK:[B3]
// CHECK-NEXT: 1: 0
// CHECK-NEXT: 2: [B3.1] (ImplicitCastExpr, NullToPointer, class C *)
// CXX11-NEXT: 3: [B3.2] (CXXConstructExpr, [B3.5], class C)
// CXX11-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CXX11-NEXT: 5: [B3.4]
// CXX11-NEXT: 6: [B3.5] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B3.2] (CXXConstructExpr, [B1.2], class C)
+// CXX17-NEXT: 3: [B3.2] (CXXConstructExpr, class C)
// CXX17-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CHECK:[B4]
// CHECK-NEXT: 1: coin
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -203,13 +203,24 @@
// TODO: What exactly happens when we are? Does the temporary object live
// long enough in the region store in this case? Would checkers think
// that this object immediately goes out of scope?
- // TODO: We assume that the call site has a temporary object construction
- // context. This is no longer true in C++17 or when copy elision is
- // performed. We may need to unwrap multiple stack frames
[PATCH] D44854: [analyzer] Be more careful about C++17 copy elision.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
Just wanted to play safer in a couple of places.
In https://reviews.llvm.org/D44597 i said:
> If the object has trivial destructor then CXXBindTemporaryExpr is not there,
> and the AST is pretty much indistinguishable from the simple case so we
> re-use SimpleVariableConstructionContext and
> SimpleReturnedValueConstructionContext. I'm not entirely sure that this is
> indeed the same case, but for the purposes of the analyzer, which is the
> primary user of construction contexts, this seems fine because when the
> object has trivial destructor it is not scary to inline the constructor.
Well, this is actually an easy question. There certainly are cases where the
AST is quite distinguishable, eg. ternary conditional operator. However,
because construction contexts are implemented as a whitelist of possible ASTs
that we support, it's easy to see that the ternary operator is in fact the only
case. The patch suppresses construction contexts in this case for now, because
the example from http://lists.llvm.org/pipermail/cfe-dev/2018-March/057357.html
made me nervous (even though this particular example works well when there are
no destructors involved, so this is a speculative play-safe thingy without an
actual analyzer-based test).
Also raise the improperly-constructed flag for the return value construction
context into a non-temporary. This also has no effect on the analyzer because
we'd inline the constructor anyway. Additionally i added a test case to
demonstrate how this is broken.
Repository:
rC Clang
https://reviews.llvm.org/D44854
Files:
lib/Analysis/CFG.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/cxx17-mandatory-elision.cpp
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- /dev/null
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+template struct AddressVector {
+ T *buf[10];
+ int len;
+
+ AddressVector() : len(0) {}
+
+ void push(T *t) {
+buf[len] = t;
+++len;
+ }
+};
+
+class ClassWithoutDestructor {
+ AddressVector
+
+public:
+ ClassWithoutDestructor(AddressVector ) : v(v) {
+v.push(this);
+ }
+
+ ClassWithoutDestructor(ClassWithoutDestructor &) : v(c.v) { v.push(this); }
+ ClassWithoutDestructor(const ClassWithoutDestructor ) : v(c.v) {
+v.push(this);
+ }
+};
+
+ClassWithoutDestructor make1(AddressVector ) {
+ return ClassWithoutDestructor(v);
+}
+ClassWithoutDestructor make2(AddressVector ) {
+ return make1(v);
+}
+ClassWithoutDestructor make3(AddressVector ) {
+ return make2(v);
+}
+
+void testMultipleReturns() {
+ AddressVector v;
+ ClassWithoutDestructor c = make3(v);
+
+#if __cplusplus >= 201703L
+ // FIXME: Both should be TRUE.
+ clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == ); // expected-warning{{FALSE}}
+#else
+ clang_analyzer_eval(v.len == 5); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] != v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[2] != v.buf[3]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[3] != v.buf[4]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[4] == ); // expected-warning{{TRUE}}
+#endif
+}
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -108,6 +108,9 @@
C c = C::get();
}
+// FIXME: Find construction contexts for both branches in C++17.
+// Note that once it gets detected, the test for the get() branch would not
+// fail, because FileCheck allows partial matches.
// CHECK: void simpleVariableWithTernaryOperator(bool coin)
// CHECK:[B1]
// CXX11-NEXT: 1: [B4.2] ? [B2.5] : [B3.6]
@@ -122,15 +125,15 @@
// CXX11-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B2.4])
// CXX11-NEXT: 4: [B2.3]
// CXX11-NEXT: 5: [B2.4] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT: 3: [B2.2]() (CXXRecordTypedCall, [B1.2])
+// CXX17-NEXT: 3: [B2.2]()
// CHECK:[B3]
// CHECK-NEXT: 1: 0
// CHECK-NEXT: 2: [B3.1] (ImplicitCastExpr, NullToPointer, class C *)
// CXX11-NEXT: 3: [B3.2] (CXXConstructExpr, [B3.5], class C)
// CXX11-NEXT: 4: C([B3.3]) (CXXFunctionalCastExpr, ConstructorConversion, class C)
// CXX11-NEXT: 5: [B3.4]
// CXX11-NEXT: 6: [B3.5] (CXXConstructExpr, [B1.2], class C)
-// CXX17-NEXT:
