Save and restore GetLastError() on Windows. #62
Conversation
|
I should mention: I have tried building and running this on Windows (using the hot variety). I have not run it against the tests in tests/, but it works well when I run it with my language that uses the MPS (so at least it passes the smoke-test). |
There was a problem hiding this comment.
Thanks for sending the pull request, which looks good. I've made a couple of comments, but we also need some test coverage. If you look at #32 you'll see that I did this by updating test/function/236.c, which was convenient because this test was already exercising thread suspend and restore on POSIX platforms. In your case you'll want to write a new test case, which will mean figuring out how to use the test system! There are some slender usage notes in test/README, otherwise ask if you need help.
fstromback
force-pushed
the
save-errno-win32
branch
from
ab3ac55
to
56a1ac8
Compare
This is to illustrate that the value in GetLastError() may be clobbered by the exception handler on Windows in some circumstances. As this commit is before the patch, the test currently fails (clearly showing the issue).
fstromback
force-pushed
the
save-errno-win32
branch
from
56a1ac8
to
f80c9a4
Compare
|
I have updated the branch now as suggested above. I have also added a test case for this (in 237.c). I put that in its own commit now, so it is easy to see that the test fails consistently on Windows first, then when the fix is applied, it passes consistently. It was a bit tricky to get the right conditions for the issue to be visible. The test is written so that it does its best to set up everything, verify that the right conditions are met and then do the test. This way it can reliably determine that it tests the right thing, and can fail if it for some reasons fails to set up the right conditions (the MPS must decide to raise both the read and write barrier somewhere, so we can trigger a scan in the vectored exception handler). I have described in quite a bit of detail how it works inside the test file. Hopefully this is understandable. |
code/protw3.c
Outdated
| * GetLastError() and SetLastError() are defined. | ||
| */ | ||
|
|
||
| #define LAST_ERROR_SAVE BEGIN DWORD _saved_last_error = GetLastError(); BEGIN |
There was a problem hiding this comment.
I used the ERRNO_SAVE and ERRNO_RESTORE as a template here initially. But since it is only used in one location at the moment I agree that the macro-free version is better. I will fix that.
design/thread-manager.txt
Outdated
| 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. |
| * the MPS will call a system call that fails in the exception handler | ||
| * when responding to the hit. | ||
| * | ||
| * The tool we use here is a separate thread. We create and register |
There was a problem hiding this comment.
This is massively cunning. Applause!
This is a patch for the problem outlined in issue Ravenbrook#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).
fstromback
force-pushed
the
save-errno-win32
branch
from
f80c9a4
to
ef5382a
Compare
|
Thank you for your feedback @NickBarnes ! I have made the suggested changes (removed the macros and added the link in the documentation). |
|
Hi again, I just wanted to check in on the status of this PR. I think I have made all requested changes, but I might have missed something (either in the code or regarding licences etc.). If so, please let me know! |
There was a problem hiding this comment.
Sorry for being so dilatory in getting this merged. I've been putting it off because we no longer have any continuous integration coverage on Windows. (This used to live in a Jenkins instance in the Ravenbrook office, but that was taken offline when the office was closed and no-one has had the spoons to get it going again.) But the continuous integration situation is not going to get better by doing nothing, so I recommend merging as-is (or with the typo fixes below) and worrying about the Windows build at the point when (or if) someone is able to get continuous integration going again.
|
Thank you for the pointers and the update! I have fixed the issues you pointed to, I hope they are satisfactory. |
|
Pull request merged — thank you for the contribution. |
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).
I have tried to update the documentation similar to what was done for the errno
patch for POSIX, but as I am not entirely familiar with the RST format, I may have
made some mistakes there.