[PATCH] D43144: [analyzer] Implement path notes for temporary destructors.

[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.

Temporaries are destroyed at the end of their `CXXBindTemporaryExpr`, which can be picked up from their `CFGTemporaryDtor`. Note that lifetime-extended temporaries are different: they are destroyed via `CFGAutomaticObjectDtor` for their reference, and they don't have a `CFGTemoraryDtor` of their own. Unless, well, they are copied with an elidable copy-constructor, in which case the temporary destructor is there, but it still fires immediately, and lifetime-extended copy is still dealt with via an automatic destructor.

I think this is one of the places where diagnostics may become confusing due to presence of elidable constructors and their respective destructors. In the second test case nobody would expect anything to be destroyed on the line on which we show the "Calling '~C'" note. So we may need to either think of actually eliding elidable constructors together with their destructors, or at least explaining to the user that the constructor is elidable as part of the path note.


Repository:
  rC Clang

https://reviews.llvm.org/D43144

Files:
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/inlining/temp-dtors-path-notes.cpp


Index: test/Analysis/inlining/temp-dtors-path-notes.cpp
===================================================================
--- /dev/null
+++ test/Analysis/inlining/temp-dtors-path-notes.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_analyze_cc1 -analyze -analyzer-checker core -analyzer-config cfg-temporary-dtors=true -analyzer-output=text -verify %s
+
+namespace test_simple_temporary {
+class C {
+  int x;
+
+public:
+  C(int x): x(x) {} // expected-note{{The value 0 is assigned to field 'x'}}
+  ~C() { x = 1 / x; } // expected-warning{{Division by zero}}
+                      // expected-note at -1{{Division by zero}}
+};
+
+void test() {
+  C(0); // expected-note   {{Passing the value 0 via 1st parameter 'x'}}
+        // expected-note at -1{{Calling constructor for 'C'}}
+        // expected-note at -2{{Returning from constructor for 'C'}}
+        // expected-note at -3{{Calling '~C'}}
+}
+} // end namespace test_simple_temporary
+
+namespace test_lifetime_extended_temporary {
+class C {
+  int x;
+
+public:
+  C(int x): x(x) {} // expected-note{{The value 0 is assigned to field 'x'}}
+  void nop() const {}
+  ~C() { x = 1 / x; } // expected-warning{{Division by zero}}
+                      // expected-note at -1{{Division by zero}}
+};
+
+void test(int coin) {
+  // We'd divide by zero in the temporary destructor that goes after the
+  // elidable copy-constructor from C(0) to the lifetime-extended temporary.
+  // So in fact this example has nothing to do with lifetime extension.
+  // Actually, it would probably be better to elide the constructor, and
+  // put the note for the destructor call at the closing brace after nop.
+  const C &c = coin ? C(1) : C(0); // expected-note{{Assuming 'coin' is 0}}
+                                   // expected-note at -1{{'?' condition is false}}
+                                   // expected-note at -2{{Passing the value 0 via 1st parameter 'x'}}
+                                   // expected-note at -3{{Calling constructor for 'C'}}
+                                   // expected-note at -4{{Returning from constructor for 'C'}}
+                                   // expected-note at -5{{Calling '~C'}}
+  c.nop();
+}
+} // end namespace test_lifetime_extended_temporary
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -579,8 +579,14 @@
     const CFGNewAllocator &Alloc = Source.castAs<CFGNewAllocator>();
     return PathDiagnosticLocation(Alloc.getAllocatorExpr(), SM, CallerCtx);
   }
-  case CFGElement::TemporaryDtor:
-    llvm_unreachable("not yet implemented!");
+  case CFGElement::TemporaryDtor: {
+    // Temporary destructors are for temporaries. They die immediately at around
+    // the location of CXXBindTemporaryExpr. If they are lifetime-extended,
+    // they'd be dealt with via an AutomaticObjectDtor instead.
+    const CFGTemporaryDtor &Dtor = Source.castAs<CFGTemporaryDtor>();
+    return PathDiagnosticLocation::createEnd(Dtor.getBindTemporaryExpr(), SM,
+                                             CallerCtx);
+  }
   case CFGElement::LifetimeEnds:
   case CFGElement::LoopExit:
     llvm_unreachable("CFGElement kind should not be on callsite!");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43144.133686.patch
Type: text/x-patch
Size: 3293 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180209/df1331cd/attachment.bin>