[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Phabricator via Phabricator via cfe-commits]

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 rL325282: [analyzer] Compute the correct this-region for 
temporary destructors. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43104?vs=134351=134468#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43104

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  cfe/trunk/test/Analysis/analyzer-config.cpp
  cfe/trunk/test/Analysis/temp-obj-dtors-option.cpp
  cfe/trunk/test/Analysis/temporaries.cpp

Index: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -250,6 +250,9 @@
   /// \sa mayInlineCXXSharedPtrDtor
   Optional InlineCXXSharedPtrDtor;
 
+  /// \sa mayInlineCXXTemporaryDtors
+  Optional InlineCXXTemporaryDtors;
+
   /// \sa mayInlineObjCMethod
   Optional ObjCInliningMode;
 
@@ -493,6 +496,17 @@
   /// accepts the values "true" and "false".
   bool mayInlineCXXSharedPtrDtor();
 
+  /// Returns true if C++ temporary destructors should be inlined during
+  /// analysis.
+  ///
+  /// If temporary destructors are disabled in the CFG via the
+  /// 'cfg-temporary-dtors' option, temporary destructors would not be
+  /// inlined anyway.
+  ///
+  /// This is controlled by the 'c++-temp-dtor-inlining' config option, which
+  /// accepts the values "true" and "false".
+  bool mayInlineCXXTemporaryDtors();
+
   /// Returns whether or not paths that go through null returns should be
   /// suppressed.
   ///
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -448,10 +448,6 @@
ExplodedNode *Pred,
ExplodedNodeSet );
 
-  void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
- ExplodedNodeSet ,
- ExplodedNodeSet );
-
   void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
  ExplodedNodeSet );
 
@@ -696,8 +692,22 @@
   /// IsConstructorWithImproperlyModeledTargetRegion flag is set in \p CallOpts.
   const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
  ExplodedNode *Pred,
+ const ConstructionContext *CC,
  EvalCallOptions );
 
+  /// Store the region of a C++ temporary object corresponding to a
+  /// CXXBindTemporaryExpr for later destruction.
+  static ProgramStateRef addInitializedTemporary(
+  ProgramStateRef State, const CXXBindTemporaryExpr *BTE,
+  const LocationContext *LC, const CXXTempObjectRegion *R);
+
+  /// Check if all initialized temporary regions are clear for the given
+  /// context range (including FromLC, not including ToLC).
+  /// This is useful for assertions.
+  static bool areInitializedTemporariesClear(ProgramStateRef State,
+ const LocationContext *FromLC,
+ const LocationContext *ToLC);
+
   /// Store the region returned by operator new() so that the constructor
   /// that follows it knew what location to initialize. The value should be
   /// cleared once the respective CXXNewExpr CFGStmt element is processed.
Index: cfe/trunk/test/Analysis/analyzer-config.cpp
===
--- cfe/trunk/test/Analysis/analyzer-config.cpp
+++ cfe/trunk/test/Analysis/analyzer-config.cpp
@@ -12,7 +12,9 @@
 
 class Foo {
 public:
-	void bar() {}
+  ~Foo() {}
+  void baz();
+	void bar() { const Foo  = Foo(); }
 	void foo() { bar(); }
 };
 
Index: cfe/trunk/test/Analysis/temp-obj-dtors-option.cpp
===
--- cfe/trunk/test/Analysis/temp-obj-dtors-option.cpp
+++ cfe/trunk/test/Analysis/temp-obj-dtors-option.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=false 

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Phabricator via Phabricator via cfe-commits]

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 rC325282: [analyzer] Compute the correct this-region for 
temporary destructors. (authored by dergachev, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/analyzer-config.cpp
  test/Analysis/temp-obj-dtors-option.cpp
  test/Analysis/temporaries.cpp

Index: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -250,6 +250,9 @@
   /// \sa mayInlineCXXSharedPtrDtor
   Optional InlineCXXSharedPtrDtor;
 
+  /// \sa mayInlineCXXTemporaryDtors
+  Optional InlineCXXTemporaryDtors;
+
   /// \sa mayInlineObjCMethod
   Optional ObjCInliningMode;
 
@@ -493,6 +496,17 @@
   /// accepts the values "true" and "false".
   bool mayInlineCXXSharedPtrDtor();
 
+  /// Returns true if C++ temporary destructors should be inlined during
+  /// analysis.
+  ///
+  /// If temporary destructors are disabled in the CFG via the
+  /// 'cfg-temporary-dtors' option, temporary destructors would not be
+  /// inlined anyway.
+  ///
+  /// This is controlled by the 'c++-temp-dtor-inlining' config option, which
+  /// accepts the values "true" and "false".
+  bool mayInlineCXXTemporaryDtors();
+
   /// Returns whether or not paths that go through null returns should be
   /// suppressed.
   ///
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -448,10 +448,6 @@
ExplodedNode *Pred,
ExplodedNodeSet );
 
-  void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *BTE,
- ExplodedNodeSet ,
- ExplodedNodeSet );
-
   void VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
  ExplodedNodeSet );
 
@@ -696,8 +692,22 @@
   /// IsConstructorWithImproperlyModeledTargetRegion flag is set in \p CallOpts.
   const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
  ExplodedNode *Pred,
+ const ConstructionContext *CC,
  EvalCallOptions );
 
+  /// Store the region of a C++ temporary object corresponding to a
+  /// CXXBindTemporaryExpr for later destruction.
+  static ProgramStateRef addInitializedTemporary(
+  ProgramStateRef State, const CXXBindTemporaryExpr *BTE,
+  const LocationContext *LC, const CXXTempObjectRegion *R);
+
+  /// Check if all initialized temporary regions are clear for the given
+  /// context range (including FromLC, not including ToLC).
+  /// This is useful for assertions.
+  static bool areInitializedTemporariesClear(ProgramStateRef State,
+ const LocationContext *FromLC,
+ const LocationContext *ToLC);
+
   /// Store the region returned by operator new() so that the constructor
   /// that follows it knew what location to initialize. The value should be
   /// cleared once the respective CXXNewExpr CFGStmt element is processed.
Index: test/Analysis/analyzer-config.cpp
===
--- test/Analysis/analyzer-config.cpp
+++ test/Analysis/analyzer-config.cpp
@@ -12,7 +12,9 @@
 
 class Foo {
 public:
-	void bar() {}
+  ~Foo() {}
+  void baz();
+	void bar() { const Foo  = Foo(); }
 	void foo() { bar(); }
 };
 
Index: test/Analysis/temp-obj-dtors-option.cpp
===
--- test/Analysis/temp-obj-dtors-option.cpp
+++ test/Analysis/temp-obj-dtors-option.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=false -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DINLINE -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct S {
+  int 
+
+  S(int ) : x(x) { ++x; }
+  ~S() { --x; }
+};
+
+void foo() {
+  int x = 0;
+  S(x).x += 1;
+  

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 134351.
NoQ added a comment.

Whoops - recall that we still need to cleanup the temporaries map even, now 
that we know that we cannot assert that it's already empty. Clean up the map 
and enable the assertion that becomes tautological until the respective FIXME 
is fixed.


https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/analyzer-config.cpp
  test/Analysis/temp-obj-dtors-option.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -779,3 +779,116 @@
 }
 
 } // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+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; }
