Skip to content

[3.8] bpo-39016: type_mro_modified produces negative refcount#17555

Closed
ctismer wants to merge 1 commit intopython:3.8from
pyside:3.8-refcount
Closed

[3.8] bpo-39016: type_mro_modified produces negative refcount#17555
ctismer wants to merge 1 commit intopython:3.8from
pyside:3.8-refcount

Conversation

@ctismer
Copy link
Copy Markdown
Contributor

@ctismer ctismer commented Dec 10, 2019

In the "if (custom) {" branch, lookup_maybe_method, which uses
borrowed references, is not handled correctly.
Solved by removing all PyXDECREF calls.

https://bugs.python.org/issue39016

It the "if (custom) {" branch, lookup_maybe_method, which uses
borrowed references, is not handled correctly.
Solved by removing all PyXDECREF calls.
@pablogsal pablogsal changed the base branch from 3.8 to master December 10, 2019 16:20
@pablogsal pablogsal changed the base branch from master to 3.8 December 10, 2019 16:20
@pablogsal
Copy link
Copy Markdown
Member

@ctismer Can you open the PR against master instead? We will backport the change to 3.8 after is merged.

@pablogsal pablogsal closed this Dec 10, 2019
@ambv ambv reopened this Dec 10, 2019
@pablogsal pablogsal changed the title bpo-39016: type_mro_modified produces negative refcount [3.8] bpo-39016: type_mro_modified produces negative refcount Dec 10, 2019
@ctismer
Copy link
Copy Markdown
Contributor Author

ctismer commented Dec 10, 2019

No guys, this "if (custom)" part is only in the 3.8 branch.
It makes no sense to use master. Maybe this is just a leftover and should be removed?

@pablogsal
Copy link
Copy Markdown
Member

pablogsal commented Dec 10, 2019

No guys, this "if (custom)" part is only in the 3.8 branch.
It makes no sense to use master.

Yeah, apologies for the confusion. I answered in the issue as well.

This still needs a NEWS entry and ideally a test.

@vstinner
Copy link
Copy Markdown
Member

No guys, this "if (custom)" part is only in the 3.8 branch.

Would it make any sense to reuse master code in 3.8?

@ctismer
Copy link
Copy Markdown
Contributor Author

ctismer commented Dec 11, 2019

No guys, this "if (custom)" part is only in the 3.8 branch.

Would it make any sense to reuse master code in 3.8?

No idea. I was not aware that 3.8 and master diverge that much. Should the "if (custom) {" clause perhaps be adjusted/removed like in master?

Update: master is probably a bit behind Python 3.8, see the comment in type_mro_modified. So I would probably use the corrected 3.8 version (no Py_XDECREF's) and apply it to master as well.

It was a change by Julien Palard from 2019-05-26, 180dc1b
Not sure why that did not go into master. Was added with good intention but bad refcounting ;-)

@ambv
Copy link
Copy Markdown
Contributor

ambv commented Dec 11, 2019

@JulienPalard, can you shed some light why your change only went into 3.8 and not into master?

@encukou
Copy link
Copy Markdown
Member

encukou commented Dec 11, 2019

There's some misunderstanding there?
Julien's change is in master, and this patch applies cleanly.

@encukou
Copy link
Copy Markdown
Member

encukou commented Dec 11, 2019

Unfortunately, this isn't the right fix :(
Let's discuss more in the issue.

@ctismer
Copy link
Copy Markdown
Contributor Author

ctismer commented Dec 11, 2019

Sorry, I believe that I was mislead and thought that lookup_maybe_method returns a borrowed reference. This was a mistake, _PyType_Lookup returns a borrowed reference. Closing the issue so far :-(

@ctismer ctismer closed this Dec 11, 2019
@ctismer ctismer deleted the 3.8-refcount branch December 11, 2019 22:34
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.

7 participants