extend pyo3-ffi-check to check functions#5965
Conversation
|
Let me know if you want some support with this. I'd really like to extend the FFI checking to abi3 and abi3t as well. |
davidhewitt
added
the
CI-no-fail-fast
|
Help is welcome, yes. Fixing all the definitions for all versions looks like it might take a bit of work, worse I am running into variants of #4882 when trying to try to test for different versions. Maybe I will first try to get #5862 merged first which will simplify the build process and make it easier to probe. Feel free to push to this PR in the meanwhile. |
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
I think this is ready for review. It's quite a bit of fiddly machinery; ultimately what we gain is:
- We have a way to verify symbols are correctly exposing either an exported symbol, a static inline function, or a macro reimplementation.
- We have a way to verify that all functions have the correct argument count (not necessarily correct types).
This has caught quite a number of cases where we've got slightly incorrect definitions one way or another. I originally started fixing in this PR, but I decided to roll that work back because the fixes became numerous enough that I felt they deserved their own PRs.
One exception - I updated a bunch of functions which are removed on Python 3.15, as prerelease software I didn't think they needed a changelog and it meant we got to a green CI in fewer hops.
| // TODO: probably need to clean these up | ||
| const EXCLUDED_SYMBOLS: &[&str] = &[ | ||
| // CPython deprecated these but the symbols still exist, pyo3-ffi will probably clean them up anyway | ||
| "_PyCode_GetExtra", | ||
| "_PyCode_SetExtra", | ||
| "_PyEval_RequestCodeExtraIndex", | ||
| // FIXME: probably outdated definitions that fail to build, need investigation, | ||
| // temporarily here to make the build pass to get CI running | ||
| "_PyFloat_CAST", | ||
| "_PyObject_MakeTpCall", | ||
| "_PyRun_AnyFileObject", | ||
| "_PyRun_InteractiveLoopObject", | ||
| "_PyRun_SimpleFileObject", | ||
| "_PySequence_IterSearch", | ||
| "_PySet_NextEntry", | ||
| "_PyUnicode_CheckConsistency", | ||
| "_Py_CheckFunctionResult", | ||
| "PyCode_New", | ||
| "PyCode_NewWithPosOnlyArgs", | ||
| "PyCFunction_New", | ||
| "PyObject_GET_WEAKREFS_LISTPTR", | ||
| "PyFrame_BlockSetup", | ||
| "PySys_AddWarnOption", | ||
| "PySys_AddWarnOptionUnicode", | ||
| "PySys_AddXOption", | ||
| "PySys_HasWarnOptions", | ||
| "PySys_SetPath", | ||
| "PyUnicode_ClearFreeList", | ||
| "PyUnicode_Encode", | ||
| "PyUnicode_EncodeASCII", | ||
| "PyUnicode_EncodeCharmap", | ||
| "PyUnicode_EncodeDecimal", | ||
| "PyUnicode_EncodeLatin1", | ||
| "PyUnicode_EncodeRawUnicodeEscape", | ||
| "PyUnicode_EncodeUTF7", | ||
| "PyUnicode_EncodeUTF8", | ||
| "PyUnicode_EncodeUTF16", | ||
| "PyUnicode_EncodeUTF32", | ||
| "PyUnicode_EncodeUnicodeEscape", | ||
| "PyUnicode_TransformDecimalToASCII", | ||
| "PyUnicode_TranslateCharmap", | ||
| "_Py_HashBytes", | ||
| // This symbol was not in headers but still public until Python 3.10, | ||
| // should be able to remove this exclusion once support for 3.9 dropped | ||
| "Py_GetArgcArgv", | ||
| // pyo3-ffi defined these functions for 3.8 but they only exist for 3.9+ | ||
| "PyObject_CallOneArg", | ||
| "PyObject_Vectorcall", | ||
| "PyVectorcall_Function", | ||
| "PyObject_VectorcallDict", | ||
| // Needs fixing: since 3.9 it takes thread state as first argument | ||
| "_PyEval_EvalFrameDefault", | ||
| // Needs fixing: argument count is wrong on 3.13 against headers | ||
| "_PyLong_AsByteArray", | ||
| // CPython gates these on a HAVE_FORK macro, pyo3-ffi needs to replicate this? | ||
| "PyOS_BeforeFork", | ||
| "PyOS_AfterFork_Parent", | ||
| "PyOS_AfterFork_Child", | ||
| // Private symbols that pyo3-ffi should stop exporting | ||
| "_PyUnicode_COMPACT_DATA", | ||
| "_PyUnicode_NONCOMPACT_DATA", | ||
| "_PyUnicode_Ready", | ||
| // See https://github.com/python/cpython/pull/139166/changes#r3214904694 | ||
| "Py_IS_TYPE", | ||
| "Py_SIZE", | ||
| ]; |
There was a problem hiding this comment.
All these symbols are potentially slightly wrong against one or more Python versions; I would like to fix these however I think it makes sense to do so in one or more PRs after this PR itself is merged.
|
|
||
| [options] | ||
| exclude-newer = "0001-01-01T00:00:00Z" # This has no effect and is included for backwards compatibility when using relative exclude-newer values. | ||
| exclude-newer-span = "P7D" |
There was a problem hiding this comment.
Was this intended to come in with this PR?
There was a problem hiding this comment.
Given all the security breaches, I set a global cooldown in my uv settings. I can't find a way to prevent uv from subsequently re-locking.
Given it should be harmless, I was happy to let this in, I'll revert it later if it's a problem? Otherwise I get this diff on my local working copy every patch I work on :/
| "PyCode_NewWithPosOnlyArgs", | ||
| "PyCFunction_New", | ||
| "PyObject_GET_WEAKREFS_LISTPTR", | ||
| "PyFrame_BlockSetup", |
There was a problem hiding this comment.
There are a number of symbols that appear in this list and the list above. Was that intentional? If not, probably best to de-duplicate.
There was a problem hiding this comment.
Good catch, I wrote a compile-time assert there are no deduplicates and cleaned this up 👍
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
2 participants
This attempts to add more correctness to
pyo3-ffiby validating that external function symbols actually exist and have the same number of arguments. This is quite a step forward, wherepyo3-ffi-checkcurrently doesn't check anything for functions.