libstdc++'s unique_ptr destructor is currently implemented as: ~unique_ptr() noexcept { reset(); } This has the effect of resetting the stored pointer to null, and then invoking the deleter on the formerly-stored pointer. I believe this is inconsistent with the language standard, which specifies the destructor as "If get() == nullptr there are no effects. Otherwise get_deleter()(get())." (note no mention of any side effects on the value of the stored pointer). This is a problem because this implementation will break code that (legitimately, AFAICT) relies on being able to continue to invoke operations on the scoped_ptr while the destructor is executing. The fix is to reimplement the destructor (in both the base template and the array specialization) as ~unique_ptr() noexcept { if (__p != pointer()) get_deleter()(__p); } If the intent is to zero out __p to help catch use-after-destruction errors, I believe it would be permissible to set __p to null after the call to get_deleter(), because at that point the change would no longer be visible to conforming code. To make this concrete, here's an example: the following program prints "bad" under libstdc++, but I believe a standard-conforming implementation is required to print "good": ============================= #include <iostream> #include <memory> using std::cout; using std::endl; using std::unique_ptr; struct A; struct B { unique_ptr<A> a; }; struct A { B* b; ~A() { if (b->a == nullptr) { cout << "bad" << endl; } else { cout << "good" << endl; } } }; int main(int argc, char** argv) { B b; b.a.reset(new A); b.a->b = &b; } =============================== As a point of comparison, MSVC++ 2010 prints "good" on this example program.
Hmm, the behaviour was probalby correct prior to fixing PR 43183, as the old implementation of reset() did exactly what is required of the destructor. However, the lifetime of the unique_ptr ends when the destructor starts ([basic.life]/1) so I don't believe it is legitimate to access it at that point.
That said, whether the testcase is valid or not, I don't see any harm in making the change.
Looking more closely, [basic.life]/5 and [class.cdtor]/1 do seem to allow your testcase. So confirmed.
I'll test this change: @@ -169,7 +169,13 @@ #endif // Destructor. - ~unique_ptr() noexcept { reset(); } + ~unique_ptr() noexcept + { + auto& __ptr = std::get<0>(_M_t); + if (__ptr != pointer()) + get_deleter()(__ptr); + __ptr = pointer(); + } // Assignment. unique_ptr&
Don't forget the array specialization. Doesn't the first line of your new destructor shadow the __p member with a __p local variable? Why is that line needed at all?
(In reply to comment #5) > Don't forget the array specialization. I won't :-) > Doesn't the first line of your new destructor shadow the __p member with a __p > local variable? Why is that line needed at all? There is no __p member.
(In reply to comment #6) > (In reply to comment #5) > > Don't forget the array specialization. > > I won't :-) > > > Doesn't the first line of your new destructor shadow the __p member with a __p > > local variable? Why is that line needed at all? > > There is no __p member. Ah, sorry, I was evidently misreading something. I've corrected the bug description.
Author: redi Date: Sun Aug 26 00:12:40 2012 New Revision: 190676 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190676 Log: PR libstdc++/54351 * include/bits/unique_ptr.h (unique_ptr<T>::~unique_ptr): Do not use reset(). (unique_ptr<T[]>::~unique_ptr()): Likewise. * testsuite/20_util/unique_ptr/54351.cc: New. * testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error line numbers. Added: trunk/libstdc++-v3/testsuite/20_util/unique_ptr/54351.cc - copied, changed from r190674, trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/unique_ptr.h trunk/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
Author: redi Date: Sun Aug 26 00:29:41 2012 New Revision: 190681 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=190681 Log: 2012-08-26 Jonathan Wakely <jwakely.gcc@gmail.com> Geoff Romer <gromer@google.com> PR libstdc++/54351 * include/bits/unique_ptr.h (unique_ptr<T>::~unique_ptr): Do not use reset(). (unique_ptr<T[]>::~unique_ptr()): Likewise. * testsuite/20_util/unique_ptr/54351.cc: New. * testsuite/20_util/unique_ptr/assign/48635_neg.cc: Adjust dg-error line numbers. Added: branches/gcc-4_7-branch/libstdc++-v3/testsuite/20_util/unique_ptr/54351.cc - copied, changed from r190679, branches/gcc-4_7-branch/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc Modified: branches/gcc-4_7-branch/libstdc++-v3/ChangeLog branches/gcc-4_7-branch/libstdc++-v3/include/bits/unique_ptr.h branches/gcc-4_7-branch/libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
fixed for 4.7.2
From discussion on the C++ LWG reflector, it appears that the standard's requirements on library types are intended to apply only during their lifetime, although the standard does not currently make this clear. LWG 2224 (http://cplusplus.github.com/LWG/lwg-active.html#2224) is tracking this issue, and its resolution should give a definitive answer. That being the case, the old behavior was not a bug, but conformant with the the intent of the standard (if not the precise wording). The new behavior is conformant as well, of course, so it's up to you whether to revert the changes; I just wanted to document for future reference that ~unique_ptr is in fact permitted to modify the stored pointer before invoking the deleter, so this bug report should not be an obstacle to e.g. having ~unique_ptr store a poison value in order to catch client bugs.
~unique_ptr()'s specification doesn't say that it assigns to the stored pointer at any time. Since unique_ptr can be used with pointer-like things whose assignment operator can have observable side effects, is doing this assignment in the destructor conforming?
It isn't specified whether it is assigned to or not in the destructor, so I think it's conforming. D::pointer is required to meet the NullablePointer requirements, which includes CopyAssignable, so in the absence of any restrictions to the contrary the implementation can rely on assignment (and typically both release() and reset() will assign to the pointer, and reset() is a valid implementation of the destructor).
Well, I would have argued that if the specification doesn't say that a function does X, then it doesn't do X. NullablePointer/CopyAssignable only means that the assignment operation must be supported. But then I realized that a literal reading of the specification would mean that the deleter must be called using a copy of the stored pointer - the return value of get() - rather than the stored pointer itself (again, hypothetically detectable using an instrumented fancy pointer-like thing). Which would be just absurd.