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