Skip to content
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

Fix the signature of tp_new #687

Merged
merged 1 commit into from
Aug 21, 2024
Merged

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Aug 19, 2024

According to the docs, the signature of tp_new should be: PyObject* tp_new(PyTypeObject *subtype, PyObject *args, PyObject *kwds)

We don't care about the arguments, but according to the C standard clause 6.3.2.3.8 this is undefined behavior:

A pointer to a function of one type may be converted to a pointer to a
function of another type and back again; the result shall compare equal to
the original pointer. If a converted pointer is used to call a function whose
type is not compatible with the pointed-to type, the behavior is undefined.

In normal platforms the behavior is to work as expected, but in WebAssembly, this crashes.

Fixes pyodide/pyodide#5015.

cc @henryiii @ryanking13

According to the docs, the signature of `tp_new` should be:
`PyObject* tp_new(PyTypeObject *subtype, PyObject *args, PyObject *kwds)`
https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new

We don't care about the arguments, but according to the C standard 6.3.2.3.8
this is undefined behavior:

> A pointer to a function of one type may be converted to a pointer to a
> function of another type and back again; the result shall compare equal to
> the original pointer. If a converted pointer is used to call a function whose
> type is not compatible with the pointed-to type, the behavior is undefined.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf#page=60

In normal platforms the behavior is to work as expected, but in WebAssembly,
this crashes.

Fixes pyodide/pyodide#5015.
@hoodmane
Copy link
Contributor Author

I wonder if we could add Pyodide tests to nanobind CI.

@henryiii
Copy link
Contributor

We've got them in pybind11, so probably?

@hoodmane
Copy link
Contributor Author

The CI uses cmake directly and I was unable to figure out how to get it to stop saying:

Could NOT find Python (missing: Python_INCLUDE_DIRS Development.Module)

So probably someone with a bit more cmake experience would have to do this.

As an aside, I see that people on the internet suggest using python -c "import sysconfig; print(sysconfig.get_path('include'))" to get the include path, which doesn't work correctly in our Pyodide virtual environments. Probably we should fix that...

@leandro-benedet-garcia
Copy link

@hoodmane I will give it a try in another PR

@henryiii
Copy link
Contributor

FYI, in pybind11 I used scikit-build-core to make a tests/pyproject.toml that build the tests as a wheel, then made sure the tests could run against an installed version of the tests binary.

@wjakob
Copy link
Owner

wjakob commented Aug 21, 2024

Oops, that's not good. Thank you for finding and fixing this issue. I am open to including PyIodide in the test matrix—however, someone else would need to create a PR, I don't have the time to pursue this myself.

@wjakob wjakob merged commit 2e266c0 into wjakob:master Aug 21, 2024
24 checks passed
wjakob pushed a commit that referenced this pull request Aug 21, 2024
According to the docs, the signature of `tp_new` should be:
`PyObject* tp_new(PyTypeObject *subtype, PyObject *args, PyObject *kwds)`
https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new

We don't care about the arguments, but according to the C standard 6.3.2.3.8
this is undefined behavior:

> A pointer to a function of one type may be converted to a pointer to a
> function of another type and back again; the result shall compare equal to
> the original pointer. If a converted pointer is used to call a function whose
> type is not compatible with the pointed-to type, the behavior is undefined.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf#page=60

In normal platforms the behavior is to work as expected, but in WebAssembly,
this crashes.

Fixes pyodide/pyodide#5015.
wjakob pushed a commit that referenced this pull request Aug 21, 2024
According to the docs, the signature of `tp_new` should be:
`PyObject* tp_new(PyTypeObject *subtype, PyObject *args, PyObject *kwds)`
https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new

We don't care about the arguments, but according to the C standard 6.3.2.3.8
this is undefined behavior:

> A pointer to a function of one type may be converted to a pointer to a
> function of another type and back again; the result shall compare equal to
> the original pointer. If a converted pointer is used to call a function whose
> type is not compatible with the pointed-to type, the behavior is undefined.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf#page=60

In normal platforms the behavior is to work as expected, but in WebAssembly,
this crashes.

Fixes pyodide/pyodide#5015.
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.

RuntimeError: null function or function signature mismatch in github action
4 participants