Bug 64399 - g++ does not diagnose when upcasting owning pointer (e.g. unique_ptr) with non-virtual destructor
Summary: g++ does not diagnose when upcasting owning pointer (e.g. unique_ptr) with no...
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.8.3
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-23 22:58 UTC by Matthew Woehlke
Modified: 2017-05-06 22:32 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Woehlke 2014-12-23 22:58:14 UTC
Consider the following code:

  #include <memory>
  
  struct X { ~X(); };
  struct Y : X { ~Y(); };
  
  std::unique_ptr<Y> f();
  void g()
  {
    std::unique_ptr<X> x = f();
  }

This code is almost certainly broken; ownership of the constructed Y is transferred to an owning unique_ptr in a way that will most likely (in general; "always" in this specific example) result in Y's destructor never being called.

It seems pretty silly that there is no diagnostic for this. There ought to be a warning in unique_ptr's conversion operator converting from Derived to Base if ~Base is not virtual. (Arranging at the library level to trip -Wnon-virtual-dtor seems logical.)
Comment 1 Jonathan Wakely 2014-12-23 23:33:10 UTC
PR 58876
Comment 2 Matthew Woehlke 2014-12-23 23:36:59 UTC
(In reply to Jonathan Wakely from comment #1)
> PR 58876

*Almost*, except I am proposing that -Wnon-virtual-dtor should trip even if X does not otherwise have virtual methods. (Just why you'd be writing such code, I'm not sure, but...)

Odd that didn't turn up in my search for existing bugs...
Comment 3 Thiago Macieira 2014-12-24 01:37:12 UTC
Because it's not a bug.

This is a totally valid scenario.
Comment 4 Jonathan Wakely 2014-12-24 08:58:56 UTC
It might be valid with a custom deleter, but the example shown has undefined behaviour.
Comment 5 Jonathan Wakely 2014-12-24 09:01:54 UTC
N.B. we definitely want -Wdelete-non-virtual-dtor not the less useful -Wnon-virtual-dtor
Comment 6 Marc Glisse 2014-12-24 10:36:41 UTC
(In reply to Jonathan Wakely from comment #4)
> It might be valid with a custom deleter, but the example shown has undefined
> behaviour.

When the derived class does not add any member or redefine any important functionality, it is not an uncommon technique to call the base class destructor on a derived class. It might pedantically be illegal, but it is useful, and I believe some people would like to avoid the warning when the two destructors are equivalent.
Comment 7 Matthew Woehlke 2014-12-24 16:59:37 UTC
(In reply to Thiago Macieira from comment #3)
> Because it's not a bug.
> 
> This is a totally valid scenario.

Valid in what way? I constructed a Y but arranged, probably by accident, that its dtor is never called. I fail to see how that's not likely a bug in my code that reasonably warrants a diagnostic. (Note that I'm talking about a *warning*, and possibly one that isn't even on by default, not an error.)

(In reply to Marc Glisse from comment #6)
> It might pedantically be illegal, but it is useful, and I believe some people
> would like to avoid the warning when the two destructors are equivalent.

However, the compiler doesn't know that here, because I didn't provided a definition thereof; Y's dtor, even in this example, could have important side effects. Even if the compiler *can* prove equivalence, I'd be suspicious whether this was intended, but I'd be okay with a different (i.e. more pedantic) warning in that case. (I'd also point out that it's not unreasonable to require the user to somehow annotate if this is intentional if they care about avoiding the warning when it's enabled.)

Anyway, I still get no warning if Y has members that need to be destroyed, which definitely causes bad behavior when its dtor isn't called.