fix raw-dylib opt-out on windows x86

newsfragments/6410.fixed.2.md

@@ -0,0 +1 @@
+Fix FFI definition `PyVectorcall_Call` failing to link on Python 3.11 and older.

newsfragments/6410.fixed.3.md

@@ -0,0 +1 @@
+Fix DLL load failures on Windows with PyPy when `raw-dylib` linking is disabled.

newsfragments/6410.fixed.md

@@ -0,0 +1 @@
+Fix link failures on 32-bit Windows when `raw-dylib` linking is disabled.

noxfile.py

@@ -1334,13 +1334,16 @@ def load_pkg_versions():
 
 @nox.session(name="ffi-check")
 def ffi_check(session: nox.Session):
-    extra_args = []
-    # This flag can be useful for debugging ffi-check errors, but overall the
-    # short message format is easier to read
-    if "--long-message-format" not in session.posargs:
-        extra_args.append("--message-format=short")
-
-    _run_cargo(session, "run", _FFI_CHECK, *extra_args)
+    # on windows, missing symbols are reported best at link time against a
+    # proper import library, so running with raw dylib disabled gets the best
+    # feedback. Exercise both paths.
+    no_raw_dylib_env = {**os.environ, "PYO3_USE_RAW_DYLIB": "0"}
+    raw_dylib_env = {**os.environ, "PYO3_USE_RAW_DYLIB": "1"}
+    if sys.platform == "win32":
+        # only relevant to run this on windows; the env var is ignored on
+        # other platforms
+        _run_cargo(session, "run", _FFI_CHECK, env=no_raw_dylib_env)
+    _run_cargo(session, "run", _FFI_CHECK, env=raw_dylib_env)
     _check_raw_dylib_macro(session)
 
 

pyo3-ffi-check/definitions/Cargo.toml

@@ -8,5 +8,6 @@ publish = false
 pyo3-ffi = { path = "../../pyo3-ffi" }
 
 [build-dependencies]
-bindgen = "0.72"
+bindgen = "0.73"
+target-lexicon = "0.13"
 pyo3-build-config = { path = "../../pyo3-build-config" }

pyo3-ffi-check/definitions/build.rs

@@ -1,7 +1,8 @@
 use std::env;
 use std::path::PathBuf;
 
-use bindgen::callbacks::ItemInfo;
+use bindgen::callbacks::{ItemInfo, ItemKind};
+use target_lexicon::{Architecture, OperatingSystem, Triple};
 
 #[derive(Debug)]
 struct ParseCallbacks;
@@ -20,12 +21,14 @@ impl bindgen::callbacks::ParseCallbacks for ParseCallbacks {
 }
 
 #[derive(Debug)]
-struct PyPyReplaceCallbacks;
+struct WindowsX86RawDylibCallbacks;
 
