-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Draft: Implement native NLR on Windows under MSVC #11887
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
Conversation
Code size report:
|
Codecov Report
@@ Coverage Diff @@
## master #11887 +/- ##
=======================================
Coverage 98.41% 98.41%
=======================================
Files 155 155
Lines 20564 20564
=======================================
Hits 20238 20238
Misses 326 326
|
7386d8c
to
aa6282b
Compare
Signed-off-by: Kuba Sunderland-Ober <kuba@mareimbrium.org>
Signed-off-by: Kuba Sunderland-Ober <kuba@mareimbrium.org>
Signed-off-by: Kuba Sunderland-Ober <kuba@mareimbrium.org>
aa6282b
to
3474a65
Compare
Out of interest: how did you get the asm code? Is it from the disassembly of the msvc setjmp implementation? It looks rather similar but that's no surprise, after are there's only so many ways to do this. Just for reference it's shown here #4652). That being said: since setjmp is available and works it's not clear to me what we're gaining by switching to a custom implementation. |
@@ -45,6 +45,7 @@ | |||
</PropertyGroup> | |||
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" /> | |||
<ImportGroup Label="ExtensionSettings"> | |||
<Import Project="$(VCTargetsPath)\BuildCustomizations\masm.props" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make indentation the same as the rest, also in other places?
</ImportGroup> | ||
</Project> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline shouldn't change here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line is what MSVC nowadays puts at the end of the file if anyone ever wanted to open it in non-read only mode. I can of course remove it.
@@ -28,7 +28,9 @@ | |||
|
|||
#if !MICROPY_NLR_SETJMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still be defined for msvc builds so the new code isn't actually used, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems not to be defined under MSVC. The new implementations definitely end up being used. I had to debug them :)
I wrote it from first principles, basing on the existing code in mpy but adapting it to MS calling conventions. Never looked at MSVC code. Since custom asm NLR seems to be available and used for most targets on gcc/clang, I thought perhaps it wouldn’t be a bad idea to get it done for MSVC too. This is a first step toward getting native code emitters - at least for Intel - working on MSVC (already done just needs to be cleaned up). Thinking about it now, perhaps I should bite the bullet and get ARM NLR and native emitter working user MSVC too. |
Turns out Windows NLR handling will also need SEH state to be preserved. So this is a draft at the moment. I'll investigate whether the setjmp/longjmp implementation is sufficient. |
Native NLR now works when building with MSVC for both x86 and x64.
This is one step on the way to getting feature parity between MINGW/gcc Windows builds and MSVC.