From ef5382af27e1cc1e14d66e992b8dc90e3137d143 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Filip=20Str=C3=B6mb=C3=A4ck?= <fstromback@gmail.com>
Date: Wed, 10 Mar 2021 12:53:50 +0100
Subject: [PATCH] Save and restore GetLastError() on Windows.

This is a patch for the problem outlined in issue #61 - that the value in
GetLastError() is not automatically saved and restored when a vectored exception
handler is called. This is solved the same way as errno was handled on POSIX
systems.

This patch does *not* save and restore errno on Windows, since it seems like the
MPS does not call any functions that modify errno on the critical path on
Windows (generally only functions from POSIX do that).
---
 code/protw3.c             | 10 ++++++++++
 design/thread-manager.txt | 13 +++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/code/protw3.c b/code/protw3.c
index d961e74648..6c57731ae4 100644
--- a/code/protw3.c
+++ b/code/protw3.c
@@ -44,6 +44,7 @@ LONG WINAPI ProtSEHfilter(LPEXCEPTION_POINTERS info)
   AccessSet mode;
   Addr base, limit;
   LONG action;
+  DWORD lastError;
   MutatorContextStruct context;
 
   er = info->ExceptionRecord;
@@ -51,6 +52,12 @@ LONG WINAPI ProtSEHfilter(LPEXCEPTION_POINTERS info)
   if(er->ExceptionCode != EXCEPTION_ACCESS_VIOLATION)
     return EXCEPTION_CONTINUE_SEARCH;
 
+  /* This is the first point where we call a Windows API function that
+   * might change the last error. There are also no early returns from
+   * this point onwards.
+   */
+  lastError = GetLastError();
+
   MutatorContextInitFault(&context, info);
 
   /* assert that the exception is continuable */
@@ -97,6 +104,9 @@ LONG WINAPI ProtSEHfilter(LPEXCEPTION_POINTERS info)
     action = EXCEPTION_CONTINUE_SEARCH;
   }
 
+  /* Restore the last error value before returning. */
+  SetLastError(lastError);
+
   return action;
 }
 
diff --git a/design/thread-manager.txt b/design/thread-manager.txt
index 82350f0bbc..b9291e3c76 100644
--- a/design/thread-manager.txt
+++ b/design/thread-manager.txt
@@ -78,6 +78,13 @@ blocked in a system call, and the MPS signal handlers updating
 
 .. _GitHub issue #10: https://github.com/ravenbrook/mps/issues/10
 
+_`.req.thread.lasterror`: It would be nice if on Windows systems the
+MPS does not cause system calls in the mutator to update the value
+returned from ``GetLastError()`` when the exception handler is called
+due to a fault. This may cause the MPS to destroy the previous value
+there. (See `GitHub issue #61`_.)
+
+.. _GitHub issue #61: https://github.com/Ravenbrook/mps/issues/61
 
 Design
 ------
@@ -136,6 +143,12 @@ independent thread, should save and restore its value." All MPS
 signals handlers therefore save and restore ``errno`` using the macros
 ``ERRNO_SAVE`` and ``ERRNO_RESTORE``.
 
+_`.sol.thread.lasterror`: The documentation for ``AddVectoredExceptionHandler``
+does not mention ``GetLastError()`` at all, but testing_ the behaviour
+reveals that any value in ``GetLastError()`` is not preserved. Therefore,
+this value is saved using ``LAST_ERROR_SAVE`` and ``LAST_ERROR_RESTORE``.
+
+.. _testing: https://github.com/Ravenbrook/mps/issues/61
 
 Interface
 ---------