[Bug] Wrong Exception Handling : Destructor not called immediately

Hi all,

This is a test g++ test case for checking correct exception handling.

#ifdef GXX_EXPERIMENTAL_CXX0X
#define NOEXCEPT_FALSE noexcept (false)
#else
#define NOEXCEPT_FALSE
#endif

extern “C” void abort ();

int thrown;

int as;
struct a {
a () { ++as; }
~a () NOEXCEPT_FALSE { --as; if (thrown++ == 0) throw 42; }
};

int f (a const&) { return 1; }
int f (a const&, a const&) { return 1; }

int bs;
int as_sav;
struct b {
b (…) { ++bs; }
~b () { --bs; as_sav = as; }
};

bool p;
void g()
{
if (p) throw 42;
}

int main () {
thrown = 0;
try {
b tmp(f (a(), a()));

g();
}
catch (…) {}

// We throw when the first a is destroyed, which should destroy b before
// the other a.
if (as_sav != 1)
abort ();

thrown = 0;
try {
b tmp(f (a()));

g();
}
catch (…) {}

if (bs != 0)
abort ();
}

This Test Case aborts on ARM as well as X86 with clang latest trunk while with g++ no aborts are seen .

Here, when first temporary object 'a’ gets destroyed, its destructor is called where we throw an exception. This should immediately call destructor of ‘b’ and before calling constructor of second temporary object ‘a’. Instead, with clang, it is waiting for second temporary object ‘a’ to get destroyed and then calling its own destructor.

In my opinion, the compiler is not generating correct landing pad for the exception. Can someone please help in validating this reason? If it is a genuine bug then i will file a bug and try to work out solution for it.

Gentle Ping !! Please help to verify if this is a bug and if my analysis is correct.

I think the test case looks correct but my analysis is inconsistent with yours. However I agree there is a bug. Interestingly, while looking into it just now I wrote my own test case which revealed a separate (but perhaps related) bug in an old version of g++ (4.1.2) where destructors were being called twice. The current version appears to be correct though. Anyway, my test case follows:

#include
class Foo
{
private:
static int n;
static bool thrown;
int m_n;
public:
Foo():m_n(n++) { std::cout << "Constructor " << m_n << std::endl; }
~Foo() noexcept(false) { std::cout << "Destructor " << m_n << std::endl;
if(!thrown) {
thrown = true;
std::cout << “Throwing exception” << std::endl;
throw 0;
}
}
};

int Foo::n = 0;
bool Foo::thrown = false;

Foo bar(Foo a, Foo b) { return Foo(); }

int main()
{
try
{
Foo foo = bar(Foo(), Foo());
}
catch(…)
{
std::cout << “In exception handler” << std::endl;
}
std::cout << “Out of exception handler” << std::endl;
}

I expect the following output:

Constructor 0
Constructor 1
Constructor 2
Destructor 1
Throwing exception
Destructor 2
Destructor 0
In exception handler
Out of exception handler

With clang++ 3.4 I'm getting the following output:
Constructor 0
Constructor 1
Constructor 2
Destructor 1
Throwing exception
Destructor 0
In exception handler
Out of exception handler


So it looks like the destructor for the returned object is not being called at all (not in the wrong order). I get the same behavior in C++11 mode, too (though you have to add noexcept(false) to the destructor due to the implicit noexcept).

-- Matthew P. Del Buono

Possibly related to this: http://llvm.org/bugs/show_bug.cgi?id=14223

We still compile specific projects with -O1 to get around this issue.

  - ½

The test case in Bug 14223 is working fine with the latest trunk version.

And so it is. I've resolved the bug, adding the clang svn revision.

Thanks!

  - ½

See:

  http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050886.html

... where this exact testcase was last discussed.

One more doubt i had.

If suppose there is heirarchy of temporary objects like :

*b(b(b(a()))) ;*

and an exception occurs in the destructor of innermost object, should only
the immediate parent object's destructor should be called or cleanup should
occur
for the whole heirarchy of parent objects? If so, should it be true if
there is a function call somewhere in between the heirarchy like :

*b(b(func(b(a))));*

Regards,
Suyog Sarda

Ok. sorry for my silly question above. i tested the code for the question i had, and it seems that destructor of all temporaries
as well as other objects created before the current line are being called when exception occurs, except the destructor of the
outermost object.

From the discussion in the link shared in the previous mail and the results of the test case, i conclude that for
clang, outermost object temp of class b is not considered constructed until all the inner temporaries are destroyed.

Since the outermost ‘temp’ is considered yet to be constructed fully, its destructor is not pushed in ‘EHStack’.
As a result whenever exception occurs in the destructor of temporaries, the EHStack does not contain destructor of outermost
object and hence its cleanup doesn’t take place.

Please correct me if i am wrong? Also, while debugging i came across, following piece of code,

<i>void [CodeGenFunction::EmitCXXTryStmt](http://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CodeGenFunction.html#a079ab0affd32c3e7d992d8421776b732)(const [CXXTryStmt](http://clang.llvm.org/doxygen/classclang_1_1CXXTryStmt.html) &[S](http://clang.llvm.org/doxygen/AnalysisBasedWarnings_8cpp.html#a33dc45a03958a0bf07b5da2dec4db648)) {
        [EnterCXXTryStmt](http://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CodeGenFunction.html#ae064b29757ae2b6085d4df62ea2df530)(S);
        [EmitStmt](http://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CodeGenFunction.html#ab625dabfdcc8082335d64c4cbd009ef0)(S.[getTryBlock](http://clang.llvm.org/doxygen/classclang_1_1CXXTryStmt.html#a0b14fc308d0e5f78e4f2bd425fc308da)());
        [ExitCXXTryStmt](http://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CodeGenFunction.html#ac1f782050e59094db58fbbd1b9e3f8c1)(S);
 }</i>

If the constructor has completed, the destructor needs to be called later. IIRC, the problem that makes solving this so annoying is that, as soon as one temporary destructor terminates abnormally, the next thing to destroy is always the local variable (since it was constructed most recently), and only then proceeding to destroy the remainder of the temporaries.

That is, if we’re creating local variable X, and we needed to construct temporaries A, B, and C to do that, then if C’s destructor throws, we need to destroy X, then B, then A; and if B’s destructor throws, we need to destroy X before A.

This is not achievable with a simple stack mechanism.

John.

Also of note is
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1634, which
will introduce a "full expression storage duration" for temporaries. Right
now, the destruction order is not wonderfully well-specified; under that
change, we'll need to clarify whether X is destroyed before the
temporaries, or whether full-expression storage duration objects are
destroyed before automatic storage duration objects in all cases (which
lets us use a simple stack).

Thanks to all. I wrote a test case to see if the constructor of outermost variable completes before destructor of inner temporaries is called.

int as;
struct a {
a () { printf(“Construct A\n”); }

~a () NOEXCEPT_FALSE { printf(“Destruct A\n”); throw 42; }
};

int f (a const&) { return 1; }

struct b {
b (…) { printf(“Construct B\n”); }
~b () { printf(“Destruct B\n”); }

int x(){return 1;}
};

int main () {
thrown = 0;
try {
printf("%d\n",b(f(a())).x());
}
catch (…) {}

}

Output with clang :

Construct A
Construct B
1
Destruct B
Destruct A

This indicates that Constructor of outermost object completes before destructor of inner temporaries is called (as it was able to call function x() ).

As per the link provided by Richard, it is not yet clear what should be the order of destructor to be called.
GCC calls destructor of outer local variable as soon as the destructor of any one of the temporaries throws, destructor of other temporaries are called later.
This is not easy to achieve in clang with EHStack approach.

Now as we are not sure of the order of destructors to be called, should we approach with ‘full-expression storage duration objects are destroyed before automatic storage duration objects in all cases’, which would be achievable with EHStack approach? Because right now, as the destructor of local variable is not called at all (in our original test case ‘init-temp1.C’), there is a resource leak. Or should we wait for things to get clear and then only go ahead for solving this bug?

Hi all,

Finally !! Some interesting findings from my side on why this issue is happening. I will try to explain my findings with pseudo code :

Sample pseudo code and how clang process it :

Consider our code having three class a,b and c

main()
{

try {

b tmp1();

c tmp2(b(a));
}

catch {…}
}

A piece of code in clang to focus :

void CodeGenFunction::EmitCXXTryStmt(const CXXTryStmt &S) {
EnterCXXTryStmt(S);
EmitStmt(S.getTryBlock());
ExitCXXTryStmt(S);
}

There is an EHStack, which will contain entries which will be popped out to emit cleanup code (destructor) in case of exception happening.

When try-catch block is encountered, we visit ‘EmitStmt’ function, after some more function call we visit following code for emitting code for local variables tmp1 and tmp2:

void CodeGenFunction::EmitAutoVarDecl(const VarDecl &D) {
AutoVarEmission emission = EmitAutoVarAlloca(D);
EmitAutoVarInit(emission);
EmitAutoVarCleanups(emission);
}

First we visit EmitAutoVarInit where some initialization happens by visiting inner expression. For tmp1 object inner expression is empty, so next function EmitAutoVarCleanup will be called and entry of this object tmp1 will be put into EHStack. Since the scope of tmp1 is till end of try block, it will remain in EHStack till try block exits.

Next, We visit second object tmp2 construction, where inner expression consist of temporaries. We visit function EmitAutoVarInit where we process inner expression. Entries of Inner temporary are added into EHStack till all temporary entries are processed, but as soon as the processing of all temporaries gets over i.e. their scope gets over, these entries are popped out of EHstack and their destructor is called as a part of cleanup code.
Please note we are yet to add entry of tmp2 in EHStack as we are yet to visit EmitAutoVarCleanups function.

Now, The main problem starts - if any of the destructor of temporaries throw, we visit EHStack, pop out each entry from EHStack and emit cleanup code (destructor) for each entry. Note that, the entry of outermost local variable is still not present in the EHStack as we are yet to visit function EmitAutoVarCleanups, as a result of this, its cleanup code (destructor) is not emitted and we land into resource leak!!

I just altered the order of function calls - first call EmitAutoVarCleanups and then EmitAutoVarInit - which ensures that entry of auto variables are added in EHStack before we process inner expression*.* This resolves our original issue ‘init-temp1.c’, however it introduces 10 regressions all related to location of destructors of temporaries.

This is a resource leak issue and hence i am pressing more on it (Order in which destructors should be called is not clear, but we should eliminate atleast the resource leak). I am not sure although if my approach to resolve this issue is ok. Basically, we somehow need to ensure that auto object entry gets added in EHStack (may be a call to EmitAutoVarCleanups function) before we pop out entries for temporaries.

All your suggestions/comments/rectifications are most awaited.

Your proposed change causes the destructor for the auto object to be run if the initializer throws, even if the auto var constructor hasn’t been run yet.

John.

Your proposed change causes the destructor for the auto object to be run
if the initializer throws, even if the auto var constructor hasn't been run
yet.

My bad, i got carried away with specifics to the problem and didn't look at
whole picture.

Ok, right now, cleanup code of temporaries is getting emitted before entry
of auto variable gets added in the EHStack which is causing resource leak.
This is because, scope of the temporaries gets over (which causes their
cleanup code to emit) after initialization but before adding EHStack entry
of auto variable.
If the temporary storage duration is extended till full expression, then it
will be easy to use EHStack.

For auto variable emission there are three steps involved (Correct me if i
am wrong) :

1. EmitAlloca - emit code for alloca which will allocate memory
2. EmitInit - emit code for initializing the object which involves
evaluating the subexpression
3. EmitCleanup - EmitCleanup code for constructed object

The problem is after allocating memory successfully in 1st step, if
something goes wrong in initializing (exception in temporary destructor),
the memory allocated in 1st step is not cleaned because it happens in 3rd
step which is never visited.

Is their any other way to resolve this issue? Or only extending storage
duration will help in easy implementation?