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: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.0
: P3 major
Target Milestone: 12.0
Assignee: Jason Merrill
URL:
Keywords: wrong-code
: 80664 (view as bug list)
Depends on:
Blocks: 93033 93922 94041
  Show dependency treegraph
 
Reported: 2015-05-13 20:14 UTC by Frank Heckenbach
Modified: 2022-01-11 22:34 UTC (History)
21 users (show)

See Also:
Host:
Target:
Build:
Known to work: 12.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.
Comment 19 Jason Merrill 2020-03-10 20:34:56 UTC
No longer fixed for GCC 10.
Comment 20 Jakub Jelinek 2020-03-11 06:59:08 UTC
commit r10-7110-g14af5d9b19b0f4ee1d929e505e245ae5c2f6bdc6
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Mar 10 16:05:18 2020 -0400

    c++: Partially revert patch for PR66139.
    
    The patch for 66139 exposed a long-standing bug with
    split_nonconstant_init (since 4.7, apparently): initializion of individual
    elements of an aggregate are not a full-expressions, but
    split_nonconstant_init was making full-expressions out of them.  My fix for
    66139 extended the use of split_nonconstant_init, and thus the bug, to
    aggregate initialization of temporaries within an expression, in which
    context (PR94041) the bug is more noticeable.  PR93922 is a problem with my
    implementation strategy of splitting out at gimplification time, introducing
    function calls that weren't in the GENERIC.  So I'm going to revert the
    patch now and try again for GCC 11.
    
    gcc/cp/ChangeLog
    2020-03-10  Jason Merrill  <jason@redhat.com>
    
            PR c++/93922
            PR c++/94041
            PR c++/52320
            PR c++/66139
            * cp-gimplify.c (cp_gimplify_init_expr): Partially revert patch for
            66139: Don't split_nonconstant_init.  Remove pre_p parameter.
Comment 21 Richard Biener 2020-05-26 14:15:36 UTC
This comes up repeatedly :/
Comment 22 CVS Commits 2022-01-07 00:24:41 UTC
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:4f6bc28fc7dd86bd9e7408cbf28de1e973dd1eda

commit r12-6329-g4f6bc28fc7dd86bd9e7408cbf28de1e973dd1eda
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Mar 5 15:50:45 2020 -0500

    c++: EH and partially constructed aggr temp [PR66139]
    
    Now that PR94041 is fixed, I can return to addressing PR66139, missing
    cleanups for partially constructed aggregate temporaries.  My previous
    approach of calling split_nonconstant_init in cp_gimplify_init_expr broke
    because gimplification is too late to be introducing destructor calls.  So
    instead I now call it under cp_fold_function, just before cp_genericize;
    doing it from cp_genericize itself ran into trouble with the rewriting of
    invisible references.
    
    So now the prediction in cp_gimplify_expr that cp_gimplify_init_expr
    might need to replace references to TARGET_EXPR_SLOT within
    TARGET_EXPR_INITIAL has come to pass.  constexpr.c already had the simple
    search-and-replace tree walk I needed, but it needed to be fixed to actually
    replace all occurrences instead of just the first one.
    
    Handling of VEC_INIT_EXPR at gimplify time had similar issues that we worked
    around with build_vec_init_elt, so I'm moving that to cp_fold_function as
    well.  But it seems that build_vec_init_elt is still useful for setting the
    VEC_INIT_EXPR_IS_CONSTEXPR flag, so I'm leaving it alone.
    
    This also fixes 52320, because build_aggr_init of each X from build_vec_init
    builds an INIT_EXPR rather than call split_nonconstant_init at that point,
    and now that INIT_EXPR gets split later.
    
            PR c++/66139
            PR c++/52320
    
    gcc/cp/ChangeLog:
    
            * constexpr.c (replace_decl): Rename from replace_result_decl.
            * cp-tree.h (replace_decl): Declare it.
            * cp-gimplify.c (cp_gimplify_init_expr): Call it.
            (cp_gimplify_expr): Don't handle VEC_INIT_EXPR.
            (cp_genericize_init, cp_genericize_init_expr)
            (cp_genericize_target_expr): New.
            (cp_fold_r): Call them.
            * tree.c (build_array_copy): Add a TARGET_EXPR.
            * typeck2.c (digest_init_r): Look through a TARGET_EXPR.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp0x/initlist116.C: New test.
            * g++.dg/cpp0x/initlist117.C: New test.
            * g++.dg/cpp0x/lambda/lambda-eh.C: New test.
            * g++.dg/eh/aggregate1.C: New test.
Comment 23 Jason Merrill 2022-01-07 00:28:39 UTC
Fixed for GCC 12.