[vim/vim] if_python3,win32: Fix Python3 stable ABI (PR #13260)

K.Takata

unread,

Oct 3, 2023, 11:52:08 PM10/3/23

to vim/vim, Subscribed

There were some issues in current stable ABI implementation on Windows:

Python DLL name should be python3.dll instead of python311.dll and so on. (See: https://docs.python.org/3/c-api/stable.html)
Some non-stable API functions were used:
- _PyObject_NextNotImplemented
- PyStdPrinter_Type
reset_stdin() and hook_py_exit() didn't work with python3.dll. python3.dll is a special type of DLL called forwarder DLL. It just forwards the functions to other DLL (e.g. python311.dll). There were two issues regarding these functions:
- python3.dll doesn't have import tables. This caused a crash in get_imported_func_info(). Add a check whether the specified DLL has an import table.
- reset_stdin() and hook_py_exit() should be applied to the forwarded DLL (e.g. python311.dll), not to python3.dll. Check the export directory of python3.dll to find the forwarded DLL and apply the functions to it.

You can view, comment on, or merge this pull request online at:

https://github.com/vim/vim/pull/13260

Commit Summary

f100041 if_python3,win32: Fix Python3 stable ABI

File Changes

(5 files)

Patch Links:

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.

codecov[bot]

unread,

Oct 4, 2023, 12:02:38 AM10/4/23

to vim/vim, Subscribed

Codecov Report

Merging #13260 (f100041) into master (3f168ec) will increase coverage by 0.65%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13260      +/-   ##
==========================================
+ Coverage   82.16%   82.81%   +0.65%     
==========================================
  Files         160      150      -10     
  Lines      196149   182917   -13232     
  Branches    43949    41029    -2920     
==========================================
- Hits       161161   151480    -9681     
+ Misses      22122    18466    -3656     
- Partials    12866    12971     +105

Flag	Coverage Δ
huge-clang-Array	`82.81% <ø> (+<0.01%)`	⬆️
linux	`82.81% <ø> (+<0.01%)`	⬆️
mingw-x64-HUGE	`?`
mingw-x86-HUGE	`?`
windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/if_python3.c	`80.86% <ø> (+6.41%)`	⬆️

... and 140 files with indirect coverage changes

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 4, 2023, 6:26:45 AM10/4/23

to vim/vim, Subscribed

Thanks for looking into this. I kind of did the bare minimum for Windows when implementing it.

  * `python3.dll` doesn't have import tables. This caused a crash in `get_imported_func_info()`. Add a check whether the specified DLL has an import table.
  * `reset_stdin()` and `hook_py_exit()` should be applied to the forwarded DLL (e.g. `python311.dll`), not to `python3.dll`. Check the export directory of `python3.dll` to find the forwarded DLL and apply the functions to it.

Looking at the code, you calling get_imported_func_info on the forwarded DLL already right? Wouldn't that work? Are you adding a check to get_imported_func_info just to be safe (which isn't a bad thing)?

FWIW I kind of dislike how the Python subsystem share the same stdin/stdout with the main Vim process. It leads to odd issues like #3236. But then it's kind of hard to fix the issue because we are just loading a DLL in to the main process instead of interacting with Python as a separate process.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

K.Takata

unread,

Oct 4, 2023, 6:40:08 AM10/4/23

to vim/vim, Subscribed

Looking at the code, you calling get_imported_func_info on the forwarded DLL already right? Wouldn't that work? Are you adding a check to get_imported_func_info just to be safe (which isn't a bad thing)?

No. Only setting pythonthreedll=python3.dll without my patch, get_imported_func_info was called on python3.dll and it caused a crash.
If pythonthreedll points a forwarder DLL (python3.dll), then we have to find the actual DLL (e.g. python311.dll) and have to call get_imported_func_info on the actual DLL.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Oct 4, 2023, 2:07:32 PM10/4/23

to vim/vim, Subscribed

thanks!

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Oct 4, 2023, 2:07:42 PM10/4/23

to vim/vim, Subscribed

Closed #13260 via 119fdd9.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Yee Cheng Chin

unread,

Oct 10, 2023, 8:05:24 PM10/10/23

to vim/vim, Subscribed

Speaking of which would it make sense to switch the Windows builds (https://github.com/vim/vim-win32-installer) to use stable Python ABI? I would imagine it should be strictly better but I don't know if there are edge cases in the Windows environment that I'm thinking of. I think the python3.dll file is usually located in the same location right (C:\Windows\system32)? I don't know what happens when the user has multiple versions of Python installed though (assuming they are using the standard installer, which a lot of people don't, as there are at least a million ways to install Python).

This way users don't have to worry when a new Python version comes out and it "just works".

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

K.Takata

unread,

Oct 10, 2023, 10:12:47 PM10/10/23

to vim/vim, Subscribed

I think the python3.dll file is usually located in the same location right (C:\Windows\system32)?

No, it is installed in the python installation directory (normally C:\Program Files\Python3XX (64-bit), C:\Program Files (x86)\Python3XX-32 (32-bit), or each user's AppData directory). It will be added to PATH only when the user selected it.

Currently, if_python3 can find python3XX.dll from the registry even the python installation directory is not in PATH, but it doesn't work when python3.dll is used. This is because python3.dll just forwards to python3XX.dll but the forwarded DLL can be loaded only when it is in PATH.
I'm now thinking if there is a workaround for this.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

K.Takata

unread,

Oct 11, 2023, 7:02:04 AM10/11/23

to vim/vim, Subscribed

See #13315

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward