Navigation Menu

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

Only set HAVE_VECTORCALL flag when using vectorcall #4453

Merged
merged 4 commits into from Nov 18, 2021

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Nov 5, 2021

Before this the _Py_TPFLAGS_HAVE_VECTORCALL was set all the time when using the limited api, but a CyFunction never sets up a vectorcall function. This caused basically all function calls to crash when binding=True with the limited API.

I've also fixed a refnanny error in the module setup

I think this'll end up vastly increasing the number of tests that pass with the limited API.

Before this the _Py_TPFLAGS_HAVE_VECTORCALL was set all the time
when using the limited api, but a CyFunction never sets up a
vectorcall function. This caused basically all function calls to
crash when binding=True with the limited API.
Comment on lines 3479 to 3483
code.put_gotref(module_temp, py_object_type)
# no gotref - refnanny isn't yet set up
code.putln(code.error_goto_if_neg("PyState_AddModule(%s, &%s)" % (
module_temp, Naming.pymoduledef_cname), self.pos))
code.put_decref_clear(module_temp, type=py_object_type)
code.put_decref_clear(module_temp, type=py_object_type, nanny=False)
code.funcstate.release_temp(module_temp)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't use an allocated temp then but a plain local variable, and do the "err = AddModule(); DECREF(); if (err) ..." dance explicitly. Otherwise, decref-ing the temp in the error case would lead the refnanny astray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Since it'll get cleaned up with refnanny
@@ -3109,7 +3109,7 @@ def generate_module_init_func(self, imported_modules, env, code):
# user code in atexit or other global registries.
##code.put_decref_clear(env.module_dict_cname, py_object_type, nanny=False)
code.putln('}')
code.putln("#if !CYTHON_COMPILING_IN_LIMITED_API")
code.putln("#if !CYTHON_USE_MODULE_STATE")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also changed this (around the cleanup of __pyx_m) since it seemed more tied to CYTHON_USE_MODULE_STATE than to limited API specifically.

They were for testing locally, and aren't a useful bugs list yet
@scoder scoder added this to the 3.0 milestone Nov 18, 2021
@scoder scoder merged commit 01d323a into cython:master Nov 18, 2021
@scoder
Copy link
Contributor

scoder commented Nov 18, 2021

Thanks

@da-woods da-woods deleted the limited-api-vectorcall-fix branch November 18, 2021 17:44
da-woods added a commit to da-woods/cython that referenced this pull request Nov 25, 2021
Follow up to cython#4453.
That PR creates a temp that's only used in the limited API
(and therefore causes warnings on all other paths)
scoder pushed a commit that referenced this pull request Nov 26, 2021
Follow up to #4453 (01d323a).
That PR creates a temp that's only used in the limited API
(and therefore causes warnings on all other paths)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants