Bug 58876

Summary: No non-virtual-dtor warning in std::unique_ptr
Product: gcc Reporter: yangzhe1990
Component: libstdc++Assignee: Jonathan Wakely <redi>
Status: ASSIGNED ---    
Severity: normal CC: daniel.kruegler, dimhen, mw_triad, tavianator, webrown.cpp
Priority: P3 Keywords: diagnostic
Version: 4.8.1   
Target Milestone: ---   
See Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70692
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58875
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82745
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87614
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2013-10-30 00:00:00
Bug Depends on: 80472, 59304    
Bug Blocks:    

Description yangzhe1990 2013-10-25 10:39:39 UTC
When compiling the following code

#include <memory>

struct A {
	virtual void f() = 0;
};

struct B : public A {
	virtual void f() {};
};

int main() {
	std::unique_ptr<A> p(new B());
	A *q = NULL;
	delete q;
	return 0;
};

the compiler shows the following warning,

test.cpp: In function ‘int main()’:
test.cpp:14:9: warning: deleting object of abstract class type ‘A’ which has non-virtual destructor will cause undefined behaviour [-Wdelete-non-virtual-dtor]
  delete q;

But a warning at the unique_ptr line is also expected.
Comment 1 Jonathan Wakely 2013-10-30 15:04:07 UTC
This is because warnings are suppressed by default in system headers, and the undefined delete operation occurs in a system header.  You get the warning if you use -Wsystem-headers

I don't see an easy way to force the warning to always be emitted though.
Comment 2 Jonathan Wakely 2013-10-30 15:09:03 UTC
Actually we can use this around the definition of default_delete<>

#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wsystem-headers"
  /// Primary template of default_delete, used by unique_ptr
  template<typename _Tp>
    struct default_delete
    {
...
    };
#pragma GCC diagnostic pop
Comment 3 Paolo Carlini 2013-10-30 17:57:19 UTC
At some point Ian Taylor filed a Bugzilla about these issues, I think it's still open. Not sure what we should do in this area...
Comment 4 Jonathan Wakely 2013-10-30 17:59:59 UTC
Yes, I should dig Ian's bug out and have another look. I'm planning to throw some ideas around on the mailing list ...
Comment 5 Matthew Woehlke 2014-12-23 23:43:43 UTC
See also bug 64399, which proposes that a) the conversion itself should generate a warning, and b) the presence of other virtual methods in A should not be required for the warning to trip. (This could be achieved by something like static_assert except to emit a warning, combined with std::has_virtual_destructor, without otherwise having to fiddle with pragmas.)

Actually, this may be required for 'make_unique<A>(new B)' to warn, since the conversion of a B* ('new B') to an A* (which is what is passed to make_unique / unique_ptr::unique_ptr) should not warn. (IOW, unique_ptr / make_unique would need overloads taking any pointer type and doing the conversion inside STL so that std::has_virtual_destructor can be checked against the actual pointer type.)

...or alternatively gcc would need to detect when a converted pointer is passed to unique_ptr / make_unique, which seems like it would be harder.
Comment 6 Jonathan Wakely 2014-12-24 09:00:54 UTC
(In reply to Matthew Woehlke from comment #5)
> Actually, this may be required for 'make_unique<A>(new B)' to warn, since

That's not how make_unique works.
Comment 7 Matthew Woehlke 2014-12-24 16:59:50 UTC
(In reply to Jonathan Wakely from comment #6)
> (In reply to Matthew Woehlke from comment #5)
> > Actually, this may be required for 'make_unique<A>(new B)' to warn, since
> 
> That's not how make_unique works.

...and I'm suggesting it *should* be. (How else are you going to warn? After that executes, the pointer no longer knows that it really contains a B, unless you teach the compiler some fancy extra tricks, which seems overly complicated. Conversely, I feel that 'make_unique<A>(new B)' should warn if it's going to result in failing to call B's dtor. I might even go so far as to say 'even if the compiler can prove that B's dtor is trivial', though I'd be willing to delegate that to a different and more pedantic warning.)
Comment 8 Jonathan Wakely 2014-12-24 20:08:31 UTC
No, really, that's not how make_unique works. You do not use 'new' with make_unique, that's the whole point, so you would say make_unique<B>() to create a B. Your motivating examples should be valid C++ of you want to convince anyone, so maybe:

unique_ptr<A> p = make_unique<B>();
Comment 9 Matthew Woehlke 2014-12-26 16:31:26 UTC
(In reply to Jonathan Wakely from comment #8)
> No, really, that's not how make_unique works. You do not use 'new' with
> make_unique, that's the whole point [...]

D'oh, sorry :-). Not sure what I was thinking. I think I meant 'unique_ptr<A>(new B)', like to the original example. That said...

> unique_ptr<A> p = make_unique<B>();

...this is also a good example that I would like to see warn. (I think this has the same problems; the warning would need to trigger in the conversion operator, otherwise the knowledge of the true type is lost by the time the unique_ptr<A> is destroyed.)