Bug 66139 - destructor not called for members of partially constructed anonymous struct/array
Summary: destructor not called for members of partially constructed anonymous struct/a...
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: 10.0
Assignee: Jason Merrill
URL:
Keywords: wrong-code
: 80664 (view as bug list)
Depends on:
Blocks: 93033
  Show dependency treegraph
 
Reported: 2015-05-13 20:14 UTC by Frank Heckenbach
Modified: 2020-01-03 22:06 UTC (History)
33 users (show)

See Also:
Host:
Target:
Build:
Known to work: 10.0
Known to fail: 4.8.5, 6.3.0, 7.1.0, 8.0
Last reconfirmed: 2018-08-17 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Heckenbach 2015-05-13 20:14:31 UTC
The attached test program tries to construct various structures
consisting of two members of some type a, where construction of the
first one succeeds and the second one throws, and checks whether the
destructor of the first one was called before the exception is
caught.

Since the first member was constructed successfully, I'd expect its
destructor to be called. Granted, I don't know the exact standard
wording, but my general idea is that once a constructor call
succeeds, an object starts to exist, thus it must at some point
cease to exist, and then its destructor must be called. clang 3.5.0
and visual c++ (according to http://webcompiler.cloudapp.net/,
without the GCC extension "(a[2]) { ... }") do call the destructor
every time.

The g++ output is:

destructor called
destructor not called
destructor called
destructor not called
destructor not called
destructor not called

So with a named struct/array variable, the destructor gets called,
with anonymous ones it doesn't.

Even with a named initializer_list it doesn't, but I suppose that's
because the compiler first constructs an anonymous array (and then
would construct the initializer_list if it got there).

#include <iostream>

int c, d;

struct a
{
  a (int i) { if (i) throw i; c++; }
  ~a () { d++; }
};

void check (void (*f) ())
{
  try
  {
    c = d = 0;
    f ();
  }
  catch (int)
  {
    std::cerr << (c != 1 ? "constructor not completed exactly once\n" :
                  d == 0 ? "destructor not called\n" :
                  d == 1 ? "destructor called\n" :
                           "destructor called more than once\n");
    return;
  }
  std::cerr << "exception not thrown\n";
}

int main ()
{
  struct s { a x, y; };
  check ([] { s t { 0, 1 }; });
  check ([] { s { 0, 1 }; });
  check ([] { a t[2] { 0, 1 }; });
  check ([] { (a[2]) { 0, 1 }; });
  check ([] { std::initializer_list <a> t { 0, 1 }; });
  check ([] { std::initializer_list <a> { 0, 1 }; });
}
Comment 1 Mikhail Kremniov 2017-03-10 12:35:35 UTC
This issue is also present in GCC 5.x, 6.x and the trunk.

I wonder if it's possible to increase its priority since it makes it impossible to guarantee code exception safety.
Comment 2 Tomasz Kamiński 2017-04-18 20:36:05 UTC
Example of some real life safe-code (now raw allocations, STL container used), that is leaking:
#include <string>
#include <vector>
#include <iostream>

using namespace std;


struct Test {
    string a;
    string b;
};

Test make_test() {
    vector<string> strings {"123456789abcdef0"};
    return Test {
        strings.at(0),
        strings.at(2) // throws; the destructor for a copy of strings.at(0) is not called and memory leakds
    };
}

int main() {
    try {
        Test test = make_test();
    } catch (exception &e) {
        cerr << e.what() << endl;
    }
}

Live code proving leaks: https://wandbox.org/permlink/GjjxFw9jAE9bfvyS
Comment 3 Carlo Wood 2017-05-05 16:06:57 UTC
Still present in 7.x and 8 (HEAD).
clang does not show this behavior (no leak).