+};
+
+void test_simple_temporary() {
+  int x = 0, y = 0;
+  {
+const C  = C(x, y);
+  }
+  // One constructor and one destructor.
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+  int x = 0, y = 0;
+  {
+C c = C(x, y);
+  }
+  // Two constructors (temporary object expr and copy) and two destructors.
+  clang_analyzer_eval(x == 2);
+  clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+const C  = coin ? C(x, y) : C(z, w);
+  }
+  // This time each branch contains an additional elidable copy constructor.
+  if (coin) {
+clang_analyzer_eval(x == 2);
+clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 2);
+clang_analyzer_eval(w == 2);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+
+void test_ternary_temporary_with_copy(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+C c = coin ? C(x, y) : C(z, w);
+  }
+  // Temporary expression, elidable copy within branch,
+  // constructor for variable - 3 total.
+  if (coin) {
+clang_analyzer_eval(x == 3);
+clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 3);
+clang_analyzer_eval(w == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+} // namespace test_match_constructors_and_destructors
Index: test/Analysis/temp-obj-dtors-option.cpp
===
--- /dev/null
+++ test/Analysis/temp-obj-dtors-option.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=false -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true -DINLINE -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct S {
+  int 
+
+  S(int ) : x(x) { ++x; }
+  ~S() { --x; }
+};
+
+void foo() {
+  int x = 0;
+  S(x).x += 1;
+  clang_analyzer_eval(x == 1);
+#ifdef INLINE
+  // expected-warning@-2{{TRUE}}
+#else
+  // expected-warning@-4{{UNKNOWN}}
+#endif
+}
Index: test/Analysis/analyzer-config.cpp

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 134341.
NoQ added a comment.

All right, so the assertion that we actually destroy all temporaries in our 
`InitializedTemporaries` map is still violated quite often -  every time we do 
any sort of lifetime extension, we're actually calling the destructor on a 
different object, because `createTemporaryRegionIfNeeded()` has moved the 
object to a different place. I made a large brain dump on this matter in 
http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html . For now it 
means that the assertion is still not going in. I added it in a commented-out 
form. In fact, @klimek has tried that long before me, and failed in a similar 
manner. If only i added the assertion in the right place (not in 
`processCallExit`, but in `processEndOfFunction`, which is also called for the 
top frame), i would have seen it earlier :) So i've reinvented quite a bit of a 
wheel here.

- Move the assertion to the right place and disable it, explaining that 
lifetime extension is broken.
- Move the similar operator-new assertion to the right place as well, while 
we're at it.
- Add a flag to disable inlining of temporary destructors, which is different 
from having them at all. It's on by default but does nothing until 
`cfg-temporary-dtors` are also enabled.
- Add a test for that flag. This flag cannot be tested in `analyzer-config.cpp` 
because it is never read unless `cfg-temporary-dtors` takes a non-default 
value, so `ConfigDumper` doesn't see it.


https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/analyzer-config.cpp
  test/Analysis/temp-obj-dtors-option.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -779,3 +779,116 @@
 }
 
 } // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+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; }
+};
+
+void test_simple_temporary() {
+  int x = 0, y = 0;
+  {
+const C  = C(x, y);
+  }
+  // One constructor and one destructor.
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+  int x = 0, y = 0;
+  {
+C c = C(x, y);
+  }
+  // Two constructors (temporary object expr and copy) and two destructors.
+  clang_analyzer_eval(x == 2);
+  clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+const C  = coin ? C(x, y) : C(z, w);
+  }
+  // This time each branch contains an additional elidable copy constructor.
+  if (coin) {
+clang_analyzer_eval(x == 2);
+clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 2);
+clang_analyzer_eval(w == 2);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+
+void test_ternary_temporary_with_copy(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+C c = coin ? C(x, y) : C(z, w);
+  }
+  // Temporary expression, elidable copy within branch,
+  // constructor for variable - 3 total.
+  if (coin) {
+clang_analyzer_eval(x == 3);
+clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // 

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 133956.
NoQ added a comment.

- Test `const C  = coin ? C(x, y) : C(z, w);`.
- Fix comments surrounding the assertion.


https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  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
@@ -779,3 +779,116 @@
 }
 
 } // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+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; }
