Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

KubaO
Copy link

@KubaO KubaO commented Jun 27, 2023

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.

@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #11887 (3474a65) into master (813d559) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #11887   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files         155      155           
  Lines       20564    20564           
=======================================
  Hits        20238    20238           
  Misses        326      326           
Impacted Files Coverage Δ
py/nlr.c 100.00% <ø> (ø)
py/nlrx64.c 71.42% <ø> (ø)
py/objgenerator.c 100.00% <100.00%> (ø)

@KubaO KubaO force-pushed the windows-msvc-native-nlr branch from 7386d8c to aa6282b Compare June 27, 2023 19:47
KubaO added 3 commits June 27, 2023 15:59
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>
@KubaO KubaO force-pushed the windows-msvc-native-nlr branch from aa6282b to 3474a65 Compare June 27, 2023 20:04
@stinos
Copy link
Contributor

stinos commented Jun 28, 2023

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" />
Copy link
Contributor

@stinos stinos Jun 28, 2023

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>
Copy link
Contributor

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

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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 :)

@KubaO
Copy link
Author

KubaO commented Jun 28, 2023

Out of interest: how did you get the asm code?

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.

@KubaO KubaO changed the title Implement native NLR on Windows under MSVC Draft: Implement native NLR on Windows under MSVC Jun 28, 2023
@KubaO
Copy link
Author

KubaO commented Jun 28, 2023

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.

@KubaO KubaO closed this Jun 28, 2023
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.

2 participants