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 METH_VARARGS method signatures #21628

Merged
merged 6 commits into from Nov 15, 2021
Merged

Fix METH_VARARGS method signatures #21628

merged 6 commits into from Nov 15, 2021

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Nov 13, 2021

PR Summary

A bunch of functions are declared with METH_VARARGS calling convention which should have signature PYCFunction. However, they are declared instead with an extra PyObject* kwds argument as if they had METH_VARARGS | METH_KEYWORDS calling convention. They just ignore this argument.

I am coming from Pyodide. Web assembly has no native support for function pointer casting -- a function declared with a certain signature must be called with the same signature, or it will fail with RuntimeError: function signature mismatch. Emscripten has support for emulating function pointer casts which we are currently using, but it imposes a significant performance and stack space cost and we are trying to remove the pointer cast emulation pyodide/pyodide#1677. To do this, we need to patch Python packages so that they have function signatures that match the calling conventions they declare.

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • [N/A] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@hoodmane hoodmane marked this pull request as draft November 13, 2021 21:00
@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 13, 2021

Okay I just realized that I only fixed one file but grepping for METH_VARARGS I find a bunch more methods that have the same problem.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@hoodmane hoodmane marked this pull request as ready for review November 13, 2021 21:13
@hoodmane hoodmane changed the title Fix method signatures in _path_wrapper.cpp Fix METH_VARARGS method signatures Nov 13, 2021
@tacaswell tacaswell added this to the v3.5.1 milestone Nov 14, 2021
@tacaswell
Copy link
Member

tacaswell commented Nov 14, 2021

Does pyodide care about the difference between METH_VARARGS and METH_NOARGS?

@tacaswell
Copy link
Member

I milestoned this as 3.5.1 as I think it is safe to do in a patch release and would like to get this out for pyodide ASAP, but do not want to hold 3.5.0 for it.

@hoodmane
Copy link
Contributor Author

Does pyodide care about the difference between METH_VARARGS and METH_NOARGS?

No it doesn't since they both have signature PyObject* f(PyObject*, PyObject*).

If you want the details: Wasm32 uses 32 bit pointers so from the point of view of the wasm type system this has signature i32 f(i32, i32). It doesn't care about signed vs unsigned integers or pointers or anything, they all look the same. So it would be fine for wasm32 to declare a function as int f(int, int) and call it as PyObject* f(PyObject*, PyObject*).

Web assembly has a call_indirect instruction to call a function pointer. It takes as first argument the signature of the function pointer, as second argument the function pointer, and the remaining arguments are the arguments to the function pointer. These calls are type checked at runtime. If the signature indicated doesn't match the signature of the function, it throws a function signature mismatch error.

The problem here comes from trying to call a function with signature i32 f(i32) via a call_indirect instruction that asserts signature i32 f(i32, i32). The second argument to a METH_NOARGS function is always NULL and other architectures silently ignore it.

@timhoffm
Copy link
Member

@hoodmane Have you overlooked METH_NOARGS functions in _tri_wrapper.cpp?

@hoodmane
Copy link
Contributor Author

Have you overlooked METH_NOARGS functions in _tri_wrapper.cpp?

Looks like it, thanks.

@timhoffm timhoffm merged commit 6552a81 into matplotlib:main Nov 15, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 15, 2021
@tacaswell tacaswell modified the milestones: v3.5.1, v3.5.0 Nov 15, 2021
QuLogic added a commit that referenced this pull request Nov 15, 2021
…628-on-v3.5.x

Backport PR #21628 on branch v3.5.x (Fix METH_VARARGS method signatures )
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.

None yet

3 participants