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

xdecref_cleanup is not always correct #3455

Open
da-woods opened this issue Mar 22, 2020 · 1 comment
Open

xdecref_cleanup is not always correct #3455

da-woods opened this issue Mar 22, 2020 · 1 comment

Comments

@da-woods
Copy link
Contributor

@da-woods da-woods commented Mar 22, 2020

Previously this didn't matter because put_var_decref actually put xdecref:

cython/Cython/Compiler/Code.py

Lines 2185 to 2188 in 4dd7c18

def put_var_decref(self, entry):
if entry.type.is_pyobject:
self.putln("__Pyx_XDECREF(%s);" % self.entry_as_pyobject(entry))

However, "improved" reference counting code #3377 is tripping up on this. To demonstrate use commit 94e2c82 and change line 2080 to decref

code.put_var_xdecref(entry, have_gil=not lenv.nogil)

The first test that fails is bufaccess assign_to_object.

For arguments the issue is in a slightly different place (about 10 lines down in Nodes.py). Here del arg doesn't seem to trigger xdecref_cleanup which is probably pretty easy to fix. Test that fails is "delete"

@da-woods da-woods changed the title variable controlflow xdecref_cleanup not correct controlflow xdecref_cleanup is not correct for variables (seems OK for args) Mar 22, 2020
@da-woods da-woods changed the title controlflow xdecref_cleanup is not correct for variables (seems OK for args) xdecref_cleanup is not always correct Mar 22, 2020
@da-woods

This comment has been minimized.

Copy link
Contributor Author

@da-woods da-woods commented Mar 22, 2020

Possibly more accurate for this issue to be "Cython doesn't always generate the most efficient cleanup reference counting code" rather than blaming xdecref_cleanup which looks to be only considering the arg parsing utility code and is probably fine for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.