Switch from AddVectoredExceptionHandler to SetUnhandledExceptionFilter to avoid false positives

[Alan-Jowett]

Description

What

Catch2 incorrectly registers for SIGSEGV and other signals on Windows. The correct behavior is to register as the top-level unhandled exception filter so that it is notified last after the application has had an opportunity to handle the exception and after the debugger has been notified. The previous behavior was to register a vectored exception handler which results in Catch2 being notified prior to the application being able to handle the exception.

Why

Visual Studio 2022 uses structured exceptions to implement address sanitization (/fsanitize=address). This issue results in Catch2 being incompatible with a key bug-finding feature of VS 2022.

GitHub Issues

Closes #2332

Notes for maintainers

I didn't see any Windows-specific tests and I am unsure how to validate the new behavior in the CI/CD pipeline. I manually verified the change fixes the issue but would prefer to add a new test to the CI/CD pipeline if possible.

Signed-off-by: Alan Jowett alanjo@microsoft.com

[codecov]

Codecov Report

Merging #2334 (f923304) into devel (54e89e8) will decrease coverage by 0.25%.
The diff coverage is 60.00%.

❗ Current head f923304 differs from pull request most recent head f004061. Consider uploading reports for the commit f004061 to get more accurate results

@@            Coverage Diff             @@
##            devel    #2334      +/-   ##
==========================================
- Coverage   91.13%   90.88%   -0.25%     
==========================================
  Files         152      151       -1     
  Lines        7316     7229      -87     
==========================================
- Hits         6667     6570      -97     
- Misses        649      659      +10     

[horenmar]

Do I assume correctly that this will also fix #2286 and probably #898 as well?

[Alan-Jowett]

Added test to Misc.tests.cpp to validate modified SEH handling behavior on Windows. If this is the wrong test to add it to, let me know and I can move it.

[Alan-Jowett]

Do I assume correctly that this will also fix #2286 and probably #898 as well?

It should based on the description in the issues.

[horenmar]

Can you also add a test that unhandled exception still fails tests? To hide the test from standard run, add [.approvals] tag to it. ([approvals] is a special tag for tests that shouldn't be run as part of approval tests, . prefix is a short for [.] which hides the test unless it is explicitly selected).

Otherwise LGTM.

[horenmar]

I rebased the PR to contain the changes to JIT debugging, but the issue is still there: exception escaping the binary into CTest still causes wait until timeout.

[Alan-Jowett]

Let me take a look and see what is happening.

[Alan-Jowett]

Will take a look at this when I get back next year. Have a great new year.

[horenmar]

@Alan-Jowett Thanks for the PR, I am going to drop the failing test and merge the other parts.

It would be appreciated if you still tried to solve the issue with the JIT debugger on AppVeyor, but even without that this is still an improvement over what is there right now.

[alvinhochun]

Is it possible to backport this change to v2.x? I have hit the same issue with ASAN using LLVM/Clang.

[horenmar]

I am willing to merge a PR that does the backport (inc. tests)

[ScottHutchinson]

Is it possible to backport this change to v2.x?

My team will try to do this, unless @alvinhochun or somebody else is already working on it.

[alvinhochun]

Thanks, I haven't started backporting this for real, so you can do it if you would like to.

[ScottHutchinson]

I have manually tested the fix locally and it works. I expect to submit a PR by Monday.