Fix disengage failure logs for FatalConditionHandlerGuard

[yuriykoch]

Description

FatalConditionHandlerGuard is used within RunContext::invokeActiveTestCase().
The intent of this guard is to avoid binary crash without failed test being
reported.
Still in case FatalConditionHandlerGuard destructor being called during stack
unwinding AND finds unexpected top-level filter for SEH unhandled exception,
the binary may still crash. As result of such crash the original exception
details are being hidden.

As the Catch2 provides only CATCH_CATCH_ANON macro, with no access to
exception details by design, looks like the best way to handle issue is to:

• state requirements explicitly by noexcept specifier
• use Catch::cerr() to print out possible issue notification

[codecov]

Codecov Report

Merging #2376 (5f3a07f) into devel (529eec9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            devel    #2376      +/-   ##
==========================================
+ Coverage   91.04%   91.06%   +0.01%     
==========================================
  Files         152      152              
  Lines        7313     7313              
==========================================
+ Hits         6658     6659       +1     
+ Misses        655      654       -1     

[yuriykoch]

I've tried to align tests with the ones added by #2334. Please let me know if anything else should be done.

[horenmar]

1. I am not sure I understand the issue you are running into. Can you provide more detailed example?
2. Would backporting the changes from v3 be a better fix?

[yuriykoch]

Basically, v3 also uses CATCH_RUNTIME_ERROR within ~FatalConditionHandlerGuard() destructor, so backporting the changes from v3 wouldn't resolve a root cause.
In general, destructor should never throw during stack unwinding: https://isocpp.org/wiki/faq/exceptions#dtors-shouldnt-throw

As for example, let's say we have the following code somewhere in product or 3rd-party dependencies:

bool foo() { return true; }

class bar {
  LPTOP_LEVEL_EXCEPTION_FILTER previousTopLevelExceptionFilter;
public:
  bar() {
    previousTopLevelExceptionFilter = SetUnhandledExceptionFilter(newTopLevelExceptionFilter);
    if (foo())
      throw std::runtime_error("Failure details");
  }
  ~bar() {
    SetUnhandledExceptionFilter(previousTopLevelExceptionFilter);
  }
};

Now let's assume we use Catch2 to test such code:

CHECK_NOTHROW(bar{});

The test would result in std::terminate() call with no "Failure details" mentioned anywhere.

[yuriykoch]

@horenmar Am I missing something? Please feel free to provide any concerns you have

[horenmar]

@yuriykoch Just the usual, not enough time.

Anyway, after reading this again I have no objections. For some reason I was reading the changes as against the v2, where failing to remove Catch2's handler meant something was seriously wrong, because the VEH handlers form a list, and it would mean that our handler somehow disappeared from the list. With the changes to v3, there are indeed situations where the check can fail and shouldn't kill the process.