[analyzer] Implement path notes for temporary destructors.

[NoQ]

Summary

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.

[NoQ]

Minor indent fix.

[george.karpenkov]

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

586 ↗

(On Diff #133687)

auto?

test/Analysis/inlining/temp-dtors-path-notes.cpp

17 ↗

(On Diff #133687)

Should we have "returning from ~C"? At least we do have it for inlined functions.

[NoQ]

test/Analysis/inlining/temp-dtors-path-notes.cpp

17 ↗

(On Diff #133687)

Not here, because the bug occurs within ~C. But we should definitely test it, yeah.

[NoQ]

Another question is why do we have such inconsistency between Calling constructor for 'C' and Calling '~C', i.e. why not Calling destructor for 'C'. Seems accidental.

[NoQ]

Add a test for returning from destructor.

cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

case CFGElement::MemberDtor: {
     if (const Stmt *CallerBody = CallerInfo->getBody())
       return PathDiagnosticLocation::createEnd(CallerBody, SM, CallerCtx);
     return PathDiagnosticLocation::create(CallerInfo->getDecl(), SM);
   }
   case CFGElement::NewAllocator: {
     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 auto &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!");
   }
 
   llvm_unreachable("Unknown CFGElement kind");
 }
 

cfe/trunk/test/Analysis/inlining/temp-dtors-path-notes.cpp

+// 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@-1{{Division by zero}}
+};
+
+void test() {
+  C(0); // expected-note   {{Passing the value 0 via 1st parameter 'x'}}
+        // expected-note@-1{{Calling constructor for 'C'}}
+        // expected-note@-2{{Returning from constructor for 'C'}}
+        // expected-note@-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@-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@-1{{'?' condition is false}}
+                                   // expected-note@-2{{Passing the value 0 via 1st parameter 'x'}}
+                                   // expected-note@-3{{Calling constructor for 'C'}}
+                                   // expected-note@-4{{Returning from constructor for 'C'}}
+                                   // expected-note@-5{{Calling '~C'}}
+  c.nop();
+}
+} // end namespace test_lifetime_extended_temporary
+
+namespace test_bug_after_dtor {
+int glob;
+
+class C {
+public:
+  C() { glob += 1; }
+  ~C() { glob -= 2; } // expected-note{{The value 0 is assigned to 'glob'}}
+};
+
+void test() {
+  glob = 1;
+  C(); // expected-note   {{Calling '~C'}}
+       // expected-note@-1{{Returning from '~C'}}
+  glob = 1 / glob; // expected-warning{{Division by zero}}
+                   // expected-note@-1{{Division by zero}}
+}
+} // end namespace test_bug_after_dtor