Possibly related:
https://wandbox.org/permlink/yJM4fr9Z9D8GN6on
which also shows different behavior from clang
(found on https://akrzemi1.wordpress.com/2017/04/27/a-serious-bug-in-gcc/)

Can anyone confirm that the keyword 'wrong-code' also
applies to that latter example?
Comment 4 Richard Biener 2017-05-08 11:25:43 UTC
*** Bug 80664 has been marked as a duplicate of this bug. ***
Comment 5 Andrzej Krzemienski 2017-05-08 12:00:05 UTC
What does this mean that the status of this bug report is "NEW"? It is 2 years old. In GCC Bugzilla one can assign status "CONFIRMED" to bug reports. Why is this one not confirmed? Was nobody able to confirm that this bug exists in GCC?

It really looks serious, as it undermines C++'s exception safety rules and guarantees.
Comment 6 Markus Trippelsdorf 2017-05-08 12:13:45 UTC
(In reply to Andrzej Krzemienski from comment #5)
> What does this mean that the status of this bug report is "NEW"? It is 2
> years old. In GCC Bugzilla one can assign status "CONFIRMED" to bug reports.
> Why is this one not confirmed? Was nobody able to confirm that this bug
> exists in GCC?
> 
> It really looks serious, as it undermines C++'s exception safety rules and
> guarantees.

Calm down. NEW means confirmed, otherwise it would be UNCONFIRMED.
Writing trollish blog posts won't get the bug fixed any sooner.
Comment 7 Xi Ruoyao 2017-05-08 12:15:26 UTC
std::basic_string<...> is too large.  Replace it with a dummy
default constructable and copyable class Foo.  Then get GIMPLE:

          _1 = &<retval>->a;
          _2 = std::vector<Foo>::at (&strings, 0);
          Foo::Foo (_1, _2);
          _3 = &<retval>->b;
          _4 = std::vector<Foo>::at (&strings, 2);
          Foo::Foo (_3, _4);
          return <retval>;

No exception handling code here.
Comment 8 Markus Trippelsdorf 2017-05-10 12:29:03 UTC
*** Bug 80683 has been marked as a duplicate of this bug. ***
Comment 9 Jaak Ristioja 2017-05-10 13:04:05 UTC
According to a Stack Overflow answer [1] this bug occurs when the constructor is the first thing executed in the try-block. For example:


#include <iostream>
struct A { A(int e) { throw e; } };

struct B {
    A a{42}; // Same with = 42; syntax
};

int main() {
    try {
        // Remove this printout line to trigger the bug:
        std::cout << "Welcome" << std::endl; 
        B b;
    } catch (int e) {
        return e;
    }
}


  [1]: http://stackoverflow.com/a/43892501/3919155
Comment 10 Ondřej Majerech 2017-05-10 13:50:45 UTC
That SO answer appears to be plain out wrong. Running your snippet on GCC 6.3.1 and 8.0.0 20170507, the program calls terminate for me, even with the cout << "Welcome" line included.
Comment 11 Xi Ruoyao 2017-05-10 14:41:00 UTC
This happens for all TARGET_EXPRs with the third operand (cleanup expression),
as an INIT_EXPR's rhs.

The cleanup sequence are pushed in gimplify_target_expr, which doesn't
handle TARGET_EXPRs as the INIT_EXPRs' rhs.  So they just go missing.
Comment 12 Xi Ruoyao 2017-05-10 14:53:22 UTC
(In reply to Jaak Ristioja from comment #9)
>   [1]: http://stackoverflow.com/a/43892501/3919155

I don't think this is the same bug.
This bug seems happening because GCC created "constexpr B::B(void)", but actually
it throws, so can not be constexpr.
Comment 13 Ondřej Majerech 2017-05-10 15:19:13 UTC
(In reply to Xi Ruoyao from comment #12)
> (In reply to Jaak Ristioja from comment #9)
> >   [1]: http://stackoverflow.com/a/43892501/3919155
> 
> I don't think this is the same bug.
> This bug seems happening because GCC created "constexpr B::B(void)", but
> actually
> it throws, so can not be constexpr.

I would also think that to be a different bug, but when I reported that (bug 80683), it got marked as a duplicate of this one. Maybe the duplicate classification of 80683 should be re-reviewed?
Comment 14 dingd 2019-08-02 06:54:12 UTC
I believe this bug is still there on g++ 9.1.0.
Comment 15 dingd 2019-08-02 06:59:10 UTC
This bug may be not so rare as it seems, because it can be triggered when initializing a vector with an initializer list.
For example,

    std::vector<A> v = { A(), A() };

this is enough to trigger it if the constructor throws.
Comment 16 Antony Polukhin 2019-12-13 09:55:04 UTC
Can we increase the priority of this issue to P1 or P2? It affects the very basics of the C++.

BTW, I've minimized example. It aborts on every version of GCC with -std=c++11, passes on Clang:


int constructed = 0;

class lock_guard_ext{
public:
    lock_guard_ext() { ++constructed; }
    ~lock_guard_ext() { --constructed; }
};
 
struct Access {
    lock_guard_ext lock;
    int value;
};
 
int t() {
    throw 0;
}

Access foo1() {
    return { {}, t() };
}
 
int main () {
    try {
        foo1();
    } catch (int) {}
    if (constructed != 0)
        __builtin_abort();
}
Comment 17 Jason Merrill 2019-12-19 14:07:19 UTC
Author: jason
Date: Thu Dec 19 14:06:45 2019
New Revision: 279576

URL: https://gcc.gnu.org/viewcvs?rev=279576&root=gcc&view=rev
Log:
	PR c++/66139 - EH cleanups for partially constructed aggregates.

There were several overlapping PRs about failure to clean up fully
constructed subobjects when an exception is thrown during aggregate
initialization of a temporary.  I fixed this for non-temporaries in the
context of 57510, but that fix didn't handle temporaries.  So this patch
does split_nonconstant_init at gimplification time, which is much smaller
than alternatives I tried.

	PR c++/57510
	* cp-gimplify.c (cp_gimplify_init_expr): Use split_nonconstant_init.
	* typeck2.c (split_nonconstant_init): Handle non-variable dest.
	(split_nonconstant_init_1): Clear TREE_SIDE_EFFECTS.
	* tree.c (is_local_temp): New.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/initlist116.C
    trunk/gcc/testsuite/g++.dg/cpp0x/initlist117.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-gimplify.c
    trunk/gcc/cp/cp-tree.h
    trunk/gcc/cp/tree.c
    trunk/gcc/cp/typeck2.c
    trunk/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-eh.C
Comment 18 Jason Merrill 2019-12-19 14:15:04 UTC
Fixed for GCC 10 so far.