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 pushes_fd method signature #5833

Merged
merged 1 commit into from Nov 16, 2021

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Nov 13, 2021

Python getters are supposed to take a void* closure 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.

@hoodmane
Copy link
Contributor Author

@hoodmane hoodmane commented Nov 14, 2021

I also removed all of the (getter) casts, but I'm not sure if that will work correctly since strictly speaking most of the function pointers are defined with type e.g.,

PyObject *
get_some_attr(SpecificTypeOfObject *decoder, void *closure)

whereas the first argument is expected to be a PyObject *. So depending on what error / warning flags are set, this may generate a bunch of warnings or a compile error.

Probably it's better to put the casts back, but it's unfortunate that we can't get better type safety here -- it's so easy to mess up these signatures.

@radarhere
Copy link
Member

@radarhere radarhere commented Nov 15, 2021

It is indeed generating a bunch of warnings - https://github.com/python-pillow/Pillow/runs/4202395176#step:9:26

src/_imaging.c:3571:14: warning: initialization of ‘PyObject * (*)(PyObject *, void )’ {aka ‘struct _object * ()(struct _object *, void )’} from incompatible pointer type ‘PyObject * ()(ImagingObject *, void )’ {aka ‘struct _object * ()(struct *, void *)’} [-Wincompatible-pointer-types]
3571 | {"mode", _getattr_mode},
| ^~~~~~~~~~~~~
src/_imaging.c:3571:14: note: (near initialization for ‘getsetters[0].get’)

@hoodmane
Copy link
Contributor Author

@hoodmane hoodmane commented Nov 15, 2021

Okay I put the casts back.

@hugovk
Copy link
Member

@hugovk hugovk commented Nov 15, 2021

Please could you rebase or redo those two commits, to clean the history up a little bit?

One way:

git reset --soft HEAD~2
git push --force

Getters are supposed to have signature "PyObject *(PyObject *self, void *closure)", but the closure argument is often not used.
In wasm it causes a trap if a function is declared with one argument and then called with two.
@hoodmane hoodmane force-pushed the fix-pushes_fd-signature branch from b083f13 to 7a93328 Compare Nov 15, 2021
@hugovk hugovk merged commit 749754a into python-pillow:main Nov 16, 2021
51 checks passed
@hugovk
Copy link
Member

@hugovk hugovk commented Nov 16, 2021

Thanks!

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