RE: std::exception Interoperability

["Erik Cassel" <erik@...>]

Greg,

Thanks for finding this bug!  I made a classic C++ mistake.

I agree that a properly crafted transfer-of-ownership constructor should do
the trick.  But yes, you have to accept that a lot of "auto" stuff goes on
under the hood.

I agree that the implementation should be hidden, or some constructors and
fields should be declared private, etc.  

-Erik


-----Original Message-----
From: lua-bounces@bazar2.conectiva.com.br
[mailto:lua-bounces@bazar2.conectiva.com.br] On Behalf Of Greg Falcon
Sent: Monday, March 27, 2006 12:42 PM
To: Lua list
Subject: Re: std::exception Interoperability

On 3/25/06, Erik Cassel <erik@roblox.com> wrote:
>#define LUAI_THROW(L,c) throw(lua_exception(L,c))
...snip...
> lua_exception::~lua_exception()
> {
>         if (!committed)
>         {
>                 // The exception was caught before Lua got it.
>                 // Revert the error state
>                 // TODO: Is it safe to set it to 0 or should we
>                 // restore it to a previous state?
>                 errorJmp->status = 0;
>                 // Pop the error message
>                 lua_pop(L, 1);
>         }
> }

There's unfortunately a problem with the C++ here.  Paragraphs 3 and 4
of section 15.1 of the C++ spec cover it.

The problem has to do with your LUAI_THROW's explicitly generated
temporary object, and the rules of the lifetime of thrown objects. 
Execution of the line

throw(lua_exception(L,c))

can cause the following sequence of events:
1. A temporary lua_exception is explicitly constructed
2. The exception mechanism generates its own temporary lua_exception. 
It is constructed by copying step #1's temporary object (using
lua_exception's implicit copy c'tor)
3. Code jumps to the exception handler.  The temporary lua_exception
from step 1 goes out of scope, its d'tor is called, and since that
temporary was never commit()ed, an object is incorrectly popped from
the stack.

The reason why you didn't run into this problem is because the
compiler is allowed to optimize this away, combining the two temporary
objects into one.  From the spec, 15.1 paragraph 5:

"If the use of the [throw-expression-generated] temporary object can
be eliminated without changing the meaning of the program except for
the execution of constructors and destructors associated with the use
of the temporary object, then the exception in the handler can be
initialized directly with the argument of the throw expression."

This optimization is allowed but not required, so a
standards-compliant C++ compiler would be justified in running your
d'tor twice, once for each temporary, resulting in an extra item being
popped off the stack.

There's no completely pleasing solution to this problem.  You can't
hide the implicit copy constructor, because thrown objects need to be
copyable.  The best I can think of is to give class lua_exception
transfer-of-ownership semantics, sort of like auto_ptr:

lua_exception::lua_exception(const lua_exception& that)
       :L(that.L)
       ,errorJmp(that.errorJmp)
       ,committed(that.false)
{
  that.commit();
}

and mark member variable committed as mutable.

It's a little scary, in the way that auto_ptr is scary: making a copy
of a lua_exception object changes the behavior of that object.  If
users of this patch (as altered) treat lua_exception as an off-limits
implementation class, and not as a class to use for their own
purposes, then this problem can be avoided.

Greg F