+};
+
+void test_simple_temporary() {
+  int x = 0, y = 0;
+  {
+const C  = C(x, y);
+  }
+  // One constructor and one destructor.
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+  int x = 0, y = 0;
+  {
+C c = C(x, y);
+  }
+  // Two constructors (temporary object expr and copy) and two destructors.
+  clang_analyzer_eval(x == 2);
+  clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+const C  = coin ? C(x, y) : C(z, w);
+  }
+  // This time each branch contains an additional elidable copy constructor.
+  if (coin) {
+clang_analyzer_eval(x == 2);
+clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 2);
+clang_analyzer_eval(w == 2);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+
+void test_ternary_temporary_with_copy(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+C c = coin ? C(x, y) : C(z, w);
+  }
+  // Temporary expression, elidable copy within branch,
+  // constructor for variable - 3 total.
+  if (coin) {
+clang_analyzer_eval(x == 3);
+clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 3);
+clang_analyzer_eval(w == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+} // namespace test_match_constructors_and_destructors
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -332,6 +332,8 @@
 
 // See if we have any stale C++ allocator values.
 assert(areCXXNewAllocatorValuesClear(CEEState, calleeCtx, callerCtx));
+// See if we have any stale C++ temporary object values.
+assert(areInitializedTemporariesClear(CEEState, calleeCtx, callerCtx));
 
 ExplodedNode *CEENode = G.getNode(Loc, CEEState, false, );
 CEENode->addPredecessor(*I, G);
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -102,14 +102,15 @@
 const MemRegion *
 ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
   ExplodedNode *Pred,
+  const ConstructionContext *CC,
   EvalCallOptions ) {
   const LocationContext *LCtx = 

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 133672.
NoQ marked an inline comment as done.
NoQ added a comment.

Assert that the destructors are cleaned up. This assertion is pretty important 
because it says that we didn't miss any destructors for initialized temporaries 
during analysis.

Disable tracking initialized temporaries when `cfg-temporary-dtors` is off - a 
bug i introduced when moving the code, which was found by the newly added 
assertion :)


https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  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
@@ -779,3 +779,81 @@
 }
 
 } // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+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; }
+};
+
+void test_simple_temporary() {
+  int x = 0, y = 0;
+  {
+const C  = C(x, y);
+  }
+  // One constructor and one destructor.
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+  int x = 0, y = 0;
+  {
+C c = C(x, y);
+  }
+  // Two constructors (temporary object expr and copy) and two destructors.
+  clang_analyzer_eval(x == 2);
+  clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+C c = coin ? C(x, y) : C(z, w);
+  }
+  // This time each branch contains an additional elidable copy constructor.
+  if (coin) {
+clang_analyzer_eval(x == 3);
+clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 3);
+clang_analyzer_eval(w == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+} // namespace test_match_constructors_and_destructors
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -332,6 +332,7 @@
 
 // See if we have any stale C++ allocator values.
 assert(areCXXNewAllocatorValuesClear(CEEState, calleeCtx, callerCtx));
+assert(areInitializedTemporariesClear(CEEState, calleeCtx, callerCtx));
 
 ExplodedNode *CEENode = G.getNode(Loc, CEEState, false, );
 CEENode->addPredecessor(*I, G);
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -102,14 +102,15 @@
 const MemRegion *
 ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
   ExplodedNode *Pred,
+  const ConstructionContext *CC,
   EvalCallOptions ) {
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
   MemRegionManager  = getSValBuilder().getRegionManager();
 
   // See if we're constructing an existing region by looking at the
   // current construction context.
-  if (auto CC = getCurrentCFGElement().getAs()) {
+  if (CC) {
 if (const Stmt *TriggerStmt = CC->getTriggerStmt()) {
   if (const CXXNewExpr *CNE = dyn_cast(TriggerStmt)) {
 if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
@@ -217,10 +218,20 @@
   // the entire array).
 
   EvalCallOptions CallOpts;
+  auto C = getCurrentCFGElement().getAs();
+  const ConstructionContext *CC = C ? C->getConstructionContext() : nullptr;
+
+  const CXXBindTemporaryExpr *BTE = nullptr;
 
   switch (CE->getConstructionKind()) {
   case 

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:939
 std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame()));
+// *MRPtr may still be null when the construction context for the temporary
+// was not implemented.

Can we add an assertion somewhere else (like when leaving a stack frame 
context) to make sure that the expected temporaries have all been removed from 
InitializedTemporaries?


Repository:
  rC Clang

https://reviews.llvm.org/D43104



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:68-71
 typedef llvm::ImmutableMap
-CXXNewAllocatorValuesMap;
+ const LocationContext *>,
+   SVal>
+CXXNewAllocatorValuesMap;

For consistency.


Repository:
  rC Clang

https://reviews.llvm.org/D43104



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:68-71
 typedef llvm::ImmutableMap
-CXXNewAllocatorValuesMap;
+ const LocationContext *>,
+   SVal>
+CXXNewAllocatorValuesMap;

For consistency.


When a temporary is constructed into a `CXXBindTemporaryExpr` (i.e. when it 
needs a destructor), we can (since 
https://reviews.llvm.org/D43056/https://reviews.llvm.org/D43062) pick up the 
construction context and be sure that the temporary is initialized correctly.

Now, store the initialized temporary region in the program state with 
`CXXBindTemporaryExpr` as its key, and when the time comes to call the 
destructor, lookup the `CXXBindTemporaryExpr` in the `CFGTemporaryDtor` 
element, then lookup the region in the program state.

For this purpose, the existing program state trait is used - the one that was 
added by @klimek for making sure that the control flow for ternary operators 
with temporaries is correct. Now the region is also stuffed into it. 
Technically, it should be possible to reconstruct the region by having just the 
temporary-expression and the location context. However, this would require 
duplicating a bit of logic from the CFG.

When the this-region is available this way, give ourselves the privilege to 
attempt inlining it. Of course, normal limitations of what we will or will not 
inline will still apply.

Additionally, prevent the temporary from being garbage-collected from the 
program state until the destructor call. I don't think 
`SymRegion::isLiveRegion()` should be taught to query `ExprEngine` regarding 
live temporaries - we should be just fine treating it as a regular program 
state trait value liveness process. Also in this case `LiveVariables` analysis 
isn't going to help much: temporaries definitely outlive their 
`CXXBindTemporaryExpr`s sometimes. So i have an impression that this is the 
right way to solve the liveness issue this time.


Repository:
  rC Clang

https://reviews.llvm.org/D43104

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -779,3 +779,81 @@
 }
 
 } // namespace test_temporary_object_expr
+
+namespace test_match_constructors_and_destructors {
+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; }
+};
+
+void test_simple_temporary() {
+  int x = 0, y = 0;
+  {
+const C  = C(x, y);
+  }
+  // One constructor and one destructor.
+  clang_analyzer_eval(x == 1);
+  clang_analyzer_eval(y == 1);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_simple_temporary_with_copy() {
+  int x = 0, y = 0;
+  {
+C c = C(x, y);
+  }
+  // Two constructors (temporary object expr and copy) and two destructors.
+  clang_analyzer_eval(x == 2);
+  clang_analyzer_eval(y == 2);
+#ifdef TEMPORARY_DTORS
+  // expected-warning@-3{{TRUE}}
+  // expected-warning@-3{{TRUE}}
+#else
+  // expected-warning@-6{{UNKNOWN}}
+  // expected-warning@-6{{UNKNOWN}}
+#endif
+}
+
+void test_ternary_temporary(int coin) {
+  int x = 0, y = 0, z = 0, w = 0;
+  {
+C c = coin ? C(x, y) : C(z, w);
+  }
+  // This time each branch contains an additional elidable copy constructor.
+  if (coin) {
+clang_analyzer_eval(x == 3);
+clang_analyzer_eval(y == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(w == 0); // expected-warning{{TRUE}}
+
+  } else {
+clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(y == 0); // expected-warning{{TRUE}}
+clang_analyzer_eval(z == 3);
+clang_analyzer_eval(w == 3);
+#ifdef TEMPORARY_DTORS
+// expected-warning@-3{{TRUE}}
+// expected-warning@-3{{TRUE}}
+#else
+// expected-warning@-6{{UNKNOWN}}
+// expected-warning@-6{{UNKNOWN}}
+#endif
+  }
+}
+} // namespace test_match_constructors_and_destructors
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp