[issue34522] PyTypeObject's tp_base initialization bug

[STINNER Victor]

Change by STINNER Victor :


--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Eric Snow]

Change by Eric Snow :


--
nosy: +eric.snow

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Ronald Oussoren]

Ronald Oussoren  added the comment:

Eddie,

I have read the thread and still don't see why breaking existing code is a good 
thing to do.

As I wrote earlier there are two issues you're trying to address with one patch:

1) Not all types in the CPython tree are initialised using PyType_Ready

Fixing that is ok and the patch for that looks ok. But: I haven't checked if 
PyType_Ready is called before types are used, if the call to PyType_Ready is 
done after the type is already implicitly readied the call to PyType_Ready is 
purely cosmetic.

2) Using PyVarObject_HEAD_INIT(_type, ...) causes problems on Windows

I don't agree with the current patch for that because that *silently* changes 
the semantics of existing code, and hence can break existing C extensions. 

I haven't tested the patch yet, but I do have C extensions that use this 
pattern and where "_type" is not equivalent to NULL.

The correct change for the CPython tree is to change all instances of 
PyVarObject_HEAD_INIT(_type, ...) to PyVarObject_HEAD_INIT(NULL, ...), and 
then add explicit code to set the type of the type instance where "some_type" 
is not PyType_Type (AFAIK there aren't any of those in the CPython tree, but 
there are outside of CPython).


Another reason why changing PyVarObject_HEAD_INIT is not correct is that this 
can be used to initialize more than just PyTypeObject structs, as the name 
suggests it is used to initialize PyVarObject values and not just PyTypeObject 
values. Changing the macro also breaks those.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Eddie Elizondo]

Eddie Elizondo  added the comment:

@ronaldoussoren

* This change currently works for all CPython. If you are using this pattern 
then you probably want to be using PyType_FromSpec rather than having a static 
PyTypeObject as discussed in PEP384: https://www.python.org/dev/peps/pep-0384. 
In general, extensions should all be using PyType_FromSpec rather than static 
PyTypeObjects.

* As mentioned by Erik Bray in the e-mail thread, the usage of a static 
PyTypeObject with a PyVarObject_HEAD_INIT requires a compile-time constant. 
This causes problems cross-compatibility problems, especially with Windows. You 
can read more here: 
http://iguananaut.net/blog/programming/windows-data-import.html

In general, this pattern is just pervasive. It causes cross-compatibility 
issues, it can cause users to forget calling PyType_Ready on a type and we are 
better off without it.

Again, please read the mail thread. Everything that I'm saying here is all 
discussed there.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Ronald Oussoren]

Ronald Oussoren  added the comment:

BTW. And that -1 was even before considering that PyVarObject_HEAD_INIT is not 
a type-object specific macro, but a macro to staticky initialize values of 
PyVarObject.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Ronald Oussoren]

Ronald Oussoren  added the comment:

I was also talking about static C types, it is possible to write types with 
custom meta types in C code (even if that is even less common than using meta 
types in the first place). I have extensions where I use PyVarObject_HEAD_INIT 
without the first argument being _Type or NULL and this patch would 
break those.

This part of your patch silently changes the API for CPython in a way that is 
confusing. With your patch this code changes behaviour:

static PyTypeObject my_type = {
 PyVarObject_HEAD_INIT(_meta_type, 0),
 ...
};

This is currently valid code and I use this pattern in my code. It is easy 
enough to adjust for your patch (just add "Py_TYPE(_type) = _meta_type;" 
before the call to PyType_Ready), but that is just a change for changes sake.

Why not just change all invocations of PyVarObject_HEAD_INIT in the CPython 
tree, if any? 

I do agree that explicitly calling PyType_Ready for all types would be better, 
but that is a separate issue from changing the way the static PyTypeObject is 
initialised.  

A patch that does add calls to PyType_Ready should also ensure that this is 
done before types are actually used (otherwise there is a nonzero chance that 
the explicit calls don't actually do anything due to the calls to PyType_Ready 
in strategic places in the interpreter).

Note that your patch tries to accomplish two things:

1) PyType_Ready should be called to initialize types

2) Explicitly setting the type of PyTypeObject instances through 
PyVarObject_HEAD_INIT is not necessary in the common case. 

I agree with the first item, and am -1 on a change for the second item, but -1 
on the current patch for this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Eddie Elizondo]

Eddie Elizondo  added the comment:

@ronaldoussoren Please read the complete analysis from the mailing list: 
https://mail.python.org/pipermail/python-dev/2018-August/154946.html. The 
description here was just a rehash and I probably missed some context. 

Particularly, when I said: "PyTypeObject's ob_type should always be set by 
PyType_Ready" I was referring to the PyTypeObject's that are statically set in 
C code. Metatypes explicitly have to set the ob_type and that's already handled.

In the current state of things, you have static PyTypeObjects that are being 
used before calling PyType_Ready due to this macro. This change just 
standardizes the header of static PyTypeObject throughout the entire codebase.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Ronald Oussoren]

Ronald Oussoren  added the comment:

(added "object model" reviewers to the nosy list)

--
nosy: +benjamin.peterson, twouters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Ronald Oussoren]

Ronald Oussoren  added the comment:

I don't agree with "This means that a PyTypeObject's ob_type should always be 
set by PyType_Ready.", and therefore don't agree with the change to 
PyVarObject_HEAD_INIT.

In particular this is not correct when the type has a custom metatype (e.g. 
ob_type != _Type). 

BTW. I don't understand why changing the invocation of PyVarObject_HEAD_INIT is 
needed at all.  Adding calls to PyType_Ready can be don without that change 
(and I agree with explicitly initialising all types).

--
nosy: +ronaldoussoren

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Eddie Elizondo]

Change by Eddie Elizondo :


--
keywords: +patch
pull_requests: +8432
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34522] PyTypeObject's tp_base initialization bug

[Eddie Elizondo]

New submission from Eddie Elizondo :

>From the documentation, it says that PyType_Ready should be called on `ALL` 
>type objects to finish their initialization 
>(https://docs.python.org/3/c-api/type.html#c.PyType_Ready). This means that a 
>PyTypeObject's ob_type should always be set by PyType_Ready.

It turns out that this is not actually followed by all the core types in 
CPython. This leads to the usage of types that were not initialized through 
PyType_Ready.

This fix modifies PyVarObject_HEAD_INIT to default the type to NULL so that all 
objects have to be fully initialized through PyType_Ready.

Plus:
* It initializes all the objects that were not being initialized through 
PyType_Ready.
* Modifies PyType_Ready to special case the ob_type initialization of 
PyType_Type and PyBaseObject_Type.
* It modifies the edge case of _Py_FalseStruct and _Py_TrueStruct.

Read more: https://mail.python.org/pipermail/python-dev/2018-August/154946.html

--
components: Interpreter Core
messages: 324195
nosy: eelizondo
priority: normal
severity: normal
status: open
title: PyTypeObject's tp_base initialization bug
type: behavior
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com