-impl bindgen::callbacks::ParseCallbacks for PyPyReplaceCallbacks {
-    fn item_name(&self, item_info: ItemInfo<'_>) -> Option<String> {
-        if item_info.name.starts_with("PyPy") || item_info.name.starts_with("_PyPy") {
-            Some(item_info.name.replacen("PyPy", "Py", 1))
+// Matches the adjustment in `pyo3-ffi` to force the link name for functions starting
+// with `_Py` (see `pyo3-ffi/src/impl_/macros.rs`)
+impl bindgen::callbacks::ParseCallbacks for WindowsX86RawDylibCallbacks {
+    fn generated_link_name_override(&self, item: ItemInfo<'_>) -> Option<String> {
+        if item.kind == ItemKind::Function && item.name.starts_with("_Py") {
+            Some(format!("_{}", item.name))
         } else {
             None
         }
@@ -34,12 +37,13 @@ impl bindgen::callbacks::ParseCallbacks for PyPyReplaceCallbacks {
 
 fn main() {
     let config = pyo3_build_config::get();
+    let target: Triple = env::var("TARGET").unwrap().parse().unwrap();
 
     let python_include_dir = config
         .run_python_script(
             "import sysconfig; print(sysconfig.get_config_var('INCLUDEPY'), end='');",
         )
-        .expect("failed to get lib dir");
+        .expect("failed to get include dir");
     let gil_disabled_on_windows = config
         .run_python_script(
             "import sysconfig; import platform; print(sysconfig.get_config_var('Py_GIL_DISABLED') == 1 and platform.system() == 'Windows');",
@@ -62,40 +66,31 @@ fn main() {
         .clang_args(clang_args)
         .parse_callbacks(Box::new(bindgen::CargoCallbacks::new()))
         .parse_callbacks(Box::new(ParseCallbacks))
-        .blocklist_item("memcpy")
-        .blocklist_item("memmove")
-        .blocklist_item("memset")
-        .blocklist_item("memcmp")
-        .blocklist_item("strlen")
-        .blocklist_item("bcmp");
+        // Minimising bindgen output to `Py` symbols and their dependencies, avoiding
+        // system declarations etc which are not relevant to `pyo3-ffi-check`.
+        .allowlist_type("_?Py.*")
+        .allowlist_function("_?Py.*")
+        .allowlist_var("_?Py.*|PY.*");
 
-    if matches!(
-        config.implementation(),
-        pyo3_build_config::PythonImplementation::PyPy
-    ) {
-        builder = builder.parse_callbacks(Box::new(PyPyReplaceCallbacks));
+    // Match PyO3's choice to use raw-dylib linking on Windows for the bindgen symbols
+    // so that link resolution is done identically
+    if target.operating_system == OperatingSystem::Windows {
+        println!("cargo:rerun-if-env-changed=PYO3_USE_RAW_DYLIB");
+        let lib_name = config.lib_name().expect("missing Python library name");
+        if env::var("PYO3_USE_RAW_DYLIB").map_or(true, |value| value == "1") {
+            let import_name_type = if matches!(target.architecture, Architecture::X86_32(_)) {
+                builder = builder.parse_callbacks(Box::new(WindowsX86RawDylibCallbacks));
+                ", import_name_type = \"undecorated\""
+            } else {
+                ""
+            };
+            builder = builder.extern_block_attrs(format!(
+                "#[link(name = \"{lib_name}\", kind = \"raw-dylib\"{import_name_type})]"
+            ));
+        }
     }
 
-    let bindings = builder
-        // blocklist some values which apparently have conflicting definitions on unix
-        .blocklist_item("FP_NORMAL")
-        .blocklist_item("FP_SUBNORMAL")
-        .blocklist_item("FP_NAN")
-        .blocklist_item("FP_INFINITE")
-        .blocklist_item("FP_INT_UPWARD")
-        .blocklist_item("FP_INT_DOWNWARD")
-        .blocklist_item("FP_INT_TOWARDZERO")
-        .blocklist_item("FP_INT_TONEARESTFROMZERO")
-        .blocklist_item("FP_INT_TONEAREST")
-        .blocklist_item("FP_ZERO")
-        // blocklist mingw specific types
-        .blocklist_type("__mingw_ldbl_type_t")
-        // ARM neon intrinsics cause issue on GitHub actions windows CI, also not relevant to
-        // what we're trying to check anyway.
-        .blocklist_file(r".*(\\|/)arm(64)?_neon\.h")
-        .blocklist_file(r".*(\\|/)arm_vector_types\.h")
-        .generate()
-        .expect("Unable to generate bindings");
+    let bindings = builder.generate().expect("Unable to generate bindings");
 
     let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
     bindings

pyo3-ffi-check/macro/src/lib.rs

@@ -6,7 +6,7 @@ use std::{
 };
 
 use proc_macro2::{Ident, Span, TokenStream, TokenTree};
-use pyo3_build_config::PythonVersion;
+use pyo3_build_config::{PythonImplementation, PythonVersion};
 use quote::quote;
 
 const PY_3_15: PythonVersion = PythonVersion {
@@ -195,6 +195,7 @@ pub fn for_all_fields(input: proc_macro::TokenStream) -> proc_macro::TokenStream
         let bindgen_field_ident = if (pyo3_build_config::get().target_abi().version() >= PY_3_12)
             && struct_name == "PyObject"
             && field_name == "ob_refcnt"
+            && pyo3_build_config::get().target_abi().implementation() != PythonImplementation::PyPy
         {
             // PyObject since 3.12 implements ob_refcnt as a union; bindgen creates
             // an anonymous name for the field
@@ -452,9 +453,6 @@ const MACRO_EXCLUSIONS: &[(&str, &str)] = &[
     ("Py_UNICODE_TODECIMAL", ""),
     ("Py_XDECREF", ""),
     ("Py_XINCREF", ""),
-    ("_PyCode_GetExtra", "Py_3_12"),
-    ("_PyCode_SetExtra", "Py_3_12"),
-    ("_PyEval_RequestCodeExtraIndex", "Py_3_12"),
     // These functions were only added in 3.10, but pyo3-ffi defines them for
     // all versions. Technically not macros but the machinery happens to work
     // the same way.
@@ -495,6 +493,17 @@ const EXCLUDED_SYMBOLS: &[&str] = &[
     "PyOS_BeforeFork",
     "PyOS_AfterFork_Parent",
     "PyOS_AfterFork_Child",
+    // TODO: PyPy 3.12 declares these symbols in its headers but does not implement them?
+    "PyMapping_Length",
+    "PyObject_IS_GC",
+    "PyObject_Length",
+    "PySequence_In",
+    "PySequence_Length",
+    "PyType_ClearCache",
+    // TODO: deprecated backwards compatibility aliases to be removed in PyO3 0.31
+    "_PyCode_GetExtra",
+    "_PyCode_SetExtra",
+    "_PyEval_RequestCodeExtraIndex",
 ];
 
 // Assert at compile time that `MACRO_EXCLUSIONS` and `EXCLUDED_SYMBOLS` are disjoint
@@ -548,13 +557,20 @@ pub fn for_all_functions(_input: proc_macro::TokenStream) -> proc_macro::TokenSt
             continue;
         }
 
-        if pyo3_build_config::get().implementation()
-            == pyo3_build_config::PythonImplementation::PyPy
-        {
+        let mut bindgen_name = function_name.to_owned();
+        if pyo3_build_config::get().implementation() == PythonImplementation::PyPy {
+            // For PyPy, some functions are prefixed with "PyPy", we check whether the
+            // bindgen name contains the prefixed name and use that if it does.
+            if function_name.starts_with("Py") || function_name.starts_with("_Py") {
+                let prefixed_name = function_name.replacen("Py", "PyPy", 1);
+                if BINDGEN_FUNCTION_NAMES.contains(&prefixed_name) {
+                    bindgen_name = prefixed_name;
+                }
+            }
             // If the function doesn't exist in PyPy, for now we don't care:
             // - For PyO3 inline functions it's probably fine to include anyway
             // - For extern symbols - PyPy may add them in a future release
-            if !BINDGEN_FUNCTION_NAMES.contains(function_name) {
+            if !BINDGEN_FUNCTION_NAMES.contains(&bindgen_name) {
                 continue;
             }
         }
@@ -614,6 +630,7 @@ pub fn for_all_functions(_input: proc_macro::TokenStream) -> proc_macro::TokenSt
         };
 
         let function_ident = Ident::new(function_name, Span::call_site());
+        let bindgen_ident = Ident::new(&bindgen_name, Span::call_site());
 
         let arg_types = std::iter::repeat_n(quote!(_), arg_count);
 
@@ -640,7 +657,7 @@ pub fn for_all_functions(_input: proc_macro::TokenStream) -> proc_macro::TokenSt
             .map(|(_, cfg)| if cfg.is_empty() { "all()" } else { *cfg })
             .map(|cfg| cfg.parse().expect("failed to parse macro exclusion cfg"));
 
-        let has_symbol = BINDGEN_FUNCTION_NAMES.contains(function_name);
+        let has_symbol = BINDGEN_FUNCTION_NAMES.contains(&bindgen_name);
         match (macro_exclusion_cfg, has_symbol) {
             (Some(cfg), true) => {
                 // emit an error if checking within the cfgs where a macro is expected
@@ -650,7 +667,7 @@ pub fn for_all_functions(_input: proc_macro::TokenStream) -> proc_macro::TokenSt
                 output.extend(quote!(#[cfg(#cfg)] compile_error!(#error_message);));
                 // if not within the macro range, we found a symbol, this should be good
                 output.extend(
-                    quote!(#[cfg(not(#cfg))] #macro_name!(#inline #function_ident, #modifiers (#(#arg_types),* #vararg));),
+                    quote!(#[cfg(not(#cfg))] #macro_name!(#inline #function_ident, #bindgen_ident, #modifiers (#(#arg_types),* #vararg));),
                 );
             }
             (Some(cfg), false) => {
@@ -664,7 +681,7 @@ pub fn for_all_functions(_input: proc_macro::TokenStream) -> proc_macro::TokenSt
             (None, true) => {
                 // emit the comparison macro to check that the argument count matches
                 output.extend(
-                    quote!(#macro_name!(#inline #function_ident, #modifiers (#(#arg_types),* #vararg));),
+                    quote!(#macro_name!(#inline #function_ident, #bindgen_ident, #modifiers (#(#arg_types),* #vararg));),
                 );
             }
             (None, false) => {

pyo3-ffi-check/src/main.rs

@@ -2,6 +2,13 @@ use std::{ffi::CStr, process::exit};
 
 use pyo3_ffi_check_definitions::{bindgen as bindings, pyo3_ffi};
 
+/// Functions which don't have equivalent addresses between pyo3-ffi and bindgen.
+#[cfg(not(PyPy))]
+static SPECIAL_CASE_FUNCTIONS: &[&str] = &[
+    "PyEval_RestoreThread", // PyO3 adds special handling for pthread_exit
+    "PyGILState_Ensure",    // Similar to PyEval_RestoreThread
+];
+
 fn main() {
     println!(
         "comparing pyo3-ffi against headers generated for {}",
@@ -138,38 +145,68 @@ fn main() {
         };
     }
 
+    // Check that the function signatures are compatible between pyo3-ffi and bindgen.
+    //
+    // Typically `name` == `bindgen_name`, but e.g. for PyPy this is not the case.
     macro_rules! check_function {
-        ($name:ident, [$($modifiers:tt)*] ($($arg_types:tt)*)) => {{
-            // Check functions have the same number of arguments
-            #[allow(deprecated)]
-            { pyo3_ffi::$name as $($modifiers)* fn($($arg_types)*) -> _ };
-            bindings::$name as $($modifiers)* fn($($arg_types)*) -> _;
+        ($name:ident, $bindgen_name:ident, [$($modifiers:tt)*] ($($arg_types:tt)*)) => {{
+
+            #[cfg(not(PyPy))]
+            {
+                // Check functions have the same number of arguments
+                #[allow(deprecated)]
+                let pyo3_ffi_fn = { pyo3_ffi::$name as $($modifiers)* fn($($arg_types)*) -> _ };
+                let bindgen_fn = bindings::$bindgen_name as $($modifiers)* fn($($arg_types)*) -> _;
+
+                // Check function addresses are the same (i.e. link is configured as expected).
+                // This will also trigger build errors if linker fails to find the symbol pyo3-ffi
+                // is expecting.
+                if !std::ptr::fn_addr_eq(pyo3_ffi_fn, bindgen_fn)
+                    && !SPECIAL_CASE_FUNCTIONS.contains(&stringify!($name))
+                {
+                    failed = true;
+                    println!(
+                        "error: function address of {} differs between pyo3_ffi ({:p}) and bindgen ({:p})",
+                        stringify!($name),
+                        pyo3_ffi_fn,
+                        bindgen_fn
+                    );
+                }
+            }
+
+            #[cfg(PyPy)] // FIXME https://github.com/PyO3/pyo3/pull/6389
+            {
+                // Check functions have the same number of arguments
+                #[allow(deprecated)]
+                { pyo3_ffi::$name as $($modifiers)* fn($($arg_types)*) -> _ };
+                bindings::$bindgen_name as $($modifiers)* fn($($arg_types)*) -> _;
+            }
 
             // TODO: can probably sniff arg types by binding sniffers for each argument position and then passing
             // those inside `todo_args!` to use type inference for each argument.
 
             // Check return types are compatible
             #[allow(deprecated)]
             let pyo3_ffi_return_type = ReturnTypeSniffer::new(|| unsafe { todo_args!((pyo3_ffi::$name)($($arg_types)*)) });
-            let bindgen_return_type = ReturnTypeSniffer::new(|| unsafe { todo_args!((bindings::$name)($($arg_types)*)) });
+            let bindgen_return_type = ReturnTypeSniffer::new(|| unsafe { todo_args!((bindings::$bindgen_name)($($arg_types)*)) });
 
             failed |= !ReturnTypeSniffer::check_compatible(stringify!($name), &pyo3_ffi_return_type, &bindgen_return_type);
         }};
         // case when the function is an inline function in the headers, in which case pyo3-ffi will use the
         // Rust abi and the extern symbol uses the C abi
-        (@inline $name:ident, ($($arg_types:tt)*)) => {{
+        (@inline $name:ident, $bindgen_name:ident, ($($arg_types:tt)*)) => {{
             // Check functions have the same number of arguments
             #[allow(deprecated)]
             { pyo3_ffi::$name as unsafe fn($($arg_types)*) -> _ };
-            bindings::$name as unsafe extern "C" fn($($arg_types)*) -> _;
+            bindings::$bindgen_name as unsafe extern "C" fn($($arg_types)*) -> _;
 
             // TODO: can probably sniff arg types by binding sniffers for each argument position and then passing
             // those inside `todo_args!` to use type inference for each argument.
 
             // Check return types are compatible
             #[allow(deprecated)]
             let pyo3_ffi_return_type = ReturnTypeSniffer::new(|| unsafe { todo_args!((pyo3_ffi::$name)($($arg_types)*)) });
-            let bindgen_return_type = ReturnTypeSniffer::new(|| unsafe { todo_args!((bindings::$name)($($arg_types)*)) });
+            let bindgen_return_type = ReturnTypeSniffer::new(|| unsafe { todo_args!((bindings::$bindgen_name)($($arg_types)*)) });
 
             failed |= !ReturnTypeSniffer::check_compatible(stringify!($name), &pyo3_ffi_return_type, &bindgen_return_type);
         }};

pyo3-ffi/build.rs

@@ -276,6 +276,30 @@ fn emit_link_config(build_config: &BuildConfig) -> Result<()> {
         return Ok(());
     }
 
+    // Not using raw-dylib linking: PyPy dll needs to be the import library not the DLL name
+    let lib_name = if interpreter_config.target_abi().implementation() == PythonImplementation::PyPy
+        && target_os == "windows"
+    {
+        // FIXME: this should probably be done with better configuration in pyo3-build-config
+        // for `raw-dylib` in general, rather than as a patch here.
+        //
+        // Assert expected raw pypy dll name as a sanity check for now
+        assert_eq!(
+            lib_name,
+            format!(
+                "libpypy3.{}-c",
+                interpreter_config.target_abi().version().minor
+            )
+        );
+        format!(
+            "python{}{}",
+            interpreter_config.target_abi().version().major,
+            interpreter_config.target_abi().version().minor
+        )
+    } else {
+        lib_name.to_string()
+    };
+
     println!(
         "cargo:rustc-link-lib={link_model}{alias}{lib_name}",
         link_model = if interpreter_config.shared() {

pyo3-ffi/src/abstract_.rs

@@ -75,7 +75,7 @@ extern_libpython! {
     #[cfg_attr(PyPy, link_name = "PyPyVectorcall_NARGS")]
     pub fn PyVectorcall_NARGS(nargsf: size_t) -> Py_ssize_t;
 
-    #[cfg_attr(not(any(Py_3_12, PyPy)), link_name = "_PyVectorcall_Call")] // symbol made public in 3.12
+    #[cfg(any(Py_3_12, not(Py_LIMITED_API)))]
     #[cfg_attr(PyPy, link_name = "PyPyVectorcall_Call")]
     pub fn PyVectorcall_Call(
         callable: *mut PyObject,

pyo3-ffi/src/impl_/macros.rs

@@ -15,7 +15,7 @@
 macro_rules! extern_libpython_cpython_private_fn {
     ($(#[$attrs:meta])* $vis:vis $name:ident($($args:tt)*) $(-> $ret:ty)?) => {
         #[cfg_attr(
-            all(windows, target_arch = "x86", not(any(PyPy, GraalPy))),
+            all(windows, pyo3_use_raw_dylib, target_arch = "x86"),
             link_name = concat!("_", stringify!($name))
         )]
         $(#[$attrs])*