Windows Debug builds failure: 'DbgGUIInitSolarMutexCheck': identifier not found

[mikekaganski@xxxxxxxxxxx (Kaganski Mike)]

Hi!

On 8/8/2018 3:56 PM, Stephan Bergmann wrote:
> On 07/08/18 18:14, Stephan Bergmann wrote:
>> As we speak, I happen to run into your CppunitTest_cppuhelper_qa_misc 
>> failure with my MSVC build (with latest Visual Studio 2017, see above) 
>> too, will investigate.  (The test currently only runs (late) during 
>> `make check`, see <https://gerrit.libreoffice.org/#/c/58696/> "No need 
>> for CppunitTest_cppuhelper_qa_misc to be a subsequentcheck".)
> 
> Mike is working on a fix at <https://gerrit.libreoffice.org/#/c/58730/> 
> "Don't break __CxxDetectRethrow contract"

I seem to be unable to come with a solution, so I decided to summarize 
my findings here.

So, with commit 6ddecf61ecada646fbd6f8809270d47289727e8a, Caolán has 
uncovered a hidden bug that must have been there for some time. Function 
cpp_call() in bridges/source/cpp_uno/msvc_win32_*/uno2cpp.cxx uses SEH 
(__try-__except statement) to guard the called method; 
CPPU_CURRENT_NAMESPACE::mscx_filterCppException() is used there as the 
__except expression; and the latter uses CRT's *internal* 
__CxxDetectRethrow() function to check if the handled C++ exception is 
being re-thrown. __CxxDetectRethrow() checks the opaque exception 
descriptor passed by pointer (which has internal EHExceptionRecord* 
type), and basically detects if two conditions are true: the exception 
is C++ exception (using PER_IS_MSVC_EH macro), and that it has no 
exception information (using PER_PTHROW macro); see comment in 
ExFilterRethrow in MSVCRT's crt/src/vcruntime/frame.cpp for the same check.

Unfortunately, the function __CxxDetectRethrow() modifies global state: 
it increments an internal variable (__ProcessingThrow++), which is used 
throughout CRT's exception handling code to detect that unwinding is in 
progress when its value is non-zero (one comment says "we are 
unwinding... possibly called from a dtor()"). In that case, e.g., 
std::current_exception() returns empty exception_ptr (related code is in 
CRT's __ExceptionPtr::_CurrentException() method). As documented in the 
beginning of the crt/src/vcruntime/mgdframe.cpp, where 
__CxxDetectRethrow() is defined, the function is used internally when 
C++ exceptions are handled in COM+. It is designed to be used in 
specific conditions, and after some other companion functions have been 
called (specifically, __CxxExceptionFilter and 
__CxxRegisterExceptionObject); finally, __CxxUnregisterExceptionObject 
must be called. All the mentioned functions also modify 
__ProcessingThrow; they also modify other internal variables (global 
state), allocate and release memory; and __CxxUnregisterExceptionObject 
also destructs the exception object.

All of the described complexity made me unable to come with a proper way 
to call those functions: they expect some state of SEH; they modify the 
state; not calling __CxxExceptionFilter leads to still unpaired 
decrements of __ProcessingThrow; trying to finally call 
__CxxUnregisterExceptionObject destroys the object that we tested in our 
mscx_filterCppException, which leads to use-after-delete. So the problem 
now remains: it's not enough to just revert the Caolán's commit 
mentioned above, since we obviously break CRT state now and then, and 
the revert would just hide this, not solve.

The related code (using __CxxDetectRethrow to check for the exception 
being rethrown) was introduced with commit 
37d3cdd8d280f509ffa37e47c4706213f4dcda7c from 2003; but I don't know if 
the function already had current behaviour back then.

Just for reference, I paste here the reference code given in 
crt/src/vcruntime/mgdframe.cpp as the example of how the functions work 
together:

////////////////////////////////////////////////////////////////////////////////
// Model of C++ eh in COM+
//
// void func()
// {
//     try {
//         TryBody();
//     } catch (cpp_object o)
//     {
//         CatchOBody();
//     } catch (...)
//     {
//         CatchAllBody();
//     }
// }
//
// Turns into this:
//
//
// void func()
// {
//     int rethrow;
//     // One per try block
//     int isCxxException;
//     // One per catch(...)
//     __try {
//         TryBody();
//     }
//     __except(__CxxExceptionFilter(exception,
//                                   typeinfo(cpp_object),
//                                   flags,
//                                   &o))
//     // This is how it's done already
//     {
//     // Begin catch(object) prefix
//     char *storage = _alloca(__CxxQueryExceptionSize());
//     rethrow = false;
//     __CxxRegisterExceptionObject(exception,
//                                  storage);
//     __try {
//         __try {
//             // End catch(object) prefix
//             CatchOBody();
//             // Begin catch(object) suffix
//         } __except(rethrow = __CxxDetectRethrow(exception),
//                    EXCEPTION_CONTINUE_SEARCH)
//         {}
//     }
//     __finally
//     {
//         __CxxUnregisterExceptionObject(storage,
//                                        rethrow);
//     }
//     // End catch(object) suffix
//     }
//     __except(1)
//     {
//         // Begin catch(...) prefix
//         char *storage = _alloca(__CxxQueryExceptionSize());
//         rethrow = false;
//         isCxxException = __CxxRegisterExceptionObject(exception,
//                                                       storage);
//         __try
//         {
//             __try
//             {
//             // End catch(...) prefix
//             CatchAllBody();
//             // Begin catch(...) suffix
//         } __except(rethrow = __CxxDetectRethrow(exception),
//                    EXCEPTION_CONTINUE_SEARCH)
//         {}
//     } __finally
//     {
//         if (isCxxException)
//         __CxxUnregisterExceptionObject(storage, rethrow);
//     }
//     // End catch(...) suffix
//     }
// }
//
////////////////////////////////////////////////////////////////////////////////

-- 
Best regards,
Mike Kaganski