Bug 58876 - No non-virtual-dtor warning in std::unique_ptr
Summary: No non-virtual-dtor warning in std::unique_ptr
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.8.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on: 80472 59304
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-25 10:39 UTC by yangzhe1990
Modified: 2021-09-01 15:43 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-10-30 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.)
Comment 10 Daniel Boles 2021-04-17 10:23:45 UTC
Sorry to pester, but is this likely to get anywhere, any time soon? I fixed a bug in one of my projects recently by using AddressSanitizer, which this would've caught if it warned on non-polymorphic deletion via unique_ptr<base>, so it'd be nice to see.

I presume the same goes for std::shared_ptr, too.
Comment 11 Jonathan Wakely 2021-08-31 18:20:04 UTC
(In reply to DB from comment #10)
> Sorry to pester, but is this likely to get anywhere, any time soon?

No, probably not. Comment 2 doesn't work because -Wsystem-headers can't be enabled and disabled using pragmas. It doesn't work like other warnings.

I don't think this can be fixed in the library.


> I presume the same goes for std::shared_ptr, too.

No, because shared_ptr type-erases the deleter and so deletes the dynamic type even if the stored pointer is upcast to a different static type.
Comment 12 Harald van Dijk 2021-08-31 20:09:53 UTC
(In reply to Jonathan Wakely from comment #11)
> No, probably not. Comment 2 doesn't work because -Wsystem-headers can't be
> enabled and disabled using pragmas. It doesn't work like other warnings.

However, the internal version of the #line directive, # [line number] [file name] [flags] could be used to mark a region of a system header as non-system header, which should achieve the same result, right? It might need a bit of cleanup to be maintainable, but this seems to work as a proof of concept:

--- bits/unique_ptr.h
+++ bits/unique_ptr.h
@@ -82,7 +82,9 @@
 		     "can't delete pointer to incomplete type");
 	static_assert(sizeof(_Tp)>0,
 		     "can't delete pointer to incomplete type");
+# 86 __FILE__
 	delete __ptr;
+# 88 __FILE__ 3
       }
     };
Comment 13 Jonathan Wakely 2021-09-01 09:57:28 UTC
It's so crazy, it just might work.