Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add win32 access violation handler to buffer manager #2035

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

benjaminwinger
Copy link
Collaborator

@benjaminwinger benjaminwinger commented Sep 14, 2023

Windows will produce an access violation hardware exception (a.k.a. segfault) when attempting to read from de-committed memory, something which may occur in the buffer manager if another thread releases memory which was about to be read from. On linux/unix this isn't an issue since the kernel will allow the memory to be accessed and just provide empty data.

I've worked around this by enabling the /EHa compiler flag, which turns on custom exception handling for hardware exceptions. There are two downsides to this:

  1. catch (...) will now catch hardware exceptions on windows, which is bad (note that the ... is explicit here). There are no instances of this in our code, but there are a few in third_party. One in spdlog which is just used to log information and re-throw (which should be fine), one in antlr4_runtime in a function we are not using, and a few in pybind11, which might affect the python library on windows.
  2. There is a slight performance penalty since the compiler now has to assume that every single try/catch might catch an exception, where with the default /EHs (only catches C++ exceptions) it can optimize away try/catch blocks when there are no exceptions that could be thrown. I don't think this is very significant.

The custom exception handler raises a custom exception if the exception is a segfault, and the memory location of the segfault is checked to make sure it occurred within the buffer manager's reserved virtual memory to avoid catching other segfaults within the same scope (silently ignoring those would be a major issue).

The one thing I'm not sure about, is that the use of catch (...) in pybind11 seems to be in the code that handles translating c++ exceptions into python exceptions (i.e. it will cover all exceptions thrown by our c++ code), and it looks like it's turning arbitrary non-standard exceptions into a python RuntimeError, which seems like a bad way of reporting hardware exceptions like segfaults in the C++ extension.

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (49850c8) 90.15% compared to head (4417cda) 90.15%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2035      +/-   ##
==========================================
- Coverage   90.15%   90.15%   -0.01%     
==========================================
  Files         938      938              
  Lines       33364    33363       -1     
==========================================
- Hits        30080    30079       -1     
  Misses       3284     3284              
Files Changed Coverage Δ
src/include/storage/buffer_manager/vm_region.h 100.00% <ø> (ø)
src/storage/buffer_manager/buffer_manager.cpp 94.69% <100.00%> (-0.05%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/storage/buffer_manager/buffer_manager.cpp Outdated Show resolved Hide resolved
@benjaminwinger benjaminwinger merged commit 0eed60d into master Sep 15, 2023
10 checks passed
@benjaminwinger benjaminwinger deleted the windows-access-violation branch September 15, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants