Re: [PATCH v2 1/2] msvcrt: Clean up registered C++ object in handler.

[Piotr Caban]

Hi Daniel,

On 06/06/17 00:43, Daniel Lehman wrote:
> +typedef void (CALLBACK *__WINE_FINALLY_CTX)(BOOL, PEXCEPTION_RECORD, 
> void*);
> The idea behind __FINALLY_CTX was to model native __finally behavior 
> while extending its functionality for wine. It's not possible to call 
> GetExceptionInfo/GetExceptionCode inside of finally on Windows so I 
> don't think it's a change in good direction. If you really need the 
> exception record you shouldn't use __FINALLY_CTX (but I don't think you 
> need).
> +static void CALLBACK cxx_catch_cleanup(BOOL normal, EXCEPTION_RECORD *rec, void *ctx)
> +{
> +    thread_data_t *data = msvcrt_get_thread_data();
> +    cxx_frame_info *frame = ctx;
> +
> +    if (normal)
> +        __CxxUnregisterExceptionObject(frame, FALSE);
> +    else
> +    {
> +        if (cxx_is_consolidate(rec))
> +            rec = (void*)rec->ExceptionInformation[4];
> +
> +        __CxxUnregisterExceptionObject(frame,
> +                rec->ExceptionCode == CXX_EXCEPTION &&
> +                data->exc_record->ExceptionCode == CXX_EXCEPTION &&
> +                rec->ExceptionInformation[1] == data->exc_record->ExceptionInformation[1]);
> +    }
> Do you need to call __CxxUnregisterExceptionObject with TRUE as second 
> argument in any case except of rethrow?
> /* FIXME:  native does local_unwind here in case of exception rethrow */

If you only need to handle rethrow in a special way I think it should be 
done differently. Something like this should do the job:
__TRY
{
     __TRY
     {
         ret_addr = handler(0, frame);
     }
     __EXCEPT(rethrow_filter)
     {
         /* probably all of this should be done inside  of the filter */
	/* do some unwinding here or add a FIXME */
         catch_cleanup_ctx.in_use = TRUE;
         rethrow;
     }
     __ENDTRY;
}
__FINALLY_CTX(cxx_catch_cleanup, &catch_cleanup_ctx);

The above code is incorrect if exception object needs to be preserved 
not only in case of rethrow.

What do you think about it?

Thanks,
Piotr

[PATCH v2 1/2] msvcrt: Clean up registered C++ object in handler.

[Daniel Lehman]

> > +        __CxxUnregisterExceptionObject(frame,
> > +                rec->ExceptionCode == CXX_EXCEPTION &&
> > +                data->exc_record->ExceptionCode == CXX_EXCEPTION &&
> > +                rec->ExceptionInformation[1] == data->exc_record->ExceptionInformation[1]);
> > +    }
> > Do you need to call __CxxUnregisterExceptionObject with TRUE as second argument in any case except of rethrow?

Yeah, just rethrow is handled specially.  And extra specially for SEH
> __FINALLY_CTX(cxx_catch_cleanup, &catch_cleanup_ctx);
>
> The above code is incorrect if exception object needs to be preserved not only in case of rethrow.
>
> What do you think about it?

Sounds good

Thanks,
Piotr