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@-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
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!");


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@-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
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!");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to