Cleanups and Exception handlers

Cleanups and Exception handlers

[Phil Muldoon]

I'd like to quantify and discuss strategies of cleanups and GDB exception
handlers.  It seems I am always making mistakes in this area, and the
comments in the TRY_CATCH macro (well to me) are not adequate about the
mechanics of cleanups in an exception handler.

So I would like to discuss patterns of usage with a view to updating
the comments to be more explanatory.

Take this example, something that caught me out recently:

  struct cleanups cleanups;

  cleanups = make_cleanup (xfree, foo);

  TRY_CATCH (except, RETURN_MASK_ALL)
    {
      make_cleanup_ui_out_tuple_begin_end (out, NULL);	
      ...
      // Do some other stuff
    }
  if (except.reason < 0)
    {
      // Some exception handling code
    }

  do_cleanups (cleanups);

It seems if you create a cleanup in an exception handler, even if the
old cleanup chain is saved before the exception handler starts, you
have to call do_cleanups() in the exception handler or the cleanup is
lost. It seems that the TRY_CATCH code is manipulating the cleanup
chain,  but I can't find any comments on why it does this, or how that
affects your own cleanup chain.  What happens if you discard cleanups
in an exception handler to a point where the cleanup was registered
before the exception handler started?

In the above example, assuming everything went OK in above, and no
calls produced an error() or some other call, when I call
do_cleanups() in that code, my "tuple" cleanup seems to have been
discarded.  (I know this as in my code, I rapidly encounter
ui_out->level > MAX_UI_OUT_LEVELS as the tuples were never closed in
the cleanups.

Question 1)

What is the rule regarding cleanup chains that were created outside of
an exception handler, but where additional cleanups were registered
and not processed (cleanedup) inside the exception handler.

Question 2)

If in the above example, something goes wrong, does one have to call
do_cleanups() in the exception handling block (IE if (except.reason <
0))

Example 2:


  TRY_CATCH (except, RETURN_MASK_ALL)
  {
    struct cleanups cleanups =
      make_cleanup_ui_out_tuple_begin_end (out, NULL);	
      ...
      // Do some other stuff
    do_cleanups (cleanups);	
    }
  if (except.reason < 0)
    {
      // Some exception handling code
    }

In this example we create a pointer to the cleanup chain, and perform
a tuple cleanup.  If I call do_cleanups() in this example (assuming
nothing went wrong in the TRY_CATCH block), the tuple is cleaned up
properly.  But what happens if something happens and the exception
handling "if" is called.

Question 3)

For cleanups created and registered exclusively in the TRY_CATCH
block, on some exception being raised are they cleaned up
automatically?  (As there is no scope for the exception handling
failure code to do cleanups).

This all came about as MI tuples tend to be "long lived" cleanups and
in Python we have to handle every potential GDB exception call in an
exception handler to prevent propagation upwards.  I found this leads
to giant TRY_CATCH calls that encompass tuple creation, tuple
population, and tuple closure. But I am not sure.

Cheers,

Phil

Re: Cleanups and Exception handlers

[Jan Kratochvil]

On Wed, 01 May 2013 12:28:07 +0200, Phil Muldoon wrote:
> Question 1)
> 
> What is the rule regarding cleanup chains that were created outside of
> an exception handler, but where

> additional cleanups were registered
> and not processed (cleanedup) inside the exception handler.

When you end TRY_CATCH block (therefore not exiting the block by a thrown
exception) there must be no leftover cleanups.  You must do_cleanups or
discard_cleanups them all.  Debugging some new gdb_assert at that point here,
it may make sense there.


> Question 2)
> 
> If in the above example, something goes wrong, does one have to call
> do_cleanups() in the exception handling block (IE if (except.reason <
> 0))

You have to call do_cleanups in the same TRY_CATCH block.  TRY_CATCH
completely separates handling of exceptions outside of the block vs. inside of
the block.

Therefore "Example 2" is right:

> Example 2:
> 
> 
>   TRY_CATCH (except, RETURN_MASK_ALL)
>   {
>     struct cleanups cleanups =
>       make_cleanup_ui_out_tuple_begin_end (out, NULL);	
>       ...
>       // Do some other stuff
>     do_cleanups (cleanups);	
>     }
>   if (except.reason < 0)
>     {
>       // Some exception handling code
>     }
> 
> In this example we create a pointer to the cleanup chain, and perform
> a tuple cleanup.  If I call do_cleanups() in this example (assuming
> nothing went wrong in the TRY_CATCH block), the tuple is cleaned up
> properly.  But what happens if something happens and the exception
> handling "if" is called.

In such case cleanups are executed already inside the TRY_CATCH block at the
time the exception was thrown.  throw_exception contains:
	do_cleanups (all_cleanups ());

This will execute all pending cleanup handlers inside the TRY_CATCH block.
But no cleanups outside of the TRY_CATCH block.


> Question 3)
> 
> For cleanups created and registered exclusively in the TRY_CATCH
> block, on some exception being raised are they cleaned up
> automatically?

Yes, see above:
In such case cleanups are executed already inside the TRY_CATCH block at the
time the exception was thrown.  throw_exception contains:
	do_cleanups (all_cleanups ());


> This all came about as MI tuples tend to be "long lived" cleanups and
> in Python we have to handle every potential GDB exception call in an
> exception handler to prevent propagation upwards.

It seemed to me that there were too many TRY_CATCH blocks even in cases where
nothing can throw an exception.


Jan

Re: Cleanups and Exception handlers

[Phil Muldoon]

On 01/05/13 16:21, Jan Kratochvil wrote:
> On Wed, 01 May 2013 12:28:07 +0200, Phil Muldoon wrote:
>> Question 1)
>>
>> What is the rule regarding cleanup chains that were created outside of
>> an exception handler, but where
> 
>> additional cleanups were registered
>> and not processed (cleanedup) inside the exception handler.
> 
> When you end TRY_CATCH block (therefore not exiting the block by a thrown
> exception) there must be no leftover cleanups.  You must do_cleanups or
> discard_cleanups them all.  Debugging some new gdb_assert at that point here,
> it may make sense there.

Ok thanks.  That rules out a pattern with (the tuple example) for
Python.  As that cleanup lives as long as the tuple needs to be open
(which tends to be function length).

>> Question 2)
>>
>> If in the above example, something goes wrong, does one have to call
>> do_cleanups() in the exception handling block (IE if (except.reason <
>> 0))
> 
> You have to call do_cleanups in the same TRY_CATCH block.  TRY_CATCH
> completely separates handling of exceptions outside of the block vs. inside of
> the block.
> 
> Therefore "Example 2" is right:

In the first example, where did the previously registered cleanup
outside of the TRY_CATCH go? Was it discarded (or leaked?)


>> In this example we create a pointer to the cleanup chain, and perform
>> a tuple cleanup.  If I call do_cleanups() in this example (assuming
>> nothing went wrong in the TRY_CATCH block), the tuple is cleaned up
>> properly.  But what happens if something happens and the exception
>> handling "if" is called.
> 
> In such case cleanups are executed already inside the TRY_CATCH block at the
> time the exception was thrown.  throw_exception contains:
> 	do_cleanups (all_cleanups ());

Ok thanks, so in the case of cleanups entirely encapsulated in the
TRY_CATCH block there is no need to do anything in the later exception
handling block (the "if", after).

>> This all came about as MI tuples tend to be "long lived" cleanups and
>> in Python we have to handle every potential GDB exception call in an
>> exception handler to prevent propagation upwards.
> 
> It seemed to me that there were too many TRY_CATCH blocks even in cases where
> nothing can throw an exception.

This is because most (though I have no audited all of them) calls to
ui_out_* use *_filtered function calls (at least when the output is
directed to the CLI).  These can be interrupted from GDB and Python
has to handle the resulting GDB generated keyboard interruption
exception.  It's a massive pain in the neck. ;)

Cheers

Phil

Re: Cleanups and Exception handlers

[Jan Kratochvil]

On Wed, 01 May 2013 20:46:10 +0200, Phil Muldoon wrote:
> In the first example, where did the previously registered cleanup
> outside of the TRY_CATCH go? Was it discarded (or leaked?)

Right, leaked, forgotten in memory, a bug.  The new patch will assert those:
	[patch 2/2] Assert leftover cleanups in TRY_CATCH
	http://sourceware.org/ml/gdb-patches/2013-05/msg00008.html


> On 01/05/13 16:21, Jan Kratochvil wrote:
> > In such case cleanups are executed already inside the TRY_CATCH block at the
> > time the exception was thrown.  throw_exception contains:
> > 	do_cleanups (all_cleanups ());
> 
> Ok thanks, so in the case of cleanups entirely encapsulated in the
> TRY_CATCH block there is no need to do anything in the later exception
> handling block (the "if", after).

Right.


> > It seemed to me that there were too many TRY_CATCH blocks even in cases where
> > nothing can throw an exception.
> 
> This is because most (though I have no audited all of them) calls to
> ui_out_* use *_filtered function calls (at least when the output is
> directed to the CLI).  These can be interrupted from GDB and Python
> has to handle the resulting GDB generated keyboard interruption
> exception.  It's a massive pain in the neck. ;)

Oops, OK.


Jan

Re: Cleanups and Exception handlers

[Doug Evans]

On Wed, May 1, 2013 at 12:44 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Wed, 01 May 2013 20:46:10 +0200, Phil Muldoon wrote:
>> > It seemed to me that there were too many TRY_CATCH blocks even in cases where
>> > nothing can throw an exception.
>>
>> This is because most (though I have no audited all of them) calls to
>> ui_out_* use *_filtered function calls (at least when the output is
>> directed to the CLI).  These can be interrupted from GDB and Python
>> has to handle the resulting GDB generated keyboard interruption
>> exception.  It's a massive pain in the neck. ;)
>
> Oops, OK.

A comment in the code explaining why things are the way they are
would be most welcome.

Re: Cleanups and Exception handlers

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I'd like to quantify and discuss strategies of cleanups and GDB exception
Phil> handlers.  It seems I am always making mistakes in this area, and the
Phil> comments in the TRY_CATCH macro (well to me) are not adequate about the
Phil> mechanics of cleanups in an exception handler.

Phil> So I would like to discuss patterns of usage with a view to updating
Phil> the comments to be more explanatory.

There's also a section in gdbint.texinfo, though it seems reasonably out
of date, seeing as it does not mention TRY_CATCH at all.


The simplest, and IMO therefore best, way to approach cleanups is to
pretend that they introduce blocks.

That is, when you see an assignment of a cleanup to a local:

    cleanup = make_cleanup (...);

you should mentally add a "{" to the text.

And when you see a do_cleanups or discard_cleanups call, you should
mentally add a "}".

Then, if the braces in the function -- all of them, the real ones plus
the one you added mentally -- do not balance, something is wrong.


This approach is sufficient for most of the code in gdb.  There are some
necessary exceptions to the rule (some functions must return cleanups
somehow; and also sometimes the strict lexical rule will not work), some
weird code, and some code making assumptions about its caller.  The
cleanup checker (archer.git tromey/cleanup-checker) diagnoses these.


It is of course possible to be more dynamic with cleanups and not to
pretend they are block structured.  There aren't any actual rules.
However, I think the more dynamic style is bug-prone, and my proof of
this is the large number of actual bugs in this area that I've fixed
over the years, including all the ones fixed on the cleanup-checker
branch but not yet submitted.

Tom