microsoft / wil

Windows Implementation Library
MIT License
2.58k stars 236 forks source link

Tests fail when compiled with FrameHandler4 support due to bug in FH4 #102

Open dunhor opened 5 years ago

dunhor commented 5 years ago

Still looking into it, but it seems that in the following:

        HRESULT hr = S_OK;
        try
        {
            throw std::bad_alloc();
        }
        catch (...)
        {
            hr = LOG_CAUGHT_EXCEPTION();
            auto hrDirect = wil::ResultFromCaughtException();
            REQUIRE(hr == hrDirect);
        }

The catch (...) handler is not getting executed inside of ResultTests::ExceptionHandling. FWIW something similar is working correctly:

        auto hr = []() -> HRESULT
        {
            try
            {
                throw std::bad_alloc();
            }
            catch (...)
            {
                RETURN_CAUGHT_EXCEPTION();
            }
        }();

The only difference is that the second example is within a lambda. Could be that Catch2 is doing something weird w.r.t. exception handling that's interfering with a newer update in MSVC. Still looking at it

dunhor commented 5 years ago

Should also be noted that if I change the first block above to execute within a lambda the catch handler does execute.

dunhor commented 5 years ago

Disabling FH4 seems to be a decent workaround for now. I tried disabling Catch2's vectored exception handling, thinking that may somehow be getting in the way, but the issue still persisted. Starting to think this may legitimately be a compiler bug, but I've been unable to get a minimal repro to narrow down/validate.

dunhor commented 5 years ago

Still no minimal repro, but I've narrowed it down a bit. Immediate thought is that FrameHandler4 has a leak. Debugging it I see that the search state is '0' for the throw address, which is clearly bogus and thus it is no surprise that the catch handler is bypassed. Debugging further, I see that __FrameHandler4::StateFromIp returns a seemingly correct value (129), however this value is discarded in __FrameHandler4::GetHandlerSearchState since CatchStateInParent is '0'. Clearing out the value (i.e. setting CatchStateInParent to '-2') in the debugger causes the correct catch handler to be found and the test case passes. So it seems like CatchStateInParent is set earlier in the test but never cleared.

From what I can gather, this state is ultimately caused by the test case above it, which does a DoesCodeCrash call (expecting the result to be 'true'). That function/code path is fairly hacky, but AFAICT should ultimately be valid code. The code in question is expected to issue a fail-fast which we "bypass" by providing a custom fail-fast callback that instead raises a structured exception via RaiseException that we then catch later. So it seems somewhat likely that FH4 and SEH don't mix particularly well under some circumstances. That said, given that I've been unable to devise a repro, there's likely more at play here that I'm missing.

dunhor commented 5 years ago

Took a time-travel trace of the issue and basically validated my hypothesis. Bug reported to https://developercommunity.visualstudio.com/content/problem/784770/framehandler4-is-broken-if-you-raise-a-structured.html. The reason why I wasn't able to create a minimal repro is because state 0 isn't as invalid as I thought it was. If there's no destructors, etc. outside of a try (and no other try, etc. before it), then state 0 will correspond to the try block and things will work as expected. I had to give it a destructor to run and then it started reproing

dunhor commented 5 years ago

For those curious, here's a fun repro:

void magic_fun_time()
{
    __try
    {
        []()
        {
            try { throw std::exception(); }
            catch (...) { ::RaiseException(STATUS_ACCESS_VIOLATION, 0, 0, nullptr); }
        }();
    }
    __except (EXCEPTION_EXECUTE_HANDLER) {}
}

void fun()
{
    try { throw std::exception(); }
    catch (...) { std::printf("Isn't this fun?\n"); }

    magic_fun_time();

    try { throw std::exception(); }
    catch (...) {}
}

int main([[maybe_unused]] int argc, [[maybe_unused]] char** argv)
{
    fun